Manage and display members (#8) #74

coaxial · 2015-11-25T02:51:43Z

Add a Member model + tests and add it to the admin panel

sophiedeziel · 2015-11-25T02:52:51Z

spec/factories/members.rb

+biography "MyText"
+  end
+
+end


Ca we correct indentation here please?

coaxial · 2015-11-25T14:26:42Z

cc @GalkaS

nicholasjhenry · 2015-11-27T02:33:26Z

@coaxial Would love to merge. Are you up for rebasing with master?

coaxial · 2015-11-27T03:23:23Z

I'm getting weird conflicts, it looks like unresolved things were committed?

diff --cc app/dashboards/event_dashboard.rb
index 48b312c,715bbe6..0000000
--- a/app/dashboards/event_dashboard.rb
+++ b/app/dashboards/event_dashboard.rb
@@@ -10,28 -10,12 +10,34 @@@ class EventDashboard < Administrate::Ba
    ATTRIBUTE_TYPES = {
      location: Field::BelongsTo,
      id: Field::Number,
 +<<<<<<< HEAD
 +<<<<<<< HEAD
++<<<<<<< HEAD
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
      starts_at: Field::DateTime,
      created_at: Field::DateTime,
      updated_at: Field::DateTime,
      title: Field::String,
      introduction: Field::Text,
      conclusion: Field::Text
++<<<<<<< HEAD
 +=======
 +    type: Field::String,
 +    starts_at: Field::DateTime,
 +    created_at: Field::DateTime,
 +    updated_at: Field::DateTime,
 +>>>>>>> #14 : Add events to administrate
 +=======
 +    starts_at: Field::DateTime,
 +    created_at: Field::DateTime,
 +    updated_at: Field::DateTime,
 +    title: Field::String,
 +    introduction: Field::Text,
 +    conclusion: Field::Text
 +>>>>>>> #14 : Add title, introduction, conclusion.
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
    }

    # COLLECTION_ATTRIBUTES
@@@ -40,20 -24,10 +46,26 @@@
    # By default, it's limited to four items to reduce clutter on index pages.
    # Feel free to add, remove, or rearrange items.
    COLLECTION_ATTRIBUTES = [
 +<<<<<<< HEAD
++<<<<<<< HEAD
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
      :id,
      :title,
      :starts_at,
      :location,
++<<<<<<< HEAD
 +=======
 +    :location,
 +    :id,
 +    :starts_at,
 +<<<<<<< HEAD
 +>>>>>>> #14 : Add events to administrate
 +=======
 +    :title,
 +>>>>>>> #14 : Add title, introduction, conclusion.
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
    ]

    # SHOW_PAGE_ATTRIBUTES
@@@ -64,34 -38,17 +76,46 @@@
    # an array of attributes that will be displayed
    # on the model's form (`new` and `edit`) pages.
    FORM_ATTRIBUTES = [
 +<<<<<<< HEAD
++<<<<<<< HEAD
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
      :title,
      :introduction,
      :conclusion,
      :starts_at,
      :location,
++<<<<<<< HEAD
 +=======
 +    :location,
 +    :starts_at,
 +<<<<<<< HEAD
 +>>>>>>> #14 : Add events to administrate
 +=======
 +    :title,
 +    :introduction,
 +    :conclusion
 +>>>>>>> #14 : Add title, introduction, conclusion.
++=======
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
    ]

    # Overwrite this method to customize how events are displayed
    # across all pages of the admin dashboard.
    #
 +<<<<<<< HEAD
++<<<<<<< HEAD
 +  def display_resource(event)
 +     event.title
 +  end
 +=======
 +  # def display_resource(event)
 +  #   "Event ##{event.id}"
 +  # end
 +>>>>>>> #14 : Add events to administrate
++=======
+   def display_resource(event)
+      event.title
+   end
++>>>>>>> 359248fe323be0350abb0ebbe192da3c120e431d
  end

nicholasjhenry · 2015-11-27T03:38:35Z

@coaxial I was able to rebase this on my machine. Let's me know if you need a hand, happy to help. I didn't see any unresolved commits. Please let me know if you see any.

benichu · 2015-12-01T00:40:52Z

@coaxial Would you like us to rebase that branch before merging?
Thanks for your help :)

nicholasjhenry · 2015-12-01T02:05:52Z

@coaxial and I will work on this tomorrow.

cc @benichu

nicholasjhenry · 2015-12-01T23:42:11Z

This is ready for review if anybody is up for it.

cawel · 2015-12-02T06:39:49Z

Here are a few comments in disguise of a code review:

More important:

