User page: new following section #1750

Senen · 2017-07-13T18:46:54Z

What

Show user followable activity on public user page.

Refactor

Remove popup on follow and unfollow buttons
Add AJAX notice on follows controller actions to keep user better informed

How

Add new tab to show followed objects by user.

Screenshots

Refactor screenshots

Test

Increased test coverage to:

Check automatic following tab activation when filter "follows" defined
Check "Following" tabs UI behaviour
Check following accordion behavior
Check followed object presence in correct accordion tab
Check action buttons presence, they should be shown when current user has enough rights
Check following and unfollowing notices.

Deployment

As usual.

…ps of 10 interests. Inserted interests in columns.

… author.

Senen · 2017-07-13T18:48:49Z

@voodoorai2000, @bertocq, @decabeza ahí va otro pull-request desde la playa! 😄 Esperamos vuestros comentarios.

Falta la verificación de la funcionalidad por parte de Yago Bermejo.

bertocq

Nice one 😃 👍

Some comments on the side just to improve code readability and a question that might be easy to explain

bertocq · 2017-07-17T23:29:23Z

app/controllers/users_controller.rb


 load_and_authorize_resource
 helper_method :author?
- helper_method :author_or_admin?


I can't really understand the reason to remove author_or_admin? method and its usage, can you explain a bit further? :)

@bertocq The main reason is that apparently it was not being used anywhere in the application.

👍 you're right, I did a project search and no use, although sometimes looking for the method string is not enough if you do compose de method name with a string interpolation like send("#{this}_or_#{that}") but that's not the case I guess based on the specs passing ^_^

Nice catch & cleanup 👏 😄

bertocq · 2017-07-17T23:39:33Z

app/helpers/follows_helper.rb

+ end
+
+ def entity_icon(entity)
+ case entity


Have you considered the option to use a hash here instead of a case statement? Something like:

def entity_icon(entity) { 'Proposal' => 'proposals', 'Budget::Investment' => 'budget' }[entity] end

It could be improved by moving the hash to a constant variable, or even switching keys for values to use new hash syntax:

def entity_icon(entity) { proposals: 'Proposal', budget: 'Budget::Investment' }.invert[entity] end

Last but not least, it seems odd that "proposals" is plural but "budget" is singular 🤔

I like very much this option! Less words and same clarity always better!

About the plurals question... don't worry @Senen I just realized it was wrong on consul beforehand :) I'll fix it afterwards this PR is merged! 👍

bertocq · 2017-07-17T23:42:33Z

app/models/abilities/common.rb

@@ -50,7 +50,7 @@ def initialize(user)
 can :create, Budget::Investment, budget: { phase: "accepting" }
 can :suggest, Budget::Investment, budget: { phase: "accepting" }
 can :destroy, Budget::Investment, budget: { phase: ["accepting", "reviewing"] }, author_id: user.id
- can :vote, Budget::Investment, budget: { phase: "selecting" }
+ can :vote, Budget::Investment,  budget: { phase: "selecting" }


^_^ /bertocq likes this

bertocq · 2017-07-17T23:44:24Z

app/views/users/_proposal.html.erb

@@ -0,0 +1,28 @@
+<tr id="proposal_<%= proposal.id %>">
+ <td>
+ <%= link_to proposal.title, proposal, proposal.retired? ? { class: 'retired' } : {} %>


