Dashboard Events #348

mllocs · 2018-05-01T12:18:11Z

WAT

Using the Persisters to create Events. See #347.

Test

Create and update a post (offer and inquiries)
Create and update a transfer
Create and update a member (add and remove members for orgs basically)

enricostano

Nice POC! 💪

enricostano · 2018-05-01T12:24:09Z

app/models/event.rb

+
+  private
+
+  # Validates that only one resourece id is set


s/resourece/resource

sauloperez

Looking pretty good! tests and 👍

sauloperez · 2018-05-02T11:03:53Z

app/models/event.rb

+  belongs_to :member
+  belongs_to :transfer
+
+  validates :action, inclusion: { :in => ACTIONS }, presence: true


🔥 that #️⃣ 🚀

enricostano · 2018-05-10T20:38:35Z

db/schema.rb

@@ -76,6 +76,9 @@
  add_index "documents", ["documentable_id", "documentable_type"], name: "index_documents_on_documentable_id_and_documentable_type", using: :btree
  add_index "documents", ["label"], name: "index_documents_on_label", using: :btree

+# Could not dump table "events" because of following StandardError
+#   Unknown type 'action_enum' for column 'action'
+


⚠️ a solution is to stop using db/schema.rb and start using db/structure.sql, let's talk about it

cc/ @sauloperez

Another solution is to use the AR built in enum, basically storing an integer instead of a string (classic Rails approach with validation only at model level) or ENUM (DB check level I'm proposing)

I think we are safe enough using AR enum :)

There is another option using AR enum storing integer as I said AND adding check inclusion instead of PG enum. Will propose this option as it seems a good tradeoff to me

sauloperez · 2018-05-11T06:42:13Z

db/migrate/20180501093846_create_events.rb

+        transfer_id integer REFERENCES transfers,
+        created_at  timestamp without time zone,
+        updated_at  timestamp without time zone,
+        CHECK(


I would only do this in SQL by means of an ALTER TABLE. This way we keep the rest using Rails' DSL, which might be easier to read for other devs.

mllocs · 2018-05-13T10:34:53Z

Events and Dashboard

markets · 2018-05-13T11:41:30Z

nice feature here team 👏 🔝

As @mllocs and @sauloperez pointed out, I'd stick with simpler solutions like:

a) string + model validation (inclusion)
b) AR enum

I think both options give us enough data integrity and seems an approach more easy to follow for more people.

rewritten · 2018-05-13T11:53:07Z

I have been bitten badly by cases of inconsistent data representation. Even at the cost of some more effort from the data integrity point of view, it is always much better to see the same from sql and from ruby. I’d vote for either Postgres enums (that appear as strings to the client) or just strings with validation. The former can also suffer from the same issues as ruby-to-sql enumerations in case of adding new items. El El dom, 13 may 2018 a las 13:41, Marc Anguera Insa < notifications@github.com> escribió:

…

nice feature here team 👏 🔝 As @mllocs <https://github.com/mllocs> and @sauloperez <https://github.com/sauloperez> pointed out, I'd stick with simpler solutions like: a) string + model validation (inclusion) b) AR enum I think both options give us enough data integrity and seems an approach more easy to follow for more people. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdG394H_6iE8ALat64c7SdDSvmaZPhrks5tyBvrgaJpZM4Tt37H> .

Conflict of interests 😂

sauloperez · 2018-05-16T10:58:04Z

db/migrate/20180501093846_create_events.rb

+        member_id   integer REFERENCES members,
+        transfer_id integer REFERENCES transfers,
+        created_at  timestamp without time zone,
+        updated_at  timestamp without time zone,


I left I comment about this already but it got collapsed. What about having the regular create_table and then an alter table for these checks that can't be done with AR? I think this would make it a bit more clear that we specify things AR does not provide and perhaps a bit more readable.

sauloperez · 2018-05-16T11:00:09Z

db/migrate/20180501093846_create_events.rb

@@ -0,0 +1,39 @@
+# This migration does not use Rails ActiveRecord ORM DSL since
+# it doesn't provide data integrity (foreign key) for polymorphic models.


I'm not sure I fully understand what you mean.

Read the article 😂

sseerrggii · 2018-05-17T16:28:16Z

Tested

CRUD

Offer/inquiry ✔️
Transfer (only create and update, we can't delete) ✔️
Member ✔️

mllocs requested review from enricostano and sauloperez May 1, 2018 12:18

enricostano previously requested changes May 1, 2018

View reviewed changes

app/models/event.rb Outdated

private

# Validates that only one resourece id is set

Copy link

Contributor

enricostano May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/resourece/resource

sauloperez requested changes May 2, 2018

View reviewed changes

mllocs changed the base branch from feature/introducing-post-persister to develop May 7, 2018 18:31

mllocs force-pushed the feature/dashboard-events branch from 00776b2 to d746b62 Compare May 7, 2018 18:31

enricostano reviewed May 10, 2018

View reviewed changes

sauloperez reviewed May 11, 2018

View reviewed changes

mllocs and others added 7 commits May 16, 2018 10:19

Added dashbaord events

ae0d939

Fixed typo and hashrocket

d7b3f60

Added Event model specs

17a20bc

Update and configure shoulda-matchers

780d733

Modify events table migration with plain SQL

b08f271

Add more specs to Event model

0da31cb

Use Rails enum, backed by an integer column with CHECK constraint

43a0f4f

enricostano force-pushed the feature/dashboard-events branch from afcb387 to 43a0f4f Compare May 16, 2018 08:23

Fix and improve specs

898ee75

sauloperez reviewed May 16, 2018

View reviewed changes

Use ActiveRecord DSL as much as possible

0eb2536

sauloperez approved these changes May 17, 2018

View reviewed changes

enricostano added 3 commits May 17, 2018 08:53

Create :updated event on Post update

833bf30

Create :created and :updated event for Member

dabf7e4

Create :created and :updated event for Transfer

b495581

enricostano added the needs testing label May 17, 2018

enricostano removed the needs testing label May 18, 2018

enricostano merged commit f26377d into develop May 18, 2018

enricostano deleted the feature/dashboard-events branch May 18, 2018 06:49

enricostano mentioned this pull request Aug 8, 2018

Release v1.6.0 #394

Merged

		@@ -0,0 +1,39 @@
		# This migration does not use Rails ActiveRecord ORM DSL since
		# it doesn't provide data integrity (foreign key) for polymorphic models.

Dashboard Events #348

Dashboard Events #348

Uh oh!

Conversation

mllocs commented May 1, 2018 • edited by enricostano Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WAT

Test

Uh oh!

enricostano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauloperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mllocs commented May 13, 2018

Uh oh!

markets commented May 13, 2018

Uh oh!

rewritten commented May 13, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sseerrggii commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tested

CRUD

Uh oh!

Uh oh!

mllocs commented May 1, 2018 •

edited by enricostano

Loading

sseerrggii commented May 17, 2018 •

edited

Loading