since the email and name attributes are required to be present at the application level (in the model), I would also enforce that at the database level with non-null db constraints (well, at least for the email).
I would add a db unique index for that email column
I see that a User has_one :member, and that a Member has_one :user. However, neither model has a foreign key. Fishy ! 🐟

Less important:

I think it's more idiomatic to write:

expect(member).to_not be_valid

instead of:

expect(member.valid?).to be false

except for the uniqueness validation, it is not necessary to persist the model in order to check for the validation errors. You can do:

member = Member.new(email: nil)
expect(member).to_not be_valid

Terser, and also tests will run faster 🚀

nicholasjhenry · 2015-12-02T14:53:35Z

To expand on @cawel idiomatic comments:

expect(member).to_not be_valid

can also be written as

expect(member).to be_invalid

Positive expectations/assertions are often a little easier to read.

cawel · 2015-12-02T14:58:17Z

Better indeed.

coaxial · 2015-12-02T16:07:27Z

Thank you for the feedback, much appreciated. We'll work on it tonight with @GalkaS

cawel · 2015-12-02T16:12:08Z

@nicholasjhenry should probably jump in before you refactor tonight, as he seemed to have (during the workshop) a clear idea in which db table to put that foreign key (either in 'users' table or 'members' table).

GalkaS · 2015-12-03T03:56:52Z

app/models/member.rb

+class Member < ActiveRecord::Base
+  has_many :events
+  has_many :organizations
+  has_one :user


Should we write a test for this association?

@GalkaS I don't test the presence of associations specifically -- higher level tests will verify if this association exists -- i.e the app will blow up pretty quickly when doing a request spec for example.

nicholasjhenry · 2015-12-03T14:02:18Z

@nicholasjhenry should probably jump in before you refactor tonight, as he seemed to have (during the workshop) a clear idea in which db table to put that foreign key (either in 'users' table or 'members' table).

Sorry I couldn't address this earlier. The foreign key should really be placed on the member. i.e. a member has_one user. The reason for this is that we want to have our dependencies point towards the user class, not the user class having dependencies on other classes in our system. This avoids the user class becoming a god object.

I understand that you placed it on the user as the foreign key in Administrate doesn't support belongs_to relationships out of the box. I have sometime to look at this tomorrow so I will do some investigation on what the solution to this is.

Thank you for taking the time to make this amendment, and again sorry I didn't respond earlier.

cawel · 2015-12-03T16:19:54Z

The foreign key should really be placed on the member. i.e. a member has_one user.

Note that based on his explanation, @nicholasjhenry probably meant to write: a member *belongs_to* user.

nicholasjhenry · 2015-12-04T00:11:44Z

@cawel 👍

nicholasjhenry · 2015-12-04T19:39:20Z

OK, just shows how confused I was 🙀

The current version of Administrate we have does not support has_one relationships. So moving the foreign_key to members.user_id will work as that will be a belongs_to relationship. Let me know if you're up for changing this. Thank you!

Thanks to @cawel for keeping my associations speak honest.

houndci-bot · 2016-03-10T02:04:40Z

app/dashboards/talk_dashboard.rb

