Conditionally reducing dashboard chatter for users (#16999)

This commit provides two things:

1.  Some notes related to my analysis regarding the dashboard
2.  Conditional redirects and rendering based on article policies

The code comments say most of what I want to say, but to reiterate:

When a user can't create articles nor do they already have published
articles, then we don't want to avoid showing them stats related to
articles.

Closes forem/forem#16913
Related to forem/forem#16908 and forem/forem#16931
This commit is contained in:
Jeremy Friesen 2022-03-25 13:59:53 -04:00 committed by GitHub
parent fcdb9d3fe5
commit 75041ff93f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 44 additions and 0 deletions

View file

@ -1,3 +1,16 @@
# @note The actions of this class are overloaded with three concerns:
#
# - the current user
# - a given user
# - a given organization
#
# The implementation details are such that things silently "fallback" to the current user's
# information. This fallback happens when we have quasi-policy checks say the current user
# can't access the given user or given organization.
#
# [@jeremyf] I'm including these notes for future refactors, as I've spent time trying to
# improve legibility of the code but there are logical assumptions that require revisiting
# (hence https://github.com/forem/forem/issues/16931).
class DashboardsController < ApplicationController
before_action :set_no_cache_header
before_action :authenticate_user!
@ -7,15 +20,22 @@ class DashboardsController < ApplicationController
def show
fetch_and_authorize_user
target = @user
# NOTE: This is a subtle policy check happening here that we are not encapsulating
not_authorized if params[:org_id] && !@user.org_admin?(params[:org_id] || @user.any_admin?)
@organizations = @user.admin_organizations
# NOTE: This logic is a super set of the above not_authorized check
if params[:which] == "organization" && params[:org_id] && (@user.org_admin?(params[:org_id]) || @user.any_admin?)
target = @organizations.find_by(id: params[:org_id])
@organization = target
@articles = target.articles
else
# This redirect assumes that the dashboards#show action renders article specific information.
# When a user doesn't have articles nor can they create them, we want to send them somewhere
# else.
redirect_to dashboard_following_tags_path unless policy(Article).has_existing_articles_or_can_create_new_ones?
# if the target is a user, we need to eager load the organization
@articles = target.articles.includes(:organization)
end
@ -92,6 +112,7 @@ class DashboardsController < ApplicationController
else
current_user
end
# NOTE: later we expect @user so the `||` is a bit misleading.
authorize (@user || User), :dashboard_show?
end

View file

@ -1,5 +1,6 @@
<nav class="hidden m:block" aria-label="Dashboards">
<ul class="list-none p-0">
<% if policy(Article).has_existing_articles_or_can_create_new_ones? %>
<li>
<a class="crayons-link crayons-link--block <%= "crayons-link--current" if params[:action] == "show" && (params[:which] == "organization" || params[:which].blank?) %>"
href="<%= dashboard_path %>"
@ -26,6 +27,7 @@
<span class="c-indicator"><%= @user.followers_count %></span>
</a>
</li>
<% end %>
<li>
<a class="crayons-link crayons-link--block <%= "crayons-link--current" if params[:action] == "following_tags" %>"
@ -72,11 +74,13 @@
</li>
<%- end %>
<%- if policy(Article).has_existing_articles_or_can_create_new_ones? %>
<li>
<a class="crayons-link crayons-link--block" href="<%= dashboard_analytics_path %>">
<%= t("views.dashboard.actions.analytics") %>
</a>
</li>
<%- end %>
<% if @organizations && (params[:which].blank? || params[:which] == "organization") %>
<% @organizations.each do |org| %>

View file

@ -97,6 +97,25 @@ RSpec.describe "Dashboards", type: :request do
end
end
context "when logged but has no articles nor can create them" do
it "redirects to /dashboard/following_tags" do
sign_in user
# [@jeremyf] I'm choosing not to setup the exact conditions of the data for this to be true.
# Instead, I'm relying on that function to already be tested.
#
# rubocop:disable RSpec/AnyInstance
# Pundit does not make it easy to stub the policy().method questions so I'm using the any instance antics.
allow_any_instance_of(ArticlePolicy)
.to receive(:has_existing_articles_or_can_create_new_ones?)
.and_return(false)
# rubocop:enable RSpec/AnyInstance
get dashboard_path
expect(response).to redirect_to("/dashboard/following_tags")
end
end
context "when logged in as a super admin" do
it "renders the specified user's articles" do
article