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

F/pundit page policy #453

Merged
merged 5 commits into from
Sep 6, 2023
Merged

F/pundit page policy #453

merged 5 commits into from
Sep 6, 2023

Conversation

aaguilarg
Copy link
Contributor

@aaguilarg aaguilarg commented Aug 29, 2023

Context

Potassium-generated applications with Pundit use that gem to allow access to certain resources. Sometimes we add roles to filter which pages can a user see on active admin, and if they don't have permission to see anything (not even the dashboard) the app enters on a redirect loop, leading into an exception.

Also, every Potassium-generated policy files come un-tested.

What has been done

Summary:
PagePolicy has been edited, now when de page is the Dashboard, is always shown to the user. Also, policy tests are now generated with Potassium.

Commits:

  1. Add Pundit shared examples, which will be useful for every policy spec created in this PR
  2. Add the "Always show dashboard" logic to the PagePolicy
  3. Add missing tests to the policies added in the pundit recipe
  4. Modify the pundit recipe and test that every file is copied correctly

Extra info

EDIT: what I describe below is now fixed, I added a force: true to the factory file copy, and now it works perfectly.

All the new generated tests fails on a new app, but this is because de AdminUser factory is created with empty attributes. If the email and password are added, every test pass.

FactoryBot.define do
  factory :admin_user do
    email { Faker::Internet.unique.email }
    password { 'password' }
  end
end

My doubt is: is it okay to leave it like that, with failing tests? I tried to modify the AdminUser factory file, but when the pundit recipe is ran, that file is not created yet. Also I tried to copy the AdminUser factory file (like the policies files are copied), but that generates a situation where user input is needed. This is because when the AdminUser factory file is trying to be created, it raises a warning that already exists, and user input is needed in order to decide to skip, abort, overwrite, etc.

@aaguilarg aaguilarg force-pushed the f/pundit-page-policy branch from 6e6aee3 to afc16a3 Compare August 30, 2023 16:40
@aaguilarg aaguilarg force-pushed the f/pundit-page-policy branch from afc16a3 to b2347fd Compare August 30, 2023 19:56
@aaguilarg aaguilarg requested a review from ldlsegovia August 30, 2023 20:16
@aaguilarg aaguilarg force-pushed the f/pundit-page-policy branch from faee38f to 398894a Compare September 6, 2023 15:34
@aaguilarg aaguilarg merged commit 9a62813 into master Sep 6, 2023
@aaguilarg aaguilarg deleted the f/pundit-page-policy branch September 6, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants