From 274abaf290b0708f8c593637a45a16b24146dfda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 3 Sep 2021 20:16:18 +0200 Subject: [PATCH 1/9] Remove unused method in users controller It isn't used since commit fc57bad1c. --- app/controllers/users_controller.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a50d55cf6..fc1b7c22f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,7 +2,6 @@ class UsersController < ApplicationController has_filters %w[proposals debates budget_investments comments follows], only: :show load_and_authorize_resource - helper_method :author? helper_method :valid_interests_access? def show @@ -79,10 +78,6 @@ class UsersController < ApplicationController @user.public_interests || authorized_current_user? end - def author?(proposal) - proposal.author_id == current_user.id if current_user - end - def authorized_current_user? @authorized_current_user ||= current_user && (current_user == @user || current_user.moderator? || current_user.administrator?) end From 67a13d5fea43d539ef6a1f200401c82319b4478e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 3 Sep 2021 20:22:20 +0200 Subject: [PATCH 2/9] Use local variables in user profile partials This way we'll be able to render the partials from places like a component. --- app/views/users/_activity_page.html.erb | 10 +++++----- app/views/users/_budget_investments.html.erb | 4 ++-- app/views/users/_comments.html.erb | 4 ++-- app/views/users/_debates.html.erb | 4 ++-- app/views/users/_following.html.erb | 6 +++--- app/views/users/_proposals.html.erb | 4 ++-- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/views/users/_activity_page.html.erb b/app/views/users/_activity_page.html.erb index eaa8a73b7..410a71aa1 100644 --- a/app/views/users/_activity_page.html.erb +++ b/app/views/users/_activity_page.html.erb @@ -1,5 +1,5 @@ -<%= render "following" if @follows.present? %> -<%= render "proposals" if @proposals.present? && feature?(:proposals) %> -<%= render "debates" if @debates.present? && feature?(:debates) %> -<%= render "budget_investments" if @budget_investments.present? && feature?(:budgets) %> -<%= render "comments" if @comments.present? %> +<%= render "following", user: @user, follows: @follows if @follows.present? %> +<%= render "proposals", proposals: @proposals if @proposals.present? && feature?(:proposals) %> +<%= render "debates", debates: @debates if @debates.present? && feature?(:debates) %> +<%= render "budget_investments", budge_investments: @budge_investments if @budge_investments.present? && feature?(:budgets) %> +<%= render "comments", comments: @comments if @comments.present? %> diff --git a/app/views/users/_budget_investments.html.erb b/app/views/users/_budget_investments.html.erb index 9c6775265..a2b523133 100644 --- a/app/views/users/_budget_investments.html.erb +++ b/app/views/users/_budget_investments.html.erb @@ -6,10 +6,10 @@ - <% @budget_investments.each do |budget_investment| %> + <% budget_investments.each do |budget_investment| %> <%= render "budget_investment", budget_investment: budget_investment %> <% end %> -<%= paginate @budget_investments %> +<%= paginate budget_investments %> diff --git a/app/views/users/_comments.html.erb b/app/views/users/_comments.html.erb index 7b7cf5165..be264ce05 100644 --- a/app/views/users/_comments.html.erb +++ b/app/views/users/_comments.html.erb @@ -5,7 +5,7 @@ - <% @comments.each do |comment| %> + <% comments.each do |comment| %> <%= comment_commentable_title(comment) %> @@ -17,4 +17,4 @@ -<%= paginate @comments %> +<%= paginate comments %> diff --git a/app/views/users/_debates.html.erb b/app/views/users/_debates.html.erb index 5b293b1a4..be8dbe7bc 100644 --- a/app/views/users/_debates.html.erb +++ b/app/views/users/_debates.html.erb @@ -5,7 +5,7 @@ - <% @debates.each do |debate| %> + <% debates.each do |debate| %> <%= link_to debate.title, debate %> @@ -15,4 +15,4 @@ -<%= paginate @debates %> +<%= paginate debates %> diff --git a/app/views/users/_following.html.erb b/app/views/users/_following.html.erb index 91fa8562a..480bb2da2 100644 --- a/app/views/users/_following.html.erb +++ b/app/views/users/_following.html.erb @@ -1,12 +1,12 @@
    - <% @follows.each do |followable_type, follows| %> + <% follows.each do |followable_type, follows| %>
  • <%= link_to followable_type_title(followable_type), "##{followable_type_title(followable_type).parameterize.underscore}" %>
    • <% end %>
- <% @follows.each do |followable_type, follows| %> + <% follows.each do |followable_type, follows| %>

@@ -22,6 +22,6 @@

- <%= render "interests", user: @user if valid_interests_access? %> + <%= render "interests", user: user if valid_interests_access? %>
diff --git a/app/views/users/_proposals.html.erb b/app/views/users/_proposals.html.erb index 6604ac160..76758bfd3 100644 --- a/app/views/users/_proposals.html.erb +++ b/app/views/users/_proposals.html.erb @@ -7,10 +7,10 @@ - <% @proposals.each do |proposal| %> + <% proposals.each do |proposal| %> <%= render "proposal", proposal: proposal %> <% end %> -<%= paginate @proposals %> +<%= paginate proposals %> From 62b23f1e562509510c91129c4a29199ba5852c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 3 Sep 2021 21:11:21 +0200 Subject: [PATCH 3/9] Extract component to show a user's public activity --- .../users/public_activity_component.html.erb | 35 ++++++++ .../users/public_activity_component.rb | 89 +++++++++++++++++++ app/controllers/users_controller.rb | 81 +---------------- app/views/users/_activity_page.html.erb | 5 -- app/views/users/show.html.erb | 31 +------ 5 files changed, 126 insertions(+), 115 deletions(-) create mode 100644 app/components/users/public_activity_component.html.erb create mode 100644 app/components/users/public_activity_component.rb delete mode 100644 app/views/users/_activity_page.html.erb diff --git a/app/components/users/public_activity_component.html.erb b/app/components/users/public_activity_component.html.erb new file mode 100644 index 000000000..bd91e376f --- /dev/null +++ b/app/components/users/public_activity_component.html.erb @@ -0,0 +1,35 @@ +<% load_filtered_activity if valid_access? %> +<% if valid_access? %> +
    + <% valid_filters.each do |filter| %> + <% if @activity_counts[filter] > 0 %> + <% if current_filter == filter %> +
  • +

    <%= t("users.show.filters.#{filter}", count: @activity_counts[filter]) %>

    +
    • + <% else %> +
  • + <%= link_to t("users.show.filters.#{filter}", count: @activity_counts[filter]), + current_path_with_query_params(filter: filter, page: 1) %> +
    • + <% end %> + <% end %> + <% end %> +