+  # Each different type represents an Administrate::Field object,
+  # which determines how the attribute is displayed
+  # on pages throughout the dashboard.
+  ATTRIBUTE_TYPES = {


Freeze mutable objects assigned to constants.

sophiedeziel · 2016-03-10T03:48:54Z

I fully tested the feature on my machine and I fixed stuff in a way that doesn't break anything if deployed.

However, you will notice in app/dashboards/member_dashboard.rb that I commented out the user fields. The reason is because Administrate will raise an error on create, update and show because it needs a user dashboard and controller. The dashboard is there but the controller is missing (the route too).

@coaxial, can you create that part so members can be fully managed and associated to users and to manage the user record too?

houndci-bot · 2016-03-10T03:52:54Z

app/dashboards/member_dashboard.rb

+  ATTRIBUTE_TYPES = {
+    events: Field::HasMany,
+    organizations: Field::HasMany,
+    #user: Field::BelongsTo,


Missing space after #.

houndci-bot · 2016-03-17T21:30:55Z

app/models/user.rb

@@ -53,4 +55,5 @@ def self.default_user
  def active_for_authentication?
    super && email != DEFAULT_USER_EMAIL
  end
+


Extra empty line detected at class body end.

Manage and display members (#8)

coaxial added 3 commits November 24, 2015 21:04

Create members model, add members to admin dashboard

b339e19

Validate presence of name, email and uniqueness of email

0c5d6d5

Add tests for member

7213be9

sophiedeziel reviewed Nov 25, 2015
View reviewed changes

spec/factories/members.rb

biography "MyText"

end

end

Copy link

Member

sophiedeziel Nov 25, 2015

Choose a reason for hiding this comment

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

Ca we correct indentation here please?

Merge branch 'master' of github.com:montrealrb/Montreal.rb

bc4aa11

nicholasjhenry added chore requires-rebase and removed chore labels Nov 27, 2015

nicholasjhenry self-assigned this Dec 1, 2015

nicholasjhenry added requires-code-review and removed requires-rebase labels Dec 1, 2015

benichu unassigned nicholasjhenry Dec 1, 2015

GalkaS reviewed Dec 3, 2015
View reviewed changes

nicholasjhenry added the requires-amendment label Dec 4, 2015

houndci-bot reviewed Mar 10, 2016
View reviewed changes

coaxial and others added 14 commits March 9, 2016 21:11

Merge branch 'master' of github.com:montrealrb/Montreal.rb

6c82fd2

Create members model, add members to admin dashboard

f575d2f

Fix indent

c62ab25

Refactor tests

9be67b4

Enforce non-null email at db level

1cce2a2

Add foreign key to users

da54a56

Move foreign key from Users to Members

df2a000

Make houndci happy

8c230e8

Split migration in two

e5e238c

Make hound happy again

df1e158

new schema

0cdfefd

fix the tests

7cfabff

fix the creation of a new member in the admin panel

998e42b

closes #8

320bddf

sophiedeziel removed the requires-rebase label Mar 10, 2016

last detail

0c66608

houndci-bot reviewed Mar 10, 2016
View reviewed changes

really hound?

b89b768

sophiedeziel mentioned this pull request Mar 11, 2016

Assign members to talks #61

Closed

fix the user dependency

d1921fe

houndci-bot reviewed Mar 17, 2016
View reviewed changes

sophiedeziel added 3 commits March 17, 2016 17:31

Merge branch 'master' into feature/members

112e032

make hound happy

4414ea5

add back events and organizations

3d529b7

sophiedeziel added a commit that referenced this pull request Mar 17, 2016

Merge pull request #74 from Coaxial/feature/members

20e65e9

Manage and display members (#8)

sophiedeziel merged commit 20e65e9 into montrealrb:master Mar 17, 2016

sophiedeziel deleted the feature/members branch March 17, 2016 21:44

sophiedeziel removed the Priority label Mar 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manage and display members (#8) #74

Manage and display members (#8) #74

coaxial commented Nov 25, 2015

sophiedeziel Nov 25, 2015

coaxial commented Nov 25, 2015

nicholasjhenry commented Nov 27, 2015

coaxial commented Nov 27, 2015

nicholasjhenry commented Nov 27, 2015

benichu commented Dec 1, 2015

nicholasjhenry commented Dec 1, 2015

nicholasjhenry commented Dec 1, 2015

cawel commented Dec 2, 2015

nicholasjhenry commented Dec 2, 2015

cawel commented Dec 2, 2015

coaxial commented Dec 2, 2015

cawel commented Dec 2, 2015

GalkaS Dec 3, 2015

nicholasjhenry Dec 3, 2015

nicholasjhenry commented Dec 3, 2015

cawel commented Dec 3, 2015

nicholasjhenry commented Dec 4, 2015

nicholasjhenry commented Dec 4, 2015

houndci-bot Mar 10, 2016

sophiedeziel commented Mar 10, 2016

houndci-bot Mar 10, 2016

houndci-bot Mar 17, 2016

Manage and display members (#8) #74

Manage and display members (#8) #74

Conversation

coaxial commented Nov 25, 2015

sophiedeziel Nov 25, 2015

Choose a reason for hiding this comment

coaxial commented Nov 25, 2015

nicholasjhenry commented Nov 27, 2015

coaxial commented Nov 27, 2015

nicholasjhenry commented Nov 27, 2015

benichu commented Dec 1, 2015

nicholasjhenry commented Dec 1, 2015

nicholasjhenry commented Dec 1, 2015

cawel commented Dec 2, 2015

More important:

Less important:

nicholasjhenry commented Dec 2, 2015

cawel commented Dec 2, 2015

coaxial commented Dec 2, 2015

cawel commented Dec 2, 2015

GalkaS Dec 3, 2015

Choose a reason for hiding this comment

nicholasjhenry Dec 3, 2015

Choose a reason for hiding this comment

nicholasjhenry commented Dec 3, 2015

cawel commented Dec 3, 2015

nicholasjhenry commented Dec 4, 2015

nicholasjhenry commented Dec 4, 2015

houndci-bot Mar 10, 2016

Choose a reason for hiding this comment

sophiedeziel commented Mar 10, 2016

houndci-bot Mar 10, 2016

Choose a reason for hiding this comment

houndci-bot Mar 17, 2016

Choose a reason for hiding this comment