I like that ? { class: 'retired' } : {} :) I think I have added some class="" around :( couldn't figure out this was the way 👏

bertocq · 2017-07-17T23:47:59Z

app/views/users/_proposal.html.erb

+ </td>
+ <% end %>
+
+ <% if author?(proposal) || proposal.retired? %>


It took me a bit to understand all the conditionals around this block and realize there is only 2 scenarios. Maybe this could be easier to read:

<% if proposal.retired? %> <td class="text-center"> <span class="label alert"><%= t('users.proposals.retired') %></span> </td> <% elsif author?(proposal) %> <td class="text-center"> <%= link_to t('users.proposals.retire'), retire_form_proposal_path(proposal), class: 'button hollow alert' %> </td> <% end %>

Totally agree @bertocq! We will make this change too!

bertocq · 2017-07-17T23:48:43Z

app/views/users/show.html.erb

@@ -46,7 +46,7 @@
 <p><%= t('users.show.private_activity') %></p>
 <% end %>

- <% if @user.public_interests || @authorized_current_user %>
+ <% if valid_interests_access? %>


Nice catch!

bertocq · 2017-07-17T23:51:59Z

app/helpers/follows_helper.rb

@@ -52,6 +52,18 @@ def unfollow_drop_id(followable)
 "unfollow-drop-#{entity_full_name(followable)}-#{followable.id}"
 end

+ def entity_title(title)
+ entity = title.gsub('::', '/').downcase


I think you'll love this page http:https://inflector.me/Budget::Investment :)

This two lines could be just one t("activerecord.models.#{title.underscore}.other")

Thanks for this resource. 😄 👏. I'll do this change.

bertocq · 2017-07-17T23:58:34Z

app/views/users/_following.html.erb

+ <table>
+ <tbody>
+ <% @follows.where(followable_type: followable_type).each do |follow| %>
+ <% if followable_type == "Proposal" %>


After seeing both entity_title and entity_icon helper methods. I think a third one could be nice to avoid spilling == "Proposal" and == "Budget::Investment" on a view :D, seems tricky to remember if another entity becomes as well "followable"

So the entire if-else could be replace for something like:

<%= render entity_partial(followable_type), entity_partial(followable_type).to_sym => follow.followable %>

Being the helper function at app/helpers/follows_helper.rb:

def entity_partial(title) title.parameterize.gsub('-','_') end

What do you think?

Yes @bertocq while we were programming exactly your solution we thought that this abstraction forces the developer to follow entity_partial helper method to understand where to put and how to name new followable object partials. Because of that we decide to let these lines explicitly written, but if you prefer, we can make this change in a second. I think both solutions has some disadvantage and in this case we prefer explicitness over reducing code lines.

In the scenario were another entity becomes followable, for both icon and title translation following the trail to the helper will be needed, so I don't see it as a big downside to the proposed refactor. I just imagine 4-5 entities being followable and the if-elsif-elsif.. being too big when just following a simple convention (that also complains with how rails expects things to be named) could make it easier.

But thats like you say its a subjective matter, I prefer conventions over more lines, maybe @voodoorai2000 can take a look and say if he has a hard opinion about it or not.

…ents automatically.

bertocq

Seems really great to me 👍 😃 & thanks for taking in consideration the suggestions!

bertocq · 2017-07-18T17:42:40Z

app/helpers/follows_helper.rb

@@ -9,23 +9,22 @@ def show_unfollow_action?(followable)
 end

 def follow_entity_text(followable)
- entity = followable.class.name.gsub('::', '/').downcase


👏 nice cleaning up on the side! 💪

Senen · 2017-07-19T07:29:27Z

@bertocq thanks for all your advices! They are allways welcome! 😃.We are still waiting customer validation. We'll let you know when it's done.

voodoorai2000 · 2017-07-19T13:12:33Z

app/views/users/_following.html.erb

@@ -0,0 +1,31 @@
+<ul class="accordion" data-accordion data-allow-all-closed="true">
+
+ <% @followable_types.each do |followable_type| %>


Hey,
I just sent you a small PR regarding this, hope you like it :)

voodoorai2000 · 2017-07-19T13:13:22Z

Regarding the concept of entity... it's something new that we haven't used yet in Consul... maybe we can postpone the decision of using entity by creating a FollowablesHelper and chaging the following helper methods:

FollowsHelper:
def follow_entity_text
def unfollow_entity_text
def entity_full_name
def entity_title
def entity_icon
def entity_partial

for something like this:

FollowsHelper:
def follow_text
def unfollow_text

FollowablesHelper:
def followable_full_name
def followable_title
def followable_icon
def followable_partial

Minor refactoring for follows

Senen · 2017-07-19T13:54:50Z

app/views/users/_following.html.erb

+ <table>
+ <tbody>
+ <% follows.each do |follow| %>
+ <%= render_follow(follow) %>


@voodoorai2000 nice refactor! 😍

…ows helper. Rename entity to followable.

…ble_button partial to follow_button.

…der notice on AJAX JS response.

Senen · 2017-07-19T18:03:59Z

@voodoorai2000, @bertocq, @decabeza i think we finally have final version. I made all suggested changes by @voodoorai2000 and removed unused translation key, also we removed popup on follow buttons and add a notice message to keep user informed. Off course we increased tests coverage with new notice checks and delete a lot of ugly code needed by popup but useless now.

As always, thanks for your advices. 😃

I add a link to our staging server so you can try look and feel easily.
https://rockandror-consul.herokuapp.com/budgets/1/investments/29
user: [email protected] pass: qwer1234

…owed_by? on followable model concern. Some code improvements.

bertocq

👏 👍

voodoorai2000

Awesome

Senen · 2017-07-24T10:54:51Z

@bertocq, @voodoorai2000, @decabeza this feature was approved by Yago. We can merge it! 😃

…-processes Draft phase on legislation processes

agileontheweb and others added 5 commits July 13, 2017 20:14

Add tab. Add accordion. Add style. Pass test scss-lint.

52e195d

Add following tab to user public page.

7e2e94d

Remove margin bottom in the table inside accordion. Divided into grou…

556e259

…ps of 10 interests. Inserted interests in columns.

Do not display send notification button when proposal is from another…

b0c571e

… author.

Increased test coverage to Following user public page.

cef67c5

bertocq reviewed Jul 18, 2017

View reviewed changes

Senen added 4 commits July 18, 2017 19:12

Use inflector methods instead of manual replacements.

36f9836

Simpler code to get followable object icon.

249641f

Easier conditional usage.

16987e7

Add method to follows helper to get followable partial name and argum…

57c8887

…ents automatically.

Senen force-pushed the followable-user-activity branch from 4c4dd54 to 57c8887 Compare July 18, 2017 17:36

bertocq approved these changes Jul 18, 2017

View reviewed changes

minor refactoring for follows

b7ef755