+ + <% if @activity_counts.values.inject(&:+) == 0 %> +
+ <%= t("users.show.no_activity") %> +
+ <% end %> + + <%= render "users/following", user: user, follows: @follows if @follows.present? %> + <%= render "users/proposals", proposals: @proposals if @proposals.present? && feature?(:proposals) %> + <%= render "users/debates", debates: @debates if @debates.present? && feature?(:debates) %> + <%= render "users/budget_investments", budget_investments: @budget_investments if @budget_investments.present? && feature?(:budgets) %> + <%= render "users/comments", comments: @comments if @comments.present? %> +<% else %> +
+ <%= t("users.show.private_activity") %> +
+<% end %> diff --git a/app/components/users/public_activity_component.rb b/app/components/users/public_activity_component.rb new file mode 100644 index 000000000..0d9c2e5bf --- /dev/null +++ b/app/components/users/public_activity_component.rb @@ -0,0 +1,89 @@ +class Users::PublicActivityComponent < ApplicationComponent + attr_reader :user + delegate :authorized_current_user?, :current_path_with_query_params, :valid_filters, :current_filter, to: :helpers + + def initialize(user) + @user = user + end + + def valid_access? + user.public_activity || authorized_current_user? + end + + private + + def set_activity_counts + @activity_counts = ActiveSupport::HashWithIndifferentAccess.new( + proposals: Proposal.where(author_id: @user.id).count, + debates: (Setting["process.debates"] ? Debate.where(author_id: @user.id).count : 0), + budget_investments: (Setting["process.budgets"] ? Budget::Investment.where(author_id: @user.id).count : 0), + comments: only_active_commentables.count, + follows: @user.follows.map(&:followable).compact.count) + end + + def load_filtered_activity + set_activity_counts + case params[:filter] + when "proposals" then load_proposals + when "debates" then load_debates + when "budget_investments" then load_budget_investments + when "comments" then load_comments + when "follows" then load_follows + else load_available_activity + end + end + + def load_available_activity + if @activity_counts[:proposals] > 0 + load_proposals + @current_filter = "proposals" + elsif @activity_counts[:debates] > 0 + load_debates + @current_filter = "debates" + elsif @activity_counts[:budget_investments] > 0 + load_budget_investments + @current_filter = "budget_investments" + elsif @activity_counts[:comments] > 0 + load_comments + @current_filter = "comments" + elsif @activity_counts[:follows] > 0 + load_follows + @current_filter = "follows" + end + end + + def load_proposals + @proposals = Proposal.created_by(@user).order(created_at: :desc).page(params[:page]) + end + + def load_debates + @debates = Debate.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) + end + + def load_comments + @comments = only_active_commentables.includes(:commentable).order(created_at: :desc).page(params[:page]) + end + + def load_budget_investments + @budget_investments = Budget::Investment.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) + end + + def load_follows + @follows = @user.follows.group_by(&:followable_type) + end + + def only_active_commentables + disabled_commentables = [] + disabled_commentables << "Debate" unless Setting["process.debates"] + disabled_commentables << "Budget::Investment" unless Setting["process.budgets"] + if disabled_commentables.present? + all_user_comments.where.not(commentable_type: disabled_commentables) + else + all_user_comments + end + end + + def all_user_comments + Comment.not_valuations.not_as_admin_or_moderator.where(user_id: @user.id) + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fc1b7c22f..4eceb8ecd 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,77 +3,13 @@ class UsersController < ApplicationController load_and_authorize_resource helper_method :valid_interests_access? + helper_method :authorized_current_user? def show - load_filtered_activity if valid_access? end private - def set_activity_counts - @activity_counts = ActiveSupport::HashWithIndifferentAccess.new( - proposals: Proposal.where(author_id: @user.id).count, - debates: (Setting["process.debates"] ? Debate.where(author_id: @user.id).count : 0), - budget_investments: (Setting["process.budgets"] ? Budget::Investment.where(author_id: @user.id).count : 0), - comments: only_active_commentables.count, - follows: @user.follows.map(&:followable).compact.count) - end - - def load_filtered_activity - set_activity_counts - case params[:filter] - when "proposals" then load_proposals - when "debates" then load_debates - when "budget_investments" then load_budget_investments - when "comments" then load_comments - when "follows" then load_follows - else load_available_activity - end - end - - def load_available_activity - if @activity_counts[:proposals] > 0 - load_proposals - @current_filter = "proposals" - elsif @activity_counts[:debates] > 0 - load_debates - @current_filter = "debates" - elsif @activity_counts[:budget_investments] > 0 - load_budget_investments - @current_filter = "budget_investments" - elsif @activity_counts[:comments] > 0 - load_comments - @current_filter = "comments" - elsif @activity_counts[:follows] > 0 - load_follows - @current_filter = "follows" - end - end - - def load_proposals - @proposals = Proposal.created_by(@user).order(created_at: :desc).page(params[:page]) - end - - def load_debates - @debates = Debate.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) - end - - def load_comments - @comments = only_active_commentables.includes(:commentable).order(created_at: :desc).page(params[:page]) - end - - def load_budget_investments - @budget_investments = Budget::Investment.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) - end - - def load_follows - @follows = @user.follows.group_by(&:followable_type) - end - - def valid_access? - @user.public_activity || authorized_current_user? - end - def valid_interests_access? @user.public_interests || authorized_current_user? end @@ -81,19 +17,4 @@ class UsersController < ApplicationController def authorized_current_user? @authorized_current_user ||= current_user && (current_user == @user || current_user.moderator? || current_user.administrator?) end - - def all_user_comments - Comment.not_valuations.not_as_admin_or_moderator.where(user_id: @user.id) - end - - def only_active_commentables - disabled_commentables = [] - disabled_commentables << "Debate" unless Setting["process.debates"] - disabled_commentables << "Budget::Investment" unless Setting["process.budgets"] - if disabled_commentables.present? - all_user_comments.where.not(commentable_type: disabled_commentables) - else - all_user_comments - end - end end diff --git a/app/views/users/_activity_page.html.erb b/app/views/users/_activity_page.html.erb deleted file mode 100644 index 410a71aa1..000000000 --- a/app/views/users/_activity_page.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= render "following", user: @user, follows: @follows if @follows.present? %> -<%= render "proposals", proposals: @proposals if @proposals.present? && feature?(:proposals) %> -<%= render "debates", debates: @debates if @debates.present? && feature?(:debates) %> -<%= render "budget_investments", budge_investments: @budge_investments if @budge_investments.present? && feature?(:budgets) %> -<%= render "comments", comments: @comments if @comments.present? %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 8186dbf3b..708b63275 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -22,36 +22,7 @@ <% end %> - <% if @user.public_activity || @authorized_current_user %> -
    - <% @valid_filters.each do |filter| %> - <% if @activity_counts[filter] > 0 %> - <% if @current_filter == filter %> -
  • -

    <%= t("users.show.filters.#{filter}", count: @activity_counts[filter]) %>

    -
    • - <% else %> -
  • - <%= link_to t("users.show.filters.#{filter}", count: @activity_counts[filter]), - current_path_with_query_params(filter: filter, page: 1) %> -
    • - <% end %> - <% end %> - <% end %> -
