Use membership decorator in view #351

rewritten · 2018-05-08T22:30:15Z

Solves #349

What

I add here a decorator for membership. It has access to the view context, and exposes exactly what's needed to render the view.

Why

User list view (table rows mostly) uses several models in parallel and it's difficult to follow, with all the conditionals. A decorator encapsulates all the logic in a testable class.

Bonus

I have added dry-initializer gem, which is a very lightweight library to help with different kinds of instance initialization strategies. It is not really needed here, but it's a nice addition in general, as I would also like to introduce to more dry-* gems in the future.

Write the decorator
Specify all the decorator behavior
Modify the view so it relies on the decorated member instead of user + membership
Pass only the decorated members from UsersController#index

rewritten · 2018-05-09T01:25:03Z

Lots of spec-related noise, I will squash all commits after CR is done, before merging.

markets

Very nice refactor @rewritten 👏 Decorators (and view models) are really useful for clean up logic inside views.

I also like a lot the dry-* family, but not sure if we should introduce new deps. for things we can easily implement with Ruby stdlib, even if they are nice additions. In this case, it's a really lightweight gem, so not really a big deal, but in general I'd prefer "plain" Ruby vs "syntax sugar" plugins.

I left another small comment above. But really nice PR 👌. Happy to see you here again! 😃

markets · 2018-05-12T16:19:49Z

app/decorators/member_decorator.rb

+  end
+
+  def inactive_icon
+    v.tag(:span, class: %w[glyphicon glyphicon-time]) unless active?


@rewritten I realized this produces a different html compared to the original code: an anchor inside the span (when called along with link_to_self). I think this is because this line returns a self-closed span. It looks like this:

We can fix that by using the glyph helper:

v.glyph('time') unless active?

I just tried it locally and it looks fine:

sauloperez · 2018-05-12T17:21:21Z

Welcome back home @rewritten !!

I also like a lot the dry-* family, but not sure if we should introduce new deps. for things we can easily implement with Ruby stdlib, even if they are nice additions. In this case, it's a really lightweight gem, so not really a big deal, but in general I'd prefer "plain" Ruby vs "syntax sugar" plugins.

I couldn't agree more. I particular decision we took about the development of the project (and we should state in the documentation) is that we want to keep it as simple and lightweight as possible, in terms of both infrastructure and code, to keep its maintenance cost at the minimum. We don't have enough resources to spend dev time on it and so we aim to be 80% contributor-driven.

This directly affects how serious we take the addition of new dependencies such as gems, which need to be maintained in the long run. So I suggest we remove that dry-initializer gem (I know it's hard. I do also like dry-* gems).

Besides this, I ❤️ the direction this PR is taking.

sauloperez · 2018-05-12T17:24:47Z

app/controllers/users_controller.rb

+
+    @members = current_organization.
+      members.
+      where(user_id: user_ids).


can we rather do an AR merge? This avoids one SQL query which saves a network round trip, etc. PostgreSQL knows how to match against those users better than us.

PostgreSQL knows, ActiveRecord sometimes messes up the joins. I will try to see if this is working in this case.

Indeed it is not working in this case. As @users already comes from a JOIN, AR is not really able to distinguish how to prepare the association chain.

Here it is a direct ransack on members, so we don't have to mix stuff up.

sauloperez · 2018-05-12T17:25:48Z

app/decorators/member_decorator.rb

+
+  private
+
+  def h


what about a more meaningful name like routes? It would help other devs understand all this.

sauloperez · 2018-05-12T17:27:46Z

spec/controllers/users_controller_spec.rb

-          wrong_email_member.user_id => wrong_email_member,
-          empty_email_member.user_id => empty_email_member
-        })
+        assigns(:members).each { |m| expect(m).to be_a(MemberDecorator) }


IMO it's a not a good idea to couple to particular class types. Makes future changes harder and it's sort of against Ruby's duck-type principle.

Ok but still somewhat I want to specify that what's sent to the view is a list of decorated objects. What about a general method decorated_object on the decorator? Instead of member. I have used member just for laziness, for instance when using draper I tend to use object instead of the named accessor.

sauloperez · 2018-05-12T17:28:31Z

app/controllers/users_controller.rb

+    #   merge(@search.result(distinct: false)).
+    #   page(params[:page]).
+    #   per(25).
+    #   map { |m| MemberDecorator.new(m, self.class.helpers) }


