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 Struct inheritance from headless policy example #717

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Remove Struct inheritance from headless policy example #717

merged 1 commit into from
Feb 11, 2022

Conversation

drewmoore
Copy link
Contributor

This addresses #696. Rubocop advises against inheriting from Struct, and my team found that we could simply remove this and achieve the same results. Thanks for your review.

@@ -194,7 +194,7 @@ you can retrieve it by passing a symbol.

```ruby
# app/policies/dashboard_policy.rb
class DashboardPolicy < Struct.new(:user, :dashboard)
class DashboardPolicy
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is enough: to me this suggests that just defining an empty class with a Policy suffix is enough, while in reality we need a very specific initialiser and accessors to be defined.

How would you feel about explicitly writing out the initialiser and accessors as part of this documentation?

Presumably the motivation behind using Struct was as a shorthand for that (but I do see why it doesn't work).

Alternatively maybe it's worth making a HeadlessPolicy class to help avoid this repeated boilerplate 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to use the other Struct approach, which Rubocop is OK with?

DashboardPolicy = Struct.new(:user, :dashboard) do
  # ...
end

Copy link
Contributor Author

@drewmoore drewmoore Jan 24, 2022

Choose a reason for hiding this comment

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

True, I might also have come to that conclusion. Would this help to clarify? d17ca6c. As far as having an inherited class for headless policies, we actually just inherit from the same base policy class that all of our other policies use. The only difference is that the 2nd parameter of the initializer will just be the symbol passed to authorize, in this case :dashboard. Our working implementation is actually quite minimal.

README.md Outdated
Comment on lines 203 to 212
second argument will just be the symbol `:dashboard` in this case which
first argument will just be the symbol `:dashboard` in this case which
Copy link
Member

@Burgestrand Burgestrand Jan 25, 2022

Choose a reason for hiding this comment

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

The _record is :dashboard, which is still the second argument :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the first argument provided to authorize in this case would be :dashboard, whereas the second parameter in the policy's initializer method would in this case be :dashboard.

Copy link
Member

@Burgestrand Burgestrand Jan 25, 2022

Choose a reason for hiding this comment

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

Yup! The passage does talk about the headless policy, though?

Note that the headless policy still needs to accept two arguments. The
second argument will just be the symbol :dashboard in this case which […]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an attempt to fix an unrelated issue?

In this case the authorize method accepts only one argument, while the policy initializer accepts two. This line is pretty clearly talking about the policy initializer since the line before says:

Note that the headless policy still needs to accept two arguments.

Maybe this area of the documentation could be clarified, but that feels completely independent of the change to remove Struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed: 67e0dd3

@Burgestrand
Copy link
Member

Thank you!

@Burgestrand Burgestrand merged commit 54675c6 into varvet:main Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants