From f790c3c0c1842ae65619546af69d15dc8431d542 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 11 Apr 2022 13:40:16 -0400 Subject: [PATCH] Encapsulating the "why we hide/show" something (#17166) * Encapsulating the "why we hide/show" something Prior to this commit, we were relying on our views/templates to understand why it might want to append the "hidden" dom class. That is not ideal, because it required repeating knowledge about the conditions of the policy outside of the policy's domain. With this commit, we're removing a duplication of knowledge; or more appropriately asking the steward responsible for the knowledge to produce the answer. * Refining the note to not describe a course of action --- app/helpers/application_helper.rb | 11 ++----- app/models/async_info.rb | 2 +- app/policies/application_policy.rb | 26 +++++++++++++-- app/policies/article_policy.rb | 19 ++++++++++- app/views/layouts/_nav_menu.html.erb | 4 +-- app/views/layouts/_top_bar.html.erb | 2 +- .../stories/tagged_articles/_sidebar.html.erb | 2 +- spec/helpers/application_helper_spec.rb | 20 ++---------- spec/policies/application_policy_spec.rb | 21 +++++++++--- spec/policies/article_policy_spec.rb | 32 +++++++++++++++---- 10 files changed, 94 insertions(+), 45 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 6b9db83fc..3b22823c4 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -379,8 +379,6 @@ module ApplicationHelper # @param name [String,Symbol] the HTML element name (e.g. "li", "div", "a") # @param record [Object] the record for which we're testing a policy # @param query [Symbol, String] the query we're running on the policy - # @param feature_flag [Symbol] the named feature flag that when enabled will add the "hidden" - # class to the dom element. # @param kwargs [Hash] The arguments pass, with modifications to the given :class (see # implementation details). # @@ -389,14 +387,9 @@ module ApplicationHelper # @see ApplicationPolicy.dom_class_for # @see https://api.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-content_tag # @see ./app/javascript/packs/applyApplicationPolicyToggles.js - # - # @note This assumes that for the given feature flag, when enabled, we will add the "hidden" - # class. - # @note I hope you are correctly busting the cache for this feature_flag - def application_policy_content_tag(name, record:, query:, feature_flag:, **kwargs, &block) + def application_policy_content_tag(name, record:, query:, **kwargs, &block) dom_class = kwargs.delete(:class) || kwargs.delete("class") || "" - dom_class += " #{ApplicationPolicy.dom_class_for(record: record, query: query)}" - dom_class += " hidden" if FeatureFlag.enabled?(feature_flag) + dom_class += " #{ApplicationPolicy.dom_classes_for(record: record, query: query)}" content_tag(name, class: dom_class, **kwargs, &block) end diff --git a/app/models/async_info.rb b/app/models/async_info.rb index 3f7b0a39f..230da38cb 100644 --- a/app/models/async_info.rb +++ b/app/models/async_info.rb @@ -51,7 +51,7 @@ class AsyncInfo admin: user.any_admin?, policies: [ { - dom_class: ApplicationPolicy.dom_class_for(record: Article, query: :create?), + dom_class: ApplicationPolicy.base_dom_class_for(record: Article, query: :create?), visible: visible?(record: Article, query: :create?) }, ], diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 856778dea..4e37e769c 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -91,7 +91,7 @@ class ApplicationPolicy raise ApplicationPolicy::UserRequiredError, I18n.t("policies.application_policy.you_must_be_logged_in") end - # This method provides a means for creating a consistent DOM class for "policy" related HTML elements. + # This method provides a means for creating consistent DOM classes for "policy" related HTML elements. # # @param record [Object] what object are we testing our policy? # @param query [String,Symbol] what query are we asking of the object's corresponding policy? @@ -100,9 +100,19 @@ class ApplicationPolicy # call (e.g. `policy(Article).create?`) # # @see Pundit::Authorization.policy pundit's #policy helper method + # @see Pundit::PolicyFinder # # @return [String] a dom class compliant string, see the corresponding specs for expected values. - def self.dom_class_for(record:, query:) + def self.dom_classes_for(record:, query:) + dom_classes = [base_dom_class_for(record: record, query: query)] + # I don't want the Policy instance, because due to construction, that could raise an exception. + # The class will do just fine. + policy_class = Pundit::PolicyFinder.new(record).policy + dom_classes << "hidden" if policy_class&.include_hidden_dom_class_for?(query: query) + dom_classes.join(" ") + end + + def self.base_dom_class_for(record:, query:) fragments = %w[js policy] case record when Symbol @@ -117,6 +127,18 @@ class ApplicationPolicy fragments.join("-") end + # @api private + # @abstract + # + # @param query [Symbol] the method name we would be calling on the policy instance. + # @return [TrueClass] if we should hide the dom element. + # @return [FalseClass] if we should not hide the dom element. + # rubocop:disable Lint/UnusedMethodArgument + def self.include_hidden_dom_class_for?(query:) + false + end + # rubocop:enable Lint/UnusedMethodArgument + # @param user [User] who's the one taking the action? # # @param record [Class, Object] what is the user acting on? This could be a model (e.g. Article) diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb index 221dfc4c1..76c8fb00f 100644 --- a/app/policies/article_policy.rb +++ b/app/policies/article_policy.rb @@ -12,6 +12,21 @@ class ArticlePolicy < ApplicationPolicy FeatureFlag.enabled?(:limit_post_creation_to_admins) end + # @param query [Symbol] the name of one of the ArticlePolicy action predicates (e.g. :create?, + # :new?) though as a convenience, we will also accept :new, and :create. + # @return [TrueClass] if this query should default to hidden + # @return [FalseClass] if this query should not be hidden in the UI. + # + # @note The symmetry of the case statement structure with .scope_users_authorized_to_action + def self.include_hidden_dom_class_for?(query:) + case query.to_sym + when :create?, :new?, :create, :new + limit_post_creation_to_admins? + else + false + end + end + # Helps filter a `:users_scope` to those authorized to the `:action`. I want a list of all users # who can create an Article. This policy method can help with that. # @@ -33,8 +48,10 @@ class ArticlePolicy < ApplicationPolicy # # @note Why isn't this a User.scope method? Because the logic of who can take an action on the # resource is the problem domain of the policy. + # + # @note The symmetry of the case statement structure with .include_hidden_dom_class_for? def self.scope_users_authorized_to_action(users_scope:, action:) - case action + case action.to_sym when :create?, :new?, :create, :new # Note the delicate dance to duplicate logic in a general sense. [I hope that] this is a # stop-gap solution. diff --git a/app/views/layouts/_nav_menu.html.erb b/app/views/layouts/_nav_menu.html.erb index 705abcf19..f77c478bd 100644 --- a/app/views/layouts/_nav_menu.html.erb +++ b/app/views/layouts/_nav_menu.html.erb @@ -18,9 +18,9 @@
  • <%= t("views.main.nav.moderator_center") %>
    • - + <% end %>
  • <%= t("views.main.nav.list") %>
    • diff --git a/app/views/layouts/_top_bar.html.erb b/app/views/layouts/_top_bar.html.erb index b0bd64019..d769b74e0 100644 --- a/app/views/layouts/_top_bar.html.erb +++ b/app/views/layouts/_top_bar.html.erb @@ -29,7 +29,7 @@ <% if user_signed_in? %>