Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused actions in controller filters #5535

Merged

Conversation

JoshDevHub
Copy link
Contributor

@JoshDevHub JoshDevHub commented Feb 17, 2024

What github issue is this PR for, if any?

Resolves #5502

What changed, and why?

  • In CaseContactsController, removed the update action (which isn't defined) from the only list for one of the callbacks
  • In CourtDatesController, removed the generate action (which isn't defined) from the only list for one of the callbacks

In Rails 7.1, they've added a new configuration to catch typo'd and undefined controller actions listed in the only or except lists to a controller filter. Relevant PR.

This configuration option is written by default in config/environments/test.rb and config/environments/development.rb. In the upgrade to Rails 7.1 in #5478, this configuration was disabled in order to pass the tests. By re-enabling it, we were able to catch a couple of places where unused actions were listed for a controller filter.

We are unable to set that configuration true because of two lingering issues:

  1. In ApplicationController, the #verify_authorized callback fires except for the :index action. This is an issue because ApplicationController is an abstract controller -- it offers no actions of its own. One way to fix this would be to fully apply #verify_authorized in the ApplicationController, and then, in places where this should be skipped, use skip_before_action :verify_authorized to do so.
  2. In Api::V1::BaseController, the #authorize_user! callback fires except for :create. This is the same issue as above, as this is an abstract controller that doesn't have its own actions. So it could be fixed in the same way.

Glad to pursue fully fixing those two remaining issues and enabling the raise_on_missing_callback_actions if that's desired. Or it could just be left disabled.

Edit: another option would be to use option 2 specified in the following issue: ackama/rails-template#507

How will this affect user permissions?

  • Volunteer permissions: N/A
  • Supervisor permissions: N/A
  • Admin permissions: N/A

How is this tested? (please write tests!) 💖💪

N/A

Screenshots please :)

N/A

Feelings gif (optional)

What gif best describes your feeling working on this issue? https://giphy.com/
train on rails

Feedback please? (optional)

We are very interested in your feedback! Please give us some :) https://forms.gle/1D5ACNgTs2u9gSdh9

Co-authored-by: Gustavo Valenzuela <gustavo.devvv@gmail.com>
@github-actions github-actions bot added the ruby Pull requests that update Ruby code label Feb 17, 2024
Copy link
Collaborator

@compwron compwron left a comment

Choose a reason for hiding this comment

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

seems good, tests pass

@compwron compwron merged commit c968736 into rubyforgood:main Mar 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code
Projects
None yet
2 participants