Conditional alert banner placement

[stephen-cox]

Closes #137

[stephen-cox]

This is now ready to be reviewed.

[andybroomfield]

I've checked out this update.
The condition field is added to the alert banner and the upgrade works with the current alert banners.

A couple of questions:
This visibility condition field is only added to the primary localgov_alert_banner type. I had another alert banner type created and that did not get the visibility condition added. I think we should try to get it added to all existing banner types as its kind of an expected feature, and to get this to work you have to add the specific visibility field.

Also it is showing the content type and user role contexts, are we planning on restricting the plugins initially in use?
(This is on upgrading the install)

[willguv]

@andybroomfield agree that the conditionality ought to work out of the box for all banner types

On the alerts work, @stephen-cox hid/ removed content type and user role controls so could you do the same here?

Thanks both

[andybroomfield]

I think it is hidden in the config so a fresh install won't show them, this was testing an upgrade with existing alert banners.

[stephen-cox]

@andybroomfield I have updated the code so the visibility field should be added to all existing alert banner bundles.

Only the pages visibility plugin should be added to existing alert banners. Can you check whether the condition_field module was patched with the latest patch from here: https://www.drupal.org/project/condition_field/issues/3215202?

If the patch is not installed then the condition field doesn't work properly with the provided configuration.

[andybroomfield]

@stephen-cox Ok thanks. I've applied the patch and re-run the update. This is working as expected with adding the condition field to all alert banner types and only the page plugin is enabled by default.
It would be good to increase the weight so its the last element in the form. I had a test field that was displaying below it when added.

One bug I have encounterd. When logged in as admin and setting up a page restriction, the banner only displays once on the restricted page. Then visit a page where it should not show and go the the page it shold show. Refreshing that page and the banner goes away. This is only logged in users (presume cache for anon).

I think this could be cache issue as then flushing cache brings it back. So the route of the page needs to be a cache context for the banner or the block?

[andybroomfield]

Just to confirm, this is broken with cache. Disbling the cache backends and it works as expected. With caches enabled, going to a page with a restricted banner, then going to a page without a banner, and then revisiting a page where there should be a banner, the banner will not be present. Flush cache then makes it appear (and appears on all pages).

[stephen-cox]

@andybroomfield The latest changes should fix the caching issues. They represent quite an overhaul of how the visibility conditions are evaluated, along with changes to the alert banner block.

Are alert banners only currently displayed to end users through the alter banner block? If so then the changes should be fine. If there are other ways of displaying them they will need to be updated to use the isVisible() method on an AlertBannerEntity to check the alert banner is okay to display.

[andybroomfield]

Thanks @stephen-cox will look.

Yes at present it is just the block for end users.
There are places where people can view the entity if they are a an editor to preview. Assume this won't effect that.

I think it is safe to document that if someone is using them outside a block, it is on them to check the isVisible() condition. The main use case I can think of is if someone built a view instead, so maybe a views filter plugin in the future.

[andybroomfield]

I have just checked this and it works with cache on.
Happy for this to go forward for release.

As new feature, this is a v1.1.0?

[stephen-cox]

Thanks @andybroomfield. Merged. I'll create a PR for the new release.