Feature: Group integration #250
Conversation
Provides: - A Group plugin for Alert banners. - A View for listing Alert banners belonging to groups. - User permissions to go along with the localgov_microsites_group module.
- Fixed Alert banner's entity *type* name during config dependency calculation. - Added group relation type cached clear during Alert banner bundle creation. This is needed for config import to work.
Automated test for banner allocation to a test group bundle.
The suggested dependencies (e.g. group) from composer.json are required to run some of the tests.
Fix for composer command in test runner to grab suggested packages.
|
Mostly lifted from the gnode submodule of the group module. |
| access localgov_alert_banner overview: | ||
| title: 'Access Alert banner listing page' | ||
| description: 'Access the Alert banner listing and management page, regardless of banner type.' |
There was a problem hiding this comment.
This sounds like the kind of general permission we should have in the main module, how are we access controlling it at the moment? Why do we need the seperate permission?
There was a problem hiding this comment.
So I think I'm understanding that this is for the seperate group view, which is different from the main admin view. And we only want to grand group insiders the group admin view and not the main one?
(This is me trying to understand localgov microsites permissions)
There was a problem hiding this comment.
And we only want to grant group insiders the group admin view and not the main one?
That's right.
|
As this is for microsites I'm happy to leave the review to those using it. |
| If you are using this module alongside the [localgov_microsites_group](https://github.com/localgovdrupal/localgov_microsites_group) module, then user permissions will be automatically setup during module installation. | ||
|
|
||
| Otherwise enable at least the following permissions for **Groups**: | ||
| - "Entity: View any alert banner entities" for both anonymous and authenticated users. |
|
My very quick run through within the localgov microsites distrubtion shows that this works well, thanks @Adnan-cds. Permissions are something we need to look at, just to make sure we're not granting a bit more than we should. I'm also noticing that when I click set live when creating a new alert banner, I'm taken to the view alert page, not the confirm form. |
There was a problem hiding this comment.
This largely works fine. A couple of minor issues:
- As Andy points out, when adding alert the set live option doesn't work.
- When viewing the list of alert banners to the banner type is empty; maybe this isn't needed.
Just to note, that this branch is rather out of data and alert banners weren't displaying until I merged the latest changes in the 1.x branch.
If the add alert banner form redirects to the site live form when the set live field is selected then I think this is fine to merge.
When a new banner is set to go live, we should end up in the "Put banner live" tab of the new banner after the banner form is submitted. At the moment we are ending up in the banner's "View" tab. Also, group banners should only appear in group pages and not all over the site.
Decide the active group either based on the current path or the current domain. This is useful where we are using the domain_group module to associate a domain with a group.
Once a group banner is published, redirect to the group banner listing page instead of to the site-wide banner listing page.
This is done now.
This is fixed as well. The test runs are resulting in an unrelated test failure. Is this a known issue? |
|
@Adnan-cds would like @andybroomfield to review before merging. @andybroomfield would like @stephen-cox to review before merging! 😄 |
|
@stephen-cox - might you have time to review this one? @finnlewis can you test again? and @millnut - if you get bored? 😄 |
There was a problem hiding this comment.
Working well for me. Happy to approve.
One things to note is to get alert banners to display I had to add the alert banner block to the themes header region. I suggest adding optional block config for this to the Microsites Base theme as we have for other blocks: https://github.com/localgovdrupal/localgov_microsites_base/tree/2.x/config/install
That's a good point that I missed. I remember having to manually add this block in our microsite instance. How about adding it here in the group_alert_banner submodule as an optional block config? |
That would do the trick |
Optional Alert banner block configuration for the localgov_microsites_base theme. Automates setup of the Alert banner block for LocalGov Drupal Microsites distribution users.
|
I have added an optional config file to add the Alert banner block in the localgov_microsites_base theme. I have also explained the block placement issue in the README. Please let me know if this will do. |
|
Looks fine to me 👍 |
|
Great :) Merging then... |
Provides: