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

@@ -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

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?)
         },
       ],

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)

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.

app/views/layouts/_nav_menu.html.erb

@@ -18,9 +18,9 @@
     <li>
       <a href="<%= mod_path %>" class="c-link c-link--block trusted-visible-block"><%= t("views.main.nav.moderator_center") %></a>
     </li>
-    <li class="<%= ApplicationPolicy.dom_class_for(record: Article, query: :create?) %> hidden">
+    <%= application_policy_content_tag("li", record: Article, query: :create?) do %>
       <a href="<%= new_path %>" class="c-link c-link--block"><%= t("views.main.create_post") %></a>
-    </li>
+    <% end %>
     <li>
       <a href="<%= readinglist_path %>" class="c-link c-link--block"><%= t("views.main.nav.list") %></a>
     </li>

app/views/layouts/_top_bar.html.erb

@@ -29,7 +29,7 @@
       </a>
       <% if user_signed_in? %>
         <span class="hidden m:flex ml-auto">
-          <%= application_policy_content_tag("a", record: Article, query: :create?, feature_flag: :limit_post_creation_to_admins, href: new_path, class: "c-cta c-cta--branded mr-2 whitespace-nowrap") do %>
+          <%= application_policy_content_tag("a", record: Article, query: :create?, href: new_path, class: "c-cta c-cta--branded mr-2 whitespace-nowrap") do %>
             <%= t("views.main.create_post") %>
           <% end %>
 

app/views/stories/tagged_articles/_sidebar.html.erb

@@ -4,7 +4,7 @@
     <aside class="side-bar">
       <% if @tag && @tag.rules_html.present? %>
         <div class="widget">
-        <%= application_policy_content_tag("div", record: Article, query: :create?, feature_flag: :limit_post_creation_to_admins, class: "widget") do %>
+        <%= application_policy_content_tag("div", record: Article, query: :create?, class: "widget") do %>
           <header>
             <h4><%= t("views.tags.sidebar.guidelines") %></h4>
           </header>

spec/helpers/application_helper_spec.rb

@@ -320,29 +320,13 @@ RSpec.describe ApplicationHelper, type: :helper do
 
   describe "#application_policy_content_tag" do
     subject(:content) do
-      application_policy_content_tag(html_tag_type, record: Article, query: :create?, feature_flag: given_feature_flag,
-                                                    class: "something") do
+      application_policy_content_tag("p", record: Article, query: :create?, class: "something") do
         "My Content"
       end
     end
 
-    let(:given_feature_flag) { :hello_world }
-    let(:html_tag_type) { "p" }
-
-    it "has a policy class based on record and query" do
+    it "adds the policy related classes to the HTML tag element element" do
       expect(content).to include(%(<p class="something js-policy-article-create">My Content</p>))
     end
-
-    context "when the given flag is enabled" do
-      around do |example|
-        FeatureFlag.enable(given_feature_flag)
-        example.call
-        FeatureFlag.remove(given_feature_flag)
-      end
-
-      it "adds the 'hidden' to the HTML element's class list" do
-        expect(content).to include(%(<p class="something js-policy-article-create hidden">My Content</p>))
-      end
-    end
   end
 end

spec/policies/application_policy_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe ApplicationPolicy do
   end
   # rubocop:enable RSpec/DescribedClass
 
-  describe ".dom_class_for" do
+  describe ".dom_classes_for" do
     [
       [Article, :create?, "js-policy-article-create"],
       [:Article, :create?, "js-policy-article-create"],
@@ -35,13 +35,26 @@ RSpec.describe ApplicationPolicy do
       [:WorkerBee, :create?, "js-policy-worker_bee-create"],
       [Article.new(id: 5), :create, "js-policy-article-5-create"],
       [Article.new, :create, "js-policy-article-new-create"],
-    ].each do |record, query, expected|
-      context "when record=#{record.inspect} and query=#{query.inspect}" do
-        subject { described_class.dom_class_for(record: record, query: query) }
+      [
+        Article.new,
+        :create,
+        "js-policy-article-new-create hidden",
+        # This needs to be a proc and not a lambda; if it's a lambda, the context for evaluation is
+        # wildly off.  (and you need to do `before { instance_exec(&before_proc) }`); This is the
+        # way to make the `before` call below the least surprising.
+        proc {
+          allow(ArticlePolicy).to receive(:include_hidden_dom_class_for?).with(query: :create).and_return(true)
+        },
+      ],
+    ].each do |record, query, expected, before_proc|
+      context "when record=#{record.inspect} and query=#{query.inspect}#{' with hidden true' if before_proc}" do
+        subject { described_class.dom_classes_for(record: record, query: query) }
 
         let(:record) { record }
         let(:query) { query }
 
+        before(&before_proc) if before_proc
+
         it { is_expected.to eq(expected) }
       end
     end

spec/policies/article_policy_spec.rb

@@ -20,17 +20,20 @@ RSpec.describe ArticlePolicy do
   let(:policy) { described_class.new(user, resource) }
 
   describe ".scope_users_authorized_to_action" do
-    let!(:regular_user) { create(:user) }
-    let!(:super_admin_user) { create(:user, :super_admin) }
-
-    before { create(:user, :suspended) }
+    before do
+      User.destroy_all # For some reason I'm getting extra users than there should be
+      super_admin
+      author
+      suspended_user
+    end
 
     context "when limit_post_creation_to_admins is true" do
       before { allow(described_class).to receive(:limit_post_creation_to_admins?).and_return(true) }
 
       it "omits suspended and regular users" do
         results = described_class.scope_users_authorized_to_action(users_scope: User, action: :create?).to_a
-        expect(results).to match_array([super_admin_user])
+
+        expect(results).to match_array([super_admin])
       end
     end
 
@@ -39,7 +42,24 @@ RSpec.describe ArticlePolicy do
 
       it "omits only suspended users" do
         results = described_class.scope_users_authorized_to_action(users_scope: User, action: :create?).to_a
-        expect(results).to match_array([regular_user, super_admin_user])
+        expect(results).to match_array([author, super_admin])
+      end
+    end
+  end
+
+  describe ".include_hidden_dom_class_for?" do
+    [
+      [true, :create?, true],
+      [false, :create?, false],
+      [true, :edit?, false],
+      [false, :edit?, false],
+    ].each do |limit, query, expected_value|
+      context "when limit_post_creation_to_admins is #{limit} and query is #{query}" do
+        subject { described_class.include_hidden_dom_class_for?(query: query) }
+
+        before { allow(described_class).to receive(:limit_post_creation_to_admins?).and_return(limit) }
+
+        it { is_expected.to eq(expected_value) }
       end
     end
   end