Actually I left this here to remind about making the member table ransack against members itself. I will try to complete the feature, and make the whole page based on members so we don't need the additional @users variable here.

rewritten · 2018-05-14T00:44:39Z

Dry removed 😢
Base ViewModel class added, it should be more contributor-friendly
Refactored users index action to be actually based on members (the only remnant is that it's UsersController, because it's also accessed with the members path...)
Slightly tweaked index view so ransack works based on Member relation

Ready for your second review :)

rewritten · 2018-05-14T00:47:58Z

app/controllers/users_controller.rb

+    default_sort
+
+    @search =
+      current_organization.members.joins(:account, :user).ransack(params[:q])


There doesn't seem to be any difference between doing joins before ransack.result or after it, and I don't have a clear reason for it to be in any of them instead of the other. The only "intuition" I have is that by joining after the search, we may risk joining twice. Any input?

Maybe I should just ditch the joins and use only eager_load.

rewritten · 2018-05-14T00:49:18Z

app/controllers/users_controller.rb

+    @members =
+      @search.result.page(params[:page]).per(25)
+
+    @member_view_models =


There is no easy way to keep the pagination info exposed and at the same time to decorate the objects. It's this or another "paginated collection decorator", which most of the times I find ugly.

sauloperez

👏 congrats @rewritten ! This improves a lot the view and sets the foundation for further improvements 🎉

rewritten · 2018-05-14T09:02:38Z

@markets, @sauloperez what's the process now? Who merges? Who deploys?

sauloperez · 2018-05-14T09:55:49Z

we need @sseerrggii and/or his testing crew to give it a try. As soon as it gets their approve we can go ahead and merge.

sseerrggii · 2018-05-16T11:08:11Z

I have tested and everything works fine for me 👍

enricostano

Nice, thanks @rewritten . Welcome back! 💪

rewritten added 5 commits May 9, 2018 00:24

Use membership decorator in view [WIP]

5752cd8

WIP

f6e5128

WIP

5c2c25b

Added missing tests; extract partials from row

67f8423

Fix last test

8b7595c

rewritten requested a review from markets May 9, 2018 01:21

Fix tests after running whole suite

acc0a61

Sorting by balance spec wasn't stable, check just th relevant position

cd25968

saverio-kantox mentioned this pull request May 9, 2018

Don't show emails @example.com #349

Closed

rewritten requested review from enricostano and sauloperez May 10, 2018 22:09

markets requested changes May 12, 2018

View reviewed changes

sauloperez requested changes May 12, 2018

View reviewed changes

markets mentioned this pull request May 12, 2018

Allow to search users by member_uid #354

Merged

rewritten added 2 commits May 14, 2018 01:49

CR suggestions

317db4e

Make the member list view do ransack on members table

edd4797

rewritten commented May 14, 2018

View reviewed changes

rewritten added 2 commits May 14, 2018 02:54

Ransack form can be configured. RTFM.

0dabcd1

Nicer encapsulation of search params

d42625c

sauloperez approved these changes May 14, 2018

View reviewed changes

markets approved these changes May 14, 2018

View reviewed changes

Set back the loglevel to warn in test env

57fe79c

enricostano approved these changes May 17, 2018

View reviewed changes

sauloperez merged commit e45a9bb into develop May 17, 2018

sauloperez deleted the feature/membership-decorator branch May 17, 2018 08:06

markets mentioned this pull request May 26, 2018

Refactor memberships query in users controller #312

Closed

enricostano mentioned this pull request Aug 8, 2018

Release v1.6.0 #394

Merged

Use membership decorator in view #351

Use membership decorator in view #351

Uh oh!

Conversation

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

Uh oh!

What

Why

Bonus

Uh oh!

rewritten commented May 9, 2018

Uh oh!

markets left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

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!

rewritten commented May 14, 2018

Uh oh!

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!

rewritten commented May 14, 2018

Uh oh!

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

Uh oh!

Uh oh!

sseerrggii commented May 16, 2018

Uh oh!

enricostano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rewritten commented May 8, 2018 •

edited

Loading

markets May 12, 2018 •

edited

Loading

sauloperez commented May 12, 2018 •

edited

Loading

sauloperez commented May 14, 2018 •

edited

Loading