Update policies and tests to handle button normalization #844

maebeale · 2026-02-04T16:15:41Z

This work Closes [link an issue]

What is the goal of this PR and why is this important?

Improve UX and change policy defaults

Update all form buttons to use the same code, and change tests to expect the new en.yml text
Change ApplicationPolicy to default everything to admin?
Add policies for the other models that have forms so allowed_to? will find the right policy
Rename public -> publicly_visible

How did you approach the change?

Anything else to add?

maebeale · 2026-02-04T16:19:28Z

app/controllers/application_controller.rb

@@ -1,4 +1,5 @@
 class ApplicationController < ActionController::Base
+  include ActionPolicy::Controller


this gives helper methods including allowed_to?, which we're now using on forms to determine if delete button should show

Suggested change

include ActionPolicy::Controller

Shouldn't need this explicitly. allowed_to? was already working for me and the doc's say its included in ActionController::Base

Were you getting an error without it?

oh interesting. yes, was getting an error.

i just removed it and everything still worked. TY for catching this!

maebeale · 2026-02-04T16:22:07Z

app/controllers/workshop_ideas_controller.rb

    @sectors = Sector.published
    @windows_types = WindowsType.all
+    # @category_types = CategoryType.includes(:categories).published.decorate
+    @categories_grouped =


tags need the grouped categories, not @category_types

maebeale · 2026-02-04T16:22:37Z

app/policies/ahoy_activity_policy.rb

    admin?
  end

-  def recent?


not using this endpoint anymore

maebeale · 2026-02-04T16:23:48Z

app/policies/application_policy.rb


  def index?
-    true
+    admin?


defaulting to the most secure first, and then greenlighting everything else in other policies.

this avoids mistakes in forgetting to set admin-only indexes to admin-only. this way we'll need to set not-admin-only to visible.

Good idea.

Can you add index? to the alias_rule manage?. This keeps them all set to admin? in one place and follows the convention from the Action Policy docs.

alias_rule :index?, :new?, :create?, :edit?, :update?, :destroy?, to: :manage?

ooo. great idea! yup, will do that now.

adding both index? and show?

@maebeale might as well!

maebeale · 2026-02-04T16:24:10Z

app/policies/application_policy.rb


  def show?
-    admin? || record.published?
+    admin?


same thing here -- defaulting to lockdown mode and we'll greenlight access in other policies

maebeale · 2026-02-04T16:24:53Z

app/policies/application_policy.rb


  def admin?
-    user&.super_user?
+    user&.admin?


trying to remove/minimize super_user from the code, as i'm sure we'll be moving to a different permissioning implementation once we have more roles.

Do you think it makes more sense to have the user permission checked here as this this the authorization framework? The method in User model works but is that the best place?

The hope is that any check we make for an admin? is via action_policy not directly off current_user

I agree only one place for sure if we can help it.

i like better using it here.

and removing all the places we say (...user.super_user or even ...user.admin) and instead use a policy.

maebeale · 2026-02-04T16:25:44Z

app/policies/banner_policy.rb

+  #   true
+  # end
+  #
+  def show?


we don't have UI for non-admin banner show, but, technically we would be fine w users seeing it.

maebeale · 2026-02-04T16:26:30Z

app/policies/bookmark_policy.rb

+
+  relation_scope do |relation|
+    next relation if admin?
+    relation.where(user: user)


users can only see their own bookmarks on their /personal page

maebeale · 2026-02-04T16:57:51Z

app/controllers/community_news_controller.rb

    if turbo_frame_request?
      per_page = params[:number_of_items_per_page].presence || 12
-      unfiltered = current_user.super_user? ? CommunityNews.all : CommunityNews.published
+      unfiltered =


do we still need to do this if we're using scopes in policies?

If we want to display the count of the results vs total. e.g. 5/20 we need the unfiltered number. If we want to keep it simple we could remove the total.

"Total" gets a bit messy. Total of all? total for the current user? Do we include hidden for admin?

yeah, it is getting messy. i think we'd want to say total for the current user (or guest)?

maebeale · 2026-02-04T16:58:44Z

app/models/community_news.rb

  scope :community_news_name, ->(community_news_name) {
    community_news_name.present? ? where("community_news.name LIKE ?", "%#{community_news_name}%") : all }
+  scope :featured, -> { where(featured: true) }
+  scope :public_featured, -> { published.where(public_featured: true) }


prepending published to all scopes for public_featured and publicly_visible

maebeale · 2026-02-04T18:30:10Z

app/views/assets/_upload.html.erb

    </div>

-    <div>
+    <div class="action-buttons mt-8 flex justify-center gap-3">


added this class set to all form action sections

maebeale · 2026-02-04T18:30:46Z

app/views/banners/_form.html.erb

      <%= link_to "Cancel", banners_path, class: "btn btn-secondary-outline" %>
-
-      <%= f.button :submit,
-                   (@banner.new_record? ? "Create Banner" : "Update Banner"),


don't need conditional text if using f.button :submit and en.yml

maebeale · 2026-02-04T18:31:21Z

app/views/banners/_form.html.erb

-                    class: "btn btn-danger-outline",
-                    data: { confirm: "Are you sure you want to delete this banner?" } %>
+    <div class="action-buttons mt-8 flex justify-center gap-3">
+      <% if f.object.persisted? && allowed_to?(:destroy?, f.object) %>


explicitly checking destroy permission before displaying delete button

maebeale · 2026-02-04T18:31:57Z

app/views/events/_form.html.erb


  <% if @event.errors.any? %>
-    <%= render 'shared/errors', resource: @event %>
+    <%= render "shared/errors", resource: @event %>


some ' -> " changes throughout

maebeale · 2026-02-04T18:32:54Z

app/views/windows_types/_form.html.erb

-    <%= f.input :short_name,
-                input_html: { readonly: true },
-                as: :string %>
+    <% if params[:admin] || !f.object.persisted? %>


decided to offer an easter egg to change short_name from the UI

maebeale · 2026-02-04T18:33:19Z

app/views/workshop_ideas/_form.html.erb


-<% 
-=begin %>
+<%# begin %>


not sure why there's begin and end tags in here...

Something was broken for me when I was fixing the workshop promotion.

so it can be removed now?

Yes, just need to check for error's on the page and flow but that can be a separate issue.

i'll leave it in there for you to remove when you're ready

maebeale · 2026-02-04T18:34:05Z

app/views/workshop_ideas/_form.html.erb

-=end %>
+<%# =end %>
+
+    <div class="my-4"><%= render "assets/form", owner: workshop_idea %></div>


@jmilljr24 this doesn't seem broken when moved?

In html you are not supposed to have a form nested within a form. It's hard to see on github where the main form ends so I can't tell exactly where you moved it.

yeah, it is within it...

i moved it back out

maebeale · 2026-02-04T18:35:28Z

config/locales/en.yml

-        project: "Organization"
+        project: "Organization"
+  helpers:
+    submit:


this is where all the f.button :submit text will come from now

maebeale · 2026-02-04T18:35:46Z

db/migrate/20260204163604_add_publicly_visible_to_models.rb

@@ -0,0 +1,6 @@
+class AddPubliclyVisibleToModels < ActiveRecord::Migration[8.1]
+  def change


adding publicly_visible to the two help-related tables too

maebeale · 2026-02-04T18:36:17Z

spec/factories/assets.rb

    # --- Traits for type-specific subclasses ---
    factory :primary_asset, class: "PrimaryAsset"
-    factory :gallery_assets, class: "GalleryAsset"
+    factory :gallery_asset, class: "GalleryAsset"


factories are always singularly named

maebeale · 2026-02-04T18:36:57Z

spec/factories/events.rb

    registration_close_date { 13.days.from_now }
    inactive { false }
    cost_cents { 1099 }
+    public { false }


setting this to public false to start to make tests better match default state of the object

maebeale · 2026-02-04T18:37:28Z

spec/requests/tags_spec.rb

-    it "checks authorization via ApplicationPolicy" do
-      expect_any_instance_of(ApplicationPolicy)
-        .to receive(:index?).and_call_original
+    it "checks authorization via TagPolicy" do


added a TagPolicy, to referring to that now

maebeale · 2026-02-04T18:38:13Z

spec/system/events_show_spec.rb

      visit event_path(event)

-      # Title (uses title_with_badges)
-      expect(page).to have_css("div.my-3", text: "My Event")


removing testing on specific css

maebeale · 2026-02-04T18:38:28Z

spec/system/events_show_spec.rb

+    end

-      # Times block exists
-      expect(page).to have_text("Start:")


this text isn't on the page anymore

maebeale · 2026-02-04T18:39:57Z

spec/system/events_show_spec.rb

        create(:event_registration, event: event, registrant: user)
      end

-      it "shows De-register button + registered badge + calendar links" do


chatgpt changes

maebeale · 2026-02-04T18:41:08Z

spec/views/windows_types/new.html.erb_spec.rb


      # READONLY FIELDS (since this is used in code, we're not exposing it to the user)
-      assert_select "input[name=?][readonly]", "windows_type[short_name]"
+      assert_select "input[name=?]", "windows_type[short_name]"


not readonly when new, since it's a required field

jmilljr24 · 2026-02-04T19:02:32Z

app/controllers/community_news_controller.rb

    if turbo_frame_request?
      per_page = params[:number_of_items_per_page].presence || 12
-      unfiltered = current_user.super_user? ? CommunityNews.all : CommunityNews.published
+      unfiltered =


If we want to display the count of the results vs total. e.g. 5/20 we need the unfiltered number. If we want to keep it simple we could remove the total.

"Total" gets a bit messy. Total of all? total for the current user? Do we include hidden for admin?

jmilljr24 · 2026-02-04T19:05:24Z

app/controllers/application_controller.rb

@@ -1,4 +1,5 @@
 class ApplicationController < ActionController::Base
+  include ActionPolicy::Controller


Suggested change

include ActionPolicy::Controller

Shouldn't need this explicitly. allowed_to? was already working for me and the doc's say its included in ActionController::Base

Were you getting an error without it?

jmilljr24 · 2026-02-04T19:12:16Z

app/policies/application_policy.rb


  def index?
-    true
+    admin?


Good idea.

Can you add index? to the alias_rule manage?. This keeps them all set to admin? in one place and follows the convention from the Action Policy docs.

alias_rule :index?, :new?, :create?, :edit?, :update?, :destroy?, to: :manage?

jmilljr24 · 2026-02-04T19:16:48Z

app/policies/application_policy.rb


  def admin?
-    user&.super_user?
+    user&.admin?


Do you think it makes more sense to have the user permission checked here as this this the authorization framework? The method in User model works but is that the best place?

The hope is that any check we make for an admin? is via action_policy not directly off current_user

I agree only one place for sure if we can help it.

jmilljr24 · 2026-02-04T19:29:00Z

app/views/workshop_ideas/_form.html.erb


-<% 
-=begin %>
+<%# begin %>


Something was broken for me when I was fixing the workshop promotion.

jmilljr24 · 2026-02-04T19:31:47Z

app/views/workshop_ideas/_form.html.erb

-=end %>
+<%# =end %>
+
+    <div class="my-4"><%= render "assets/form", owner: workshop_idea %></div>


In html you are not supposed to have a form nested within a form. It's hard to see on github where the main form ends so I can't tell exactly where you moved it.

jmilljr24 · 2026-02-04T19:34:16Z

app/views/workshops/_form.html.erb

                  hint: "Any tips on how to make this workshop a success") %>
      <% end %>

+      <%= render "shared/form_dropdown", form: f, field: :extra_field, label: "Extra Field" do %>


The only reason I had this at the top was because of the order displayed on the show page. Extra Field is the first thing displayed. We should ask what order they expect.

i want to leave it at the bottom of the edit screen, and still display it at the top on show. then it's not their first instinct, AND, we still allow them to insert something above everything else?

That make sense. There are workshops where they literally have pages worth of content all in "extra_field" which doesn't make sense.

As long as they know where the field will be displayed that make sense to have it as a secondary option.

jmilljr24 · 2026-02-04T19:36:51Z

app/models/concerns/featureable.rb

  included do
-    scope :featured, -> { where(featured: true, inactive: false) }
-    scope :public_featured, -> { where(public_featured: true, public: true, inactive: false) }
+    scope :featured, -> { published.where(featured: true) }


Thanks for cleaning this up. I wanted to get most of the similar scope into a concern but the inactive vs published on different models side tracked me and this is where I left off.

i'm taking a second pass on all of these booleans and scopes and trying to normalize inactive/published

jmilljr24 · 2026-02-04T20:57:41Z

app/controllers/community_news_controller.rb

      per_page = params[:number_of_items_per_page].presence || 12
-      unfiltered = current_user.super_user? ? CommunityNews.all : CommunityNews.published
+      unfiltered =
+        if current_user&.admin?


Sorry I missed this.

To use the relation_scope in the policy you need to call authorized_scope(CommunityNews.all)

That will look in the Community News Policy for the relation_scope. authorized_scope allows other options like specifying which policy or a specific named scope withing a policy as well if you need it.

maebeale commented Feb 4, 2026

View reviewed changes

app/policies/ahoy_activity_policy.rb

admin?

end

def recent?

Copy link

Collaborator Author

maebeale Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not using this endpoint anymore

maebeale commented Feb 4, 2026

View reviewed changes

maebeale added 4 commits February 4, 2026 14:34

Update policies and tests to handle button normalization

5b68f5b

Update brakeman

8babacc

Remove commented out types

6db3210

Update public->publicly_visible

0dc9cfb

maebeale added 3 commits February 4, 2026 14:34

Change remaining references to

ca082c6

Rename public_featured to publicly_featured

b1d5b2d

Fix Resource factory kind designation

ae9b976

maebeale force-pushed the update-button-text branch from c9ab08f to ae9b976 Compare February 4, 2026 19:34

Add windows type policy

51ab32a

jmilljr24 approved these changes Feb 4, 2026

View reviewed changes

Keep the Upload Asset text for asset buttons

53c1dc6

maebeale marked this pull request as ready for review February 4, 2026 19:47

Consolidate manage permissions to admin

89d70ae

jmilljr24 reviewed Feb 4, 2026

View reviewed changes

maebeale added 2 commits February 4, 2026 16:38

Move asset form back to bottom of workshop_ideas form

3f20b19

Remove unnecessary include

643a7dc

maebeale merged commit a225923 into main Feb 5, 2026
3 checks passed

maebeale deleted the update-button-text branch February 5, 2026 02:56

		@@ -1,4 +1,5 @@
		class ApplicationController < ActionController::Base
		include ActionPolicy::Controller

		@@ -0,0 +1,6 @@
		class AddPubliclyVisibleToModels < ActiveRecord::Migration[8.1]
		def change

Update policies and tests to handle button normalization #844

Update policies and tests to handle button normalization #844

Uh oh!

Conversation

maebeale commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the goal of this PR and why is this important?

How did you approach the change?

Anything else to add?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maebeale commented Feb 4, 2026 •

edited

Loading