voodoorai2000 mentioned this pull request Jul 19, 2017

Minor refactoring for follows rockandror/consuldemocracy#12

Merged

voodoorai2000 reviewed Jul 19, 2017

View reviewed changes

Merge pull request #12 from consul/follows-refactoring

3be893b

Minor refactoring for follows

Senen commented Jul 19, 2017

View reviewed changes

Senen added 5 commits July 19, 2017 17:50

Create new followables helper to have some methods previously on foll…

cba497b

…ows helper. Rename entity to followable.

Remove popup on follow buttons and remove uneeded code.

c9a6630

Remove where clause from followable button partial and rename followa…

119a6b3

…ble_button partial to follow_button.

Remove uneeded initialization after ajax refresh.

3b13350

Add followable specific notices to follows controller actions and ren…

dec7d2d

…der notice on AJAX JS response.

Senen mentioned this pull request Jul 19, 2017

A6B - Followable improvements rockandror/consuldemocracy#11

Closed

2 tasks

Remove complex methods from follow model. Create instance method foll…

b7bce2e

…owed_by? on followable model concern. Some code improvements.

bertocq approved these changes Jul 20, 2017

View reviewed changes

voodoorai2000 approved these changes Jul 21, 2017

View reviewed changes

bertocq merged commit 13871b8 into consuldemocracy:master Jul 24, 2017

taitus mentioned this pull request Jul 25, 2017

B. My activity rockandror/consuldemocracy#4

Closed

bertocq mentioned this pull request Jul 25, 2017

Chore/consul pull AyuntamientoMadrid/consul#861

Merged

voodoorai2000 deleted the followable-user-activity branch August 2, 2017 10:08

voodoorai2000 moved this from Doing to Done in Roadmap Aug 2, 2017

voodoorai2000 moved this from Done to Next Release in Roadmap Aug 5, 2017

voodoorai2000 moved this from Next Release to Archive in Roadmap Oct 2, 2017

voodoorai2000 added the release label Oct 6, 2017

voodoorai2000 removed this from Archive in Roadmap Oct 6, 2017

javierm pushed a commit to javierm/consul that referenced this pull request Dec 18, 2018

Merge pull request consuldemocracy#1750 from AyuntamientoMadrid/draft…

e7d8600

…-processes Draft phase on legislation processes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User page: new following section #1750

User page: new following section #1750

Senen commented Jul 13, 2017 •

edited

Senen commented Jul 13, 2017

bertocq left a comment

bertocq Jul 17, 2017

taitus Jul 18, 2017

bertocq Jul 18, 2017

bertocq Jul 17, 2017 •

edited

Senen Jul 18, 2017

bertocq Jul 18, 2017

bertocq Jul 17, 2017

bertocq Jul 17, 2017

bertocq Jul 17, 2017 •

edited

Senen Jul 18, 2017

bertocq Jul 17, 2017

bertocq Jul 17, 2017

Senen Jul 18, 2017 •

edited

bertocq Jul 17, 2017 •

edited

Senen Jul 18, 2017 •

edited

bertocq Jul 18, 2017 •

edited

bertocq left a comment

bertocq Jul 18, 2017

Senen commented Jul 19, 2017

voodoorai2000 Jul 19, 2017 •

edited

voodoorai2000 commented Jul 19, 2017

Senen Jul 19, 2017

Senen commented Jul 19, 2017 •

edited

bertocq left a comment

voodoorai2000 left a comment

Senen commented Jul 24, 2017 •

edited

		@@ -0,0 +1,31 @@
		<ul class="accordion" data-accordion data-allow-all-closed="true">

		<% @followable_types.each do \|followable_type\| %>

User page: new following section #1750

User page: new following section #1750

Conversation

Senen commented Jul 13, 2017 • edited

What

How

Screenshots

Test

Deployment

Senen commented Jul 13, 2017

bertocq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertocq Jul 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertocq Jul 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Senen Jul 18, 2017 • edited

Choose a reason for hiding this comment

bertocq Jul 17, 2017 • edited

Choose a reason for hiding this comment

Senen Jul 18, 2017 • edited

Choose a reason for hiding this comment

bertocq Jul 18, 2017 • edited

Choose a reason for hiding this comment

bertocq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Senen commented Jul 19, 2017

voodoorai2000 Jul 19, 2017 • edited

Choose a reason for hiding this comment

voodoorai2000 commented Jul 19, 2017

Choose a reason for hiding this comment

Senen commented Jul 19, 2017 • edited

bertocq left a comment

Choose a reason for hiding this comment

voodoorai2000 left a comment

Choose a reason for hiding this comment

Senen commented Jul 24, 2017 • edited

Senen commented Jul 13, 2017 •

edited

bertocq Jul 17, 2017 •

edited

bertocq Jul 17, 2017 •

edited

Senen Jul 18, 2017 •

edited

bertocq Jul 17, 2017 •

edited

Senen Jul 18, 2017 •

edited

bertocq Jul 18, 2017 •

edited

voodoorai2000 Jul 19, 2017 •

edited

Senen commented Jul 19, 2017 •

edited

Senen commented Jul 24, 2017 •

edited