- - <% if @activity_counts.values.inject(&:+) == 0 %> -
- <%= t("users.show.no_activity") %> -
- <% end %> - - <%= render "activity_page" %> - <% else %> -
- <%= t("users.show.private_activity") %> -
- <% end %> + <%= render Users::PublicActivityComponent.new(@user) %> From 64258baf9772a4b2dc9b0cfa4fd31ef556f9221f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 3 Sep 2021 21:15:10 +0200 Subject: [PATCH 4/9] Refactor getting the public activity information We had similar conditions many times and some duplication, particularly between the code getting records and the code counting records. We can remove it by returning a generic scope and calling the `count` method to count the records and the `order` and `page` methods when we want to pass the records to the view. And, since we only display one partial per request, we can simplify the code to render all the partials. There's one minor disadvantage to this approach: searching the code for the place where these partials are rendered is now a bit harder since searching the code for something like "render (.+) budget_investments" won't return any results. We're also writing conditions about a certain filter just once, by setting `valid_filters`. This greatly simplifies the logic, although again there's one minor disadvantage: we have to implement the `current_filter` method, duplicating the logic already defined in the `HasFilters` concern. --- .../users/public_activity_component.html.erb | 27 +++--- .../users/public_activity_component.rb | 82 ++++++++----------- app/controllers/users_controller.rb | 2 - 3 files changed, 48 insertions(+), 63 deletions(-) diff --git a/app/components/users/public_activity_component.html.erb b/app/components/users/public_activity_component.html.erb index bd91e376f..03554e9a6 100644 --- a/app/components/users/public_activity_component.html.erb +++ b/app/components/users/public_activity_component.html.erb @@ -1,33 +1,30 @@ -<% load_filtered_activity if valid_access? %> <% if valid_access? %> -
    - <% valid_filters.each do |filter| %> - <% if @activity_counts[filter] > 0 %> + <% if valid_filters.any? %> +
      + <% valid_filters.each do |filter| %> <% if current_filter == filter %>
    • -

      <%= t("users.show.filters.#{filter}", count: @activity_counts[filter]) %>

      +

      <%= t("users.show.filters.#{filter}", count: count(filter)) %>

      • <% else %>
    • - <%= link_to t("users.show.filters.#{filter}", count: @activity_counts[filter]), + <%= link_to t("users.show.filters.#{filter}", count: count(filter)), current_path_with_query_params(filter: filter, page: 1) %>
      • <% end %> <% end %> - <% end %> -
    +
- <% if @activity_counts.values.inject(&:+) == 0 %> + <% if current_filter == "follows" %> + <%= render "users/following", user: user, follows: follows.group_by(&:followable_type) %> + <% else %> + <%= render_user_partial current_filter %> + <% end %> + <% else %>
<%= t("users.show.no_activity") %>
<% end %> - - <%= render "users/following", user: user, follows: @follows if @follows.present? %> - <%= render "users/proposals", proposals: @proposals if @proposals.present? && feature?(:proposals) %> - <%= render "users/debates", debates: @debates if @debates.present? && feature?(:debates) %> - <%= render "users/budget_investments", budget_investments: @budget_investments if @budget_investments.present? && feature?(:budgets) %> - <%= render "users/comments", comments: @comments if @comments.present? %> <% else %>
<%= t("users.show.private_activity") %> diff --git a/app/components/users/public_activity_component.rb b/app/components/users/public_activity_component.rb index 0d9c2e5bf..4c11f6069 100644 --- a/app/components/users/public_activity_component.rb +++ b/app/components/users/public_activity_component.rb @@ -1,6 +1,6 @@ class Users::PublicActivityComponent < ApplicationComponent attr_reader :user - delegate :authorized_current_user?, :current_path_with_query_params, :valid_filters, :current_filter, to: :helpers + delegate :authorized_current_user?, :current_path_with_query_params, to: :helpers def initialize(user) @user = user @@ -10,66 +10,56 @@ class Users::PublicActivityComponent < ApplicationComponent user.public_activity || authorized_current_user? end + def current_filter + if valid_filters.include?(params[:filter]) + params[:filter] + else + valid_filters.first + end + end + + def valid_filters + @valid_filters ||= [ + ("proposals" if feature?(:proposals)), + ("debates" if feature?(:debates)), + ("budget_investments" if feature?(:budgets)), + "comments", + "follows" + ].compact.select { |filter| send(filter).any? } + end + private - def set_activity_counts - @activity_counts = ActiveSupport::HashWithIndifferentAccess.new( - proposals: Proposal.where(author_id: @user.id).count, - debates: (Setting["process.debates"] ? Debate.where(author_id: @user.id).count : 0), - budget_investments: (Setting["process.budgets"] ? Budget::Investment.where(author_id: @user.id).count : 0), - comments: only_active_commentables.count, - follows: @user.follows.map(&:followable).compact.count) + def proposals + Proposal.where(author_id: user.id) end - def load_filtered_activity - set_activity_counts - case params[:filter] - when "proposals" then load_proposals - when "debates" then load_debates - when "budget_investments" then load_budget_investments - when "comments" then load_comments - when "follows" then load_follows - else load_available_activity - end + def debates + Debate.where(author_id: user.id) end - def load_available_activity - if @activity_counts[:proposals] > 0 - load_proposals - @current_filter = "proposals" - elsif @activity_counts[:debates] > 0 - load_debates - @current_filter = "debates" - elsif @activity_counts[:budget_investments] > 0 - load_budget_investments - @current_filter = "budget_investments" - elsif @activity_counts[:comments] > 0 - load_comments - @current_filter = "comments" - elsif @activity_counts[:follows] > 0 - load_follows - @current_filter = "follows" - end + def comments + only_active_commentables.includes(:commentable) end - def load_proposals - @proposals = Proposal.created_by(@user).order(created_at: :desc).page(params[:page]) + def budget_investments + Budget::Investment.where(author_id: user.id) end - def load_debates - @debates = Debate.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) + def follows + @follows ||= user.follows.select { |follow| follow.followable.present? } end - def load_comments - @comments = only_active_commentables.includes(:commentable).order(created_at: :desc).page(params[:page]) + def count(filter) + send(filter).count end - def load_budget_investments - @budget_investments = Budget::Investment.where(author_id: @user.id).order(created_at: :desc).page(params[:page]) + def render_user_partial(filter) + render "users/#{filter}", "#{filter}": send(filter).order(created_at: :desc).page(page) end - def load_follows - @follows = @user.follows.group_by(&:followable_type) + def page + params[:page] end def only_active_commentables @@ -84,6 +74,6 @@ class Users::PublicActivityComponent < ApplicationComponent end def all_user_comments - Comment.not_valuations.not_as_admin_or_moderator.where(user_id: @user.id) + Comment.not_valuations.not_as_admin_or_moderator.where(user_id: user.id) end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4eceb8ecd..0912f3295 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,6 +1,4 @@ class UsersController < ApplicationController - has_filters %w[proposals debates budget_investments comments follows], only: :show - load_and_authorize_resource helper_method :valid_interests_access? helper_method :authorized_current_user? From 9bc529dce17e789d45c3b7e9a38682d8b871e9bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Sep 2021 18:06:06 +0200 Subject: [PATCH 5/9] Group almost identical public interests tests --- spec/system/users_spec.rb | 36 +++++++----------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index 646645983..b0543f890 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -245,8 +245,10 @@ describe "Users" do logout - visit user_path(user) - expect(page).to have_content("Sport") + visit user_path(user, filter: "follows") + + expect(page).to have_css "#public_interests" + expect(page).to have_content "Sport" end scenario "Not display interests when proposal has been destroyed" do @@ -272,22 +274,6 @@ describe "Users" do expect(page).not_to have_css("#public_interests") end - scenario "User can display public page" do - create(:proposal, tag_list: "Sport", followers: [user]) - - login_as(user) - visit account_path - - check "account_public_interests" - click_button "Save changes" - - logout - - visit user_path(user, filter: "follows", page: "1") - - expect(page).to have_css("#public_interests") - end - scenario "Is always visible for the owner" do create(:proposal, tag_list: "Sport", followers: [user]) @@ -298,7 +284,9 @@ describe "Users" do click_button "Save changes" visit user_path(user, filter: "follows", page: "1") - expect(page).to have_css("#public_interests") + + expect(page).to have_css "#public_interests" + expect(page).to have_content "Tags of elements you follow" end scenario "Is always visible for admins" do @@ -341,16 +329,6 @@ describe "Users" do expect(page).to have_content("Tags of elements this user follows") end - - scenario "Should display custom interests title when user is visiting own user page" do - create(:proposal, tag_list: "Sport", followers: [user]) - - user.update!(public_interests: true) - login_as(user) - visit user_path(user, filter: "follows", page: "1") - - expect(page).to have_content("Tags of elements you follow") - end end describe "Special comments" do From 0875c214ba3a1380eb4e88bc291f1c5660930a0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Sep 2021 18:11:50 +0200 Subject: [PATCH 6/9] Make following tab tests easier to understand The `click_link` part did nothing other than scrolling to the element, since in these cases we've got a same-page link and the element it links to is already on the page. Programmers reading the test would expect the link to load the page or change to a different tab and would think the element it links to wasn't there before clicking the link (at least I did). --- spec/system/users_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index b0543f890..5a9607f9c 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -438,8 +438,8 @@ describe "Users" do login_as user visit user_path(user, filter: "follows") - click_link "Citizen proposals" + expect(page).to have_link "Citizen proposals", href: "#citizen_proposals" expect(page).to have_content proposal.title end @@ -493,8 +493,8 @@ describe "Users" do budget_investment = create(:budget_investment, author: user, followers: [user]) visit user_path(user, filter: "follows") - click_link "Investments" + expect(page).to have_link "Investments", href: "#investments" expect(page).to have_link budget_investment.title end end From 3cd4f3827e1a215062410e0778dce3571daa2ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Sep 2021 19:40:48 +0200 Subject: [PATCH 7/9] Hide what users are following unless they allow it It could be argued that seeing which proposals a user follows is a good indicator of which proposals a user has supported, since we're automatically creating follows for supported proposals since commit 74fbde09f. So now, we're extending the `public_interests` funcionality, so it only shows elements users are following if they've enabled it. This is an improvement over using the `public_activity` attribute in two ways: * The `public_interests` attribute is disabled by default, so by default other users won't be able to see what a user is following * Who has created proposals/debates/investments/comments is public information, while who is following which elements is not; so enabling `public_activity` shouldn't imply potentially private information should be displayed as well We've considered removing the `public_interests` attribute completely and just hiding the "following" page for everyone except its owner, but keeping it provides more compatibility with existing installations. --- .../users/public_activity_component.rb | 8 +- app/controllers/users_controller.rb | 10 +- app/views/users/_following.html.erb | 2 +- config/locales/en/activerecord.yml | 2 +- config/locales/es/activerecord.yml | 2 +- .../users/public_activity_component_spec.rb | 82 ++++ spec/models/user_spec.rb | 6 + spec/system/users_spec.rb | 388 +++++++++--------- 8 files changed, 287 insertions(+), 213 deletions(-) create mode 100644 spec/components/users/public_activity_component_spec.rb diff --git a/app/components/users/public_activity_component.rb b/app/components/users/public_activity_component.rb index 4c11f6069..8cdaa895c 100644 --- a/app/components/users/public_activity_component.rb +++ b/app/components/users/public_activity_component.rb @@ -1,6 +1,6 @@ class Users::PublicActivityComponent < ApplicationComponent attr_reader :user - delegate :authorized_current_user?, :current_path_with_query_params, to: :helpers + delegate :current_user, :valid_interests_access?, :current_path_with_query_params, to: :helpers def initialize(user) @user = user @@ -24,12 +24,16 @@ class Users::PublicActivityComponent < ApplicationComponent ("debates" if feature?(:debates)), ("budget_investments" if feature?(:budgets)), "comments", - "follows" + ("follows" if valid_interests_access?(user)) ].compact.select { |filter| send(filter).any? } end private + def authorized_current_user? + current_user == user || current_user&.moderator? || current_user&.administrator? + end + def proposals Proposal.where(author_id: user.id) end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0912f3295..8ccee9692 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,18 +1,14 @@ class UsersController < ApplicationController load_and_authorize_resource helper_method :valid_interests_access? - helper_method :authorized_current_user? def show + raise CanCan::AccessDenied if params[:filter] == "follows" && !valid_interests_access?(@user) end private - def valid_interests_access? - @user.public_interests || authorized_current_user? - end - - def authorized_current_user? - @authorized_current_user ||= current_user && (current_user == @user || current_user.moderator? || current_user.administrator?) + def valid_interests_access?(user) + user.public_interests || user == current_user end end diff --git a/app/views/users/_following.html.erb b/app/views/users/_following.html.erb index 480bb2da2..d09f99fe6 100644 --- a/app/views/users/_following.html.erb +++ b/app/views/users/_following.html.erb @@ -22,6 +22,6 @@
- <%= render "interests", user: user if valid_interests_access? %> + <%= render "interests", user: user %>
diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index c2057e07b..2613671b5 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -281,7 +281,7 @@ en: official_level: "Official level" phone_number: "Phone number" public_activity: "Keep my list of activities public" - public_interests: "Keep the labels of the elements I follow public" + public_interests: "Keep the elements I follow public" recommended_debates: "Show debates recommendations" recommended_proposals: "Show proposals recommendations" redeemable_code: "Verification code received via email" diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index a6454e6e3..8af18867c 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -281,7 +281,7 @@ es: official_level: "Nivel del cargo" phone_number: "Teléfono" public_activity: "Mostrar públicamente mi lista de actividades" - public_interests: "Mostrar públicamente las etiquetas de los elementos que sigo" + public_interests: "Mostrar públicamente los elementos que sigo" recommended_debates: "Mostrar recomendaciones en el listado de debates" recommended_proposals: "Mostrar recomendaciones en el listado de propuestas" redeemable_code: "Código de verificación por carta (opcional)" diff --git a/spec/components/users/public_activity_component_spec.rb b/spec/components/users/public_activity_component_spec.rb new file mode 100644 index 000000000..39ae4010d --- /dev/null +++ b/spec/components/users/public_activity_component_spec.rb @@ -0,0 +1,82 @@ +require "rails_helper" + +describe Users::PublicActivityComponent, controller: UsersController do + around do |example| + with_request_url(Rails.application.routes.url_helpers.user_path(user)) { example.run } + end + + describe "follows tab" do + context "public interests is checked" do + let(:user) { create(:user, public_interests: true) } + let(:component) { Users::PublicActivityComponent.new(user) } + + it "is displayed for everyone" do + create(:proposal, author: user, followers: [user]) + + render_inline component + + expect(page).to have_content "1 Following" + end + + it "is not displayed when the user isn't following any followables" do + create(:proposal, author: user) + + render_inline component + + expect(page).not_to have_content "Following" + end + + it "is the active tab when the follows filters is selected" do + create(:proposal, author: user, followers: [user]) + controller.params["filter"] = "follows" + + render_inline component + + expect(page).to have_selector "li.is-active", text: "1 Following" + end + end + + context "public interests is not checked" do + let(:user) { create(:user, public_interests: false) } + let(:component) { Users::PublicActivityComponent.new(user) } + + it "is displayed for its owner" do + create(:proposal, followers: [user]) + sign_in(user) + + render_inline component + + expect(page).to have_content "1 Following" + end + + it "is not displayed for anonymous users" do + create(:proposal, author: user, followers: [user]) + + render_inline component + + expect(page).to have_content "1 Proposal" + expect(page).not_to have_content "Following" + end + + it "is not displayed for other users" do + create(:proposal, author: user, followers: [user]) + sign_in(create(:user)) + + render_inline component + + expect(page).to have_content "1 Proposal" + expect(page).not_to have_content "Following" + end + + it "is not displayed for administrators" do + create(:proposal, author: user, followers: [user]) + sign_in(create(:administrator).user) + + render_inline component + + expect(page).to have_content "1 Proposal" + expect(page).not_to have_content "Following" + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3c6103c44..d24392549 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -739,6 +739,12 @@ describe User do end end + describe "#public_interests" do + it "is false by default" do + expect(User.new.public_interests).to be false + end + end + describe ".find_by_manager_login" do it "works with a low ID" do user = create(:user) diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index 5a9607f9c..127bb1b11 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -231,106 +231,6 @@ describe "Users" do end end - describe "Public interests" do - let(:user) { create(:user) } - - scenario "Display interests" do - create(:proposal, tag_list: "Sport", followers: [user]) - - login_as(user) - visit account_path - - check "account_public_interests" - click_button "Save changes" - - logout - - visit user_path(user, filter: "follows") - - expect(page).to have_css "#public_interests" - expect(page).to have_content "Sport" - end - - scenario "Not display interests when proposal has been destroyed" do - proposal = create(:proposal, tag_list: "Sport", followers: [user]) - proposal.destroy! - - login_as(user) - visit account_path - - check "account_public_interests" - click_button "Save changes" - - logout - - visit user_path(user) - expect(page).not_to have_content("Sport") - end - - scenario "No visible by default" do - visit user_path(user) - - expect(page).to have_content(user.username) - expect(page).not_to have_css("#public_interests") - end - - scenario "Is always visible for the owner" do - create(:proposal, tag_list: "Sport", followers: [user]) - - login_as(user) - visit account_path - - uncheck "account_public_interests" - click_button "Save changes" - - visit user_path(user, filter: "follows", page: "1") - - expect(page).to have_css "#public_interests" - expect(page).to have_content "Tags of elements you follow" - end - - scenario "Is always visible for admins" do - create(:proposal, tag_list: "Sport", followers: [user]) - - login_as(user) - visit account_path - - uncheck "account_public_interests" - click_button "Save changes" - - logout - - login_as(create(:administrator).user) - visit user_path(user, filter: "follows", page: "1") - expect(page).to have_css("#public_interests") - end - - scenario "Is always visible for moderators" do - create(:proposal, tag_list: "Sport", followers: [user]) - - login_as(user) - visit account_path - - uncheck "account_public_interests" - click_button "Save changes" - - logout - - login_as(create(:moderator).user) - visit user_path(user, filter: "follows", page: "1") - expect(page).to have_css("#public_interests") - end - - scenario "Should display generic interests title" do - create(:proposal, tag_list: "Sport", followers: [user]) - - user.update!(public_interests: true) - visit user_path(user, filter: "follows", page: "1") - - expect(page).to have_content("Tags of elements this user follows") - end - end - describe "Special comments" do scenario "comments posted as moderator are not visible in user activity" do moderator = create(:administrator).user @@ -386,117 +286,203 @@ describe "Users" do describe "Following (public page)" do let(:user) { create(:user) } - scenario "Do not display follows' tab when user is not following any followables" do - visit user_path(user) + context "public interests is checked" do + let(:user) { create(:user, public_interests: true) } - expect(page).not_to have_content("0 Following") + scenario "can be accessed by anyone" do + create(:proposal, followers: [user], title: "Others follow me") + + visit user_path(user, filter: "follows") + + expect(page).to have_content "1 Following" + expect(page).to have_content "Others follow me" + end + + scenario "Gracefully handle followables that have been hidden" do + create(:proposal, followers: [user]) + create(:proposal, followers: [user], &:hide) + + visit user_path(user) + + expect(page).to have_content("1 Following") + end + + scenario "displays generic interests title" do + create(:proposal, tag_list: "Sport", followers: [user]) + + visit user_path(user, filter: "follows", page: "1") + + expect(page).to have_content("Tags of elements this user follows") + end + + describe "Proposals" do + scenario "Display following tab when user is following one proposal at least" do + create(:proposal, followers: [user]) + + visit user_path(user) + + expect(page).to have_content("1 Following") + end + + scenario "Display proposal tab when user is following one proposal at least" do + create(:proposal, followers: [user]) + + visit user_path(user, filter: "follows") + + expect(page).to have_link("Citizen proposals", href: "#citizen_proposals") + end + + scenario "Do not display proposals' tab when user is not following any proposal" do + visit user_path(user, filter: "follows") + + expect(page).not_to have_link("Citizen proposals", href: "#citizen_proposals") + end + + scenario "Display proposals with link to proposal" do + proposal = create(:proposal, author: user, followers: [user]) + + login_as user + + visit user_path(user, filter: "follows") + + expect(page).to have_link "Citizen proposals", href: "#citizen_proposals" + expect(page).to have_content proposal.title + end + + scenario "Retired proposals do not have a link to the dashboard" do + proposal = create(:proposal, :retired, author: user) + login_as user + + visit user_path(user) + + expect(page).to have_content proposal.title + expect(page).not_to have_link "Dashboard" + expect(page).to have_content("Dashboard not available for retired proposals") + end + + scenario "Published proposals have a link to the dashboard" do + proposal = create(:proposal, :published, author: user) + login_as user + + visit user_path(user) + + expect(page).to have_content proposal.title + expect(page).to have_link "Dashboard" + end + end + + describe "Budget Investments" do + scenario "Display following tab when user is following one budget investment at least" do + create(:budget_investment, followers: [user]) + + visit user_path(user) + + expect(page).to have_content("1 Following") + end + + scenario "Display budget investment tab when user is following one budget investment at least" do + create(:budget_investment, followers: [user]) + + visit user_path(user, filter: "follows") + + expect(page).to have_link("Investments", href: "#investments") + end + + scenario "Do not display budget investment tab when user is not following any budget investment" do + visit user_path(user, filter: "follows") + + expect(page).not_to have_link("Investments", href: "#investments") + end + + scenario "Display budget investment with link to budget investment" do + budget_investment = create(:budget_investment, author: user, followers: [user]) + + visit user_path(user, filter: "follows") + + expect(page).to have_link "Investments", href: "#investments" + expect(page).to have_link budget_investment.title + end + end end - scenario "Active following tab by default when follows filters selected" do - create(:proposal, author: user, followers: [user]) + context "public interests is not checked" do + let(:user) { create(:user, public_interests: false) } + + scenario "can be accessed by its owner" do + create(:proposal, followers: [user], title: "Follow me!") + + login_as(user) + + visit user_path(user, filter: "follows") + + expect(page).to have_content "1 Following" + expect(page).to have_content "Follow me!" + expect(page).to have_content "Tags of elements you follow" + end + + scenario "cannot be accessed by anonymous users" do + create(:proposal, followers: [user]) + + visit user_path(user, filter: "follows") + + expect(page).to have_content "You do not have permission to access this page" + expect(page).to have_current_path root_path + end + + scenario "cannot be accessed by other users" do + create(:proposal, followers: [user]) + + login_as(create(:user)) + + visit user_path(user, filter: "follows") + + expect(page).to have_content "You do not have permission to access this page" + expect(page).to have_current_path root_path + end + + scenario "cannot be accessed by administrators" do + create(:proposal, followers: [user]) + + login_as(create(:administrator).user) + + visit user_path(user, filter: "follows") + + expect(page).to have_content "You do not have permission to access this page" + expect(page).to have_current_path root_path + end + end + + scenario "Display interests" do + create(:proposal, tag_list: "Sport", followers: [user]) + + login_as(user) + visit account_path + + check "account_public_interests" + click_button "Save changes" + + logout visit user_path(user, filter: "follows") - expect(page).to have_selector(".activity li.is-active", text: "1 Following") + expect(page).to have_css "#public_interests" + expect(page).to have_content "Sport" end - scenario "Gracefully handle followables that have been hidden" do - create(:proposal, followers: [user]) - create(:proposal, followers: [user], &:hide) + scenario "Do not display interests when proposal has been destroyed" do + proposal = create(:proposal, tag_list: "Sport", followers: [user]) + proposal.destroy! + + login_as(user) + visit account_path + + check "account_public_interests" + click_button "Save changes" + + logout visit user_path(user) - - expect(page).to have_content("1 Following") - end - - describe "Proposals" do - scenario "Display following tab when user is following one proposal at least" do - create(:proposal, followers: [user]) - - visit user_path(user) - - expect(page).to have_content("1 Following") - end - - scenario "Display proposal tab when user is following one proposal at least" do - create(:proposal, followers: [user]) - - visit user_path(user, filter: "follows") - - expect(page).to have_link("Citizen proposals", href: "#citizen_proposals") - end - - scenario "Do not display proposals' tab when user is not following any proposal" do - visit user_path(user, filter: "follows") - - expect(page).not_to have_link("Citizen proposals", href: "#citizen_proposals") - end - - scenario "Display proposals with link to proposal" do - proposal = create(:proposal, author: user, followers: [user]) - - login_as user - - visit user_path(user, filter: "follows") - - expect(page).to have_link "Citizen proposals", href: "#citizen_proposals" - expect(page).to have_content proposal.title - end - - scenario "Retired proposals do not have a link to the dashboard" do - proposal = create(:proposal, :retired, author: user) - login_as user - - visit user_path(user) - - expect(page).to have_content proposal.title - expect(page).not_to have_link "Dashboard" - expect(page).to have_content("Dashboard not available for retired proposals") - end - - scenario "Published proposals have a link to the dashboard" do - proposal = create(:proposal, :published, author: user) - login_as user - - visit user_path(user) - - expect(page).to have_content proposal.title - expect(page).to have_link "Dashboard" - end - end - - describe "Budget Investments" do - scenario "Display following tab when user is following one budget investment at least" do - create(:budget_investment, followers: [user]) - - visit user_path(user) - - expect(page).to have_content("1 Following") - end - - scenario "Display budget investment tab when user is following one budget investment at least" do - create(:budget_investment, followers: [user]) - - visit user_path(user, filter: "follows") - - expect(page).to have_link("Investments", href: "#investments") - end - - scenario "Not display budget investment tab when user is not following any budget investment" do - visit user_path(user, filter: "follows") - - expect(page).not_to have_link("Investments", href: "#investments") - end - - scenario "Display budget investment with link to budget investment" do - user = create(:user, :level_two) - budget_investment = create(:budget_investment, author: user, followers: [user]) - - visit user_path(user, filter: "follows") - - expect(page).to have_link "Investments", href: "#investments" - expect(page).to have_link budget_investment.title - end + expect(page).not_to have_content("Sport") end end From af53b2159cf83ed5d9efb60021efd81a4ad9bb72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Sep 2021 22:56:54 +0200 Subject: [PATCH 8/9] Refactor code getting a user's comments This way it's more readable and easier to change. --- .../users/public_activity_component.rb | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/app/components/users/public_activity_component.rb b/app/components/users/public_activity_component.rb index 8cdaa895c..653bf89c7 100644 --- a/app/components/users/public_activity_component.rb +++ b/app/components/users/public_activity_component.rb @@ -43,7 +43,11 @@ class Users::PublicActivityComponent < ApplicationComponent end def comments - only_active_commentables.includes(:commentable) + Comment.not_valuations + .not_as_admin_or_moderator + .where(user_id: user.id) + .where.not(commentable_type: disabled_commentables) + .includes(:commentable) end def budget_investments @@ -66,18 +70,10 @@ class Users::PublicActivityComponent < ApplicationComponent params[:page] end - def only_active_commentables - disabled_commentables = [] - disabled_commentables << "Debate" unless Setting["process.debates"] - disabled_commentables << "Budget::Investment" unless Setting["process.budgets"] - if disabled_commentables.present? - all_user_comments.where.not(commentable_type: disabled_commentables) - else - all_user_comments - end - end - - def all_user_comments - Comment.not_valuations.not_as_admin_or_moderator.where(user_id: user.id) + def disabled_commentables + [ + ("Debate" unless feature?(:debates)), + ("Budget::Investment" unless feature?(:budgets)) + ].compact end end From 5c1a7044cd880a3d8d2c9b3c67366b241fe5b012 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Sep 2021 23:16:11 +0200 Subject: [PATCH 9/9] Hide comments when features are disabled We were already doing so for debates and investments. We probably never noticed because this is an edge case that requires enabling a feature, people adding comments, and then disabling the feature. --- .../users/public_activity_component.rb | 9 +++++-- .../users/public_activity_component_spec.rb | 25 +++++++++++++++++++ spec/factories/comments.rb | 3 ++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/app/components/users/public_activity_component.rb b/app/components/users/public_activity_component.rb index 653bf89c7..61c1fbb8e 100644 --- a/app/components/users/public_activity_component.rb +++ b/app/components/users/public_activity_component.rb @@ -73,7 +73,12 @@ class Users::PublicActivityComponent < ApplicationComponent def disabled_commentables [ ("Debate" unless feature?(:debates)), - ("Budget::Investment" unless feature?(:budgets)) - ].compact + ("Budget::Investment" unless feature?(:budgets)), + (["Legislation::Question", + "Legislation::Proposal", + "Legislation::Annotation"] unless feature?(:legislation)), + (["Poll", "Poll::Question"] unless feature?(:polls)), + ("Proposal" unless feature?(:proposals)) + ].flatten.compact end end diff --git a/spec/components/users/public_activity_component_spec.rb b/spec/components/users/public_activity_component_spec.rb index 39ae4010d..2f72c8495 100644 --- a/spec/components/users/public_activity_component_spec.rb +++ b/spec/components/users/public_activity_component_spec.rb @@ -79,4 +79,29 @@ describe Users::PublicActivityComponent, controller: UsersController do end end end + + describe "comments" do + let(:user) { create(:user) } + let(:component) { Users::PublicActivityComponent.new(user) } + + it "doesn't show comments for disabled features" do + Setting["process.budgets"] = false + Setting["process.debates"] = false + Setting["process.legislation"] = false + Setting["process.polls"] = false + Setting["process.proposals"] = false + + create(:budget_investment_comment, user: user) + create(:debate_comment, user: user) + create(:legislation_annotation_comment, user: user) + create(:legislation_question_comment, user: user) + create(:legislation_proposal_comment, user: user) + create(:poll_comment, user: user) + create(:proposal_comment, user: user) + + render_inline component + + expect(page).not_to have_content "Comments" + end + end end diff --git a/spec/factories/comments.rb b/spec/factories/comments.rb index 79402c448..f55581f88 100644 --- a/spec/factories/comments.rb +++ b/spec/factories/comments.rb @@ -4,7 +4,8 @@ FactoryBot.define do user sequence(:body) { |n| "Comment body #{n}" } - %i[budget_investment debate legislation_annotation legislation_question proposal topic_with_community].each do |model| + %i[budget_investment debate legislation_annotation legislation_question legislation_proposal + poll proposal topic_with_community].each do |model| factory :"#{model}_comment" do association :commentable, factory: model end