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

Support for new entity permissions form #219

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

andybroomfield
Copy link
Contributor

Ref #218

Add new annotation to get the alert banner permissions form on the manage alert banner type
entity.
Also changes the alert banner permissions generator to make sure are assigned to the alert
banner entity using BundlePermissionHandlerTrait

@andybroomfield andybroomfield marked this pull request as draft June 24, 2022 17:49
@andybroomfield andybroomfield changed the title Initial support for new permissions form Support for new entity permissions form Jun 24, 2022
@stephen-cox
Copy link
Member

Discussed at Merge Monday. Thinking was we can add a dependency to Drupal 9.4 and release it as 1.5.x

@finnlewis
Copy link
Member

Discussed in Merge Monday, @andybroomfield will reawaken this now that 9.4 is out and 9.3 is deprecated.

@finnlewis
Copy link
Member

Still on Andy's backlog, probably not get to this for another month at least.

@andybroomfield
Copy link
Contributor Author

Updated this PR dropping support for Drupal 8 and everything below Drupal 9.4 (core 9.4+ or 10).
This means adding drupal/core to composer.json so on the off chance there are sites still below 9.4 requiring this, they wont get the update.

Once #244 and #246 are merged, they will make up the final release of the 1.5.x series.
This will then form the first release of 1.6.x

Ref #218

Add new annotation to get the alert banner permissions form on the manage alert banner type
entity.
Also changes the alert banner permissions generator to make sure are assigned to the alert
banner entity using BundlePermissionHandlerTrait
As banner types now use path /admin/structure/alert-banner-types
Drops support for Drupal 8 and Drupal less than 9.4 which does not have
the BundlePermissionHandlerTrait.
@andybroomfield andybroomfield marked this pull request as ready for review May 15, 2023 17:27
@andybroomfield
Copy link
Contributor Author

andybroomfield commented May 15, 2023

This is ready for review now.
You'll now get a new manage permissions tab when editing an alert banner type under structure -> alert banners -> alert banner.

/admin/structure/alert-banner-types/localgov_alert_banner/permissions

Screenshot 2023-05-15 at 9 41 23 pm

Note: Emergency publisher doesn't need the manage permission, as they have the manage all alert banners permission.
Screenshot 2023-05-15 at 9 40 57 pm

Copy link
Member

@stephen-cox stephen-cox left a comment

Choose a reason for hiding this comment

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

This adds the permissions form, but I'm not sure if it's working as expected. If it is working it's a little confusing.

If I remove the 'View alert banner entities of type Alert banner' permission on the new permissions tab it has no effect because it's overridden by the global permission with the same name. I would expect the permission on a banner type to override the global permission when removing permissions.

@Adnan-cds
Copy link
Contributor

If it is working it's a little confusing.

I think it is working. But it is confusing indeed :D

@Adnan-cds
Copy link
Contributor

Code looks good to me. But we really need to be sure if this is working or not.

@andybroomfield
Copy link
Contributor Author

andybroomfield commented May 22, 2023

Yes, thats expected behaviour (global permission overrides per entity type), as the view all are assigned to anon user and auth user, and manage to emergrency publisher.
I did try and see if it was possible to add some explinationary copy about this, but it doesn't seem possible to target this specific instance of the permissions form.

We might consider actully removing the global permissions, that would require update hook to re-assign them.
I note that we are in fact adding permissions on the entity type when created so it wouldn't be much to extend that for the manage permissions.

My only counter to this is that some teams may have set up a different role and granted a global permission.

@finnlewis
Copy link
Member

Could do with testing to confirm that the sub-set of permissions page works as expected.

I'm happy to test further! :)

@andybroomfield
Copy link
Contributor Author

Would like to pick this up again.
Are we ok with a plan of:

  • Merge this so the new permissions tab is present (Support a new Drupal feature)
  • Do some discovery on the permission model, potentially removing the global permissions

We could then look at supporting the new permissions tab on other custom entities such as geo.

@finnlewis finnlewis self-assigned this Jul 3, 2023
@stephen-cox stephen-cox self-requested a review July 3, 2023 13:16
@finnlewis
Copy link
Member

@stephen-cox also happy with this approach... so I'll let you confirm before / if testing further.

Copy link
Member

@stephen-cox stephen-cox left a comment

Choose a reason for hiding this comment

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

This is fine. Happy to merge this and rationalise the permissions in another ticket.

@andybroomfield andybroomfield merged commit a65719d into 1.x Jul 6, 2023
@andybroomfield andybroomfield deleted the feature/218-permissions-tab branch July 6, 2023 08:47
@andybroomfield andybroomfield mentioned this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants