Schedule alert banners

[stephen-cox]

Closes #115

[stephen-cox]

There are going to be code deprecation failures until there's a release of the project with this merged in localgovdrupal/localgov_project#53

[andybroomfield]

Want to get #162 merged and a 1.1.2 release out before I approve this as this should be 1.2.0 release.

[stephen-cox]

Tests are in a pickle because we're supporting both1.x and 2.x versions of LocalGov. It's probably not the problem with this module, but has come to light with the latest 2 pull requests. I've attempted a fix but hasn't worked yet. Will return to this once I've given it some thought.

Maybe we don't want to be testing this module along with the distribution. Would it be better to test this module as a standalone module, not along with everything else in LocalGov Drupal? The profile would still be testing how it integrates with the rest of the modules.

[stephen-cox]

@andybroomfield Tests are now fixes so this is ready for a review when you have some time.

[andybroomfield]

As there is new config here to install (Workflow) and we can't rely on sites to be using something like config distro yet, we must provide an update hook here so the workflows are installed.

[andybroomfield]

Alert Banners are published using the 'Set banner live' and 'Remove banner' to publish and unpublish banners. I have hidden the content moderation controls so this can continue to be used as before. They way I did things means there's no difference to how people use alert banners.

@stephen-cox Yeah, I fiquired it was that still controlling the state. However it is possible to add the moderation control, which then does allow direct publishing. Maybe we should remove the set live option if the moderation controls are present as site builder might decide to just add it if it looks like it should be there and also want to add other states?

[stephen-cox]

Maybe we should remove the set live option if the moderation controls are present as site builder might decide to just add it if it looks like it should be there and also want to add other states?

This is a major change to how the alert banners work. I suggest it would be better to spin that out into a separate issue where it can be fully specced and discussed.

[ekes]

Seeing this error message when viewing an alert banners revisions

Error message
Warning: htmlspecialchars() expects parameter 1 to be string, object given in Drupal\Component\Utility\Html::escape() (line 424 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Utility\Html::escape(Object) (Line: 260)

...

I've recreated this in 1.x so have created a separate issue #169 and think this requires a fix PR.

[andybroomfield]

Link to wider issue on alternatives localgovdrupal/localgov#324

[andybroomfield]

I think it may be ok to pursue this method provided...
We commit to resolving the issues around the change to Workflow, as adding it to this entity implies that it is fully supported and right now it can cause breakages. I would also like to add UX at some point for setting the schedule on the banner.
I also think we should add a fix to hide the set live button if a site builder adds in the moderation form element as part of this PR if possible.
In future we should think about whether when the moderation form is present, and someone changes to publish state, if that should go to the confirm form.

I think we should aim to also drop the initial draft state, default them to the unpublish state. Its not possible to manually set that anyway so its a bit strange.

The terminolgy around alert banners is 'put live' and 'remove banner' and I think the states should follow that convention (or we change the labels to publish / unpublish... but thats a commns question).

[ekes]

@andybroomfield

I would also like to add UX at some point for setting the schedule on the banner.

I think this would be the ideal first candidate for having some simplified create, view transitions on the entity edit page.

With the Alert Banner example we know what the the 'publish' and 'unpublish' states that are expected are to be - agree it would be good to have workflow states and so changes with appropriate naming; we expect the primary use case to just be that. So a first implementation that on the entity edit form shows the upcoming "publish' / "unpublish" transitions; or allows creating them directly with just a date field (with a fallback that if there's something more complicated already configured it links to the transitions tab).

Building that for this (I'd suggest as another PR after this) would then give the foundations for something that could be used or developed further elsewhere in the site.

[ekes]

So with #159 (comment) fixed, and follow-up #159 (comment) suggested this patch seems good to me; but I think with issues #171 and #151 it looks like Alert Banner - with or without this PR - needs some more route / access tests?

[andybroomfield]

@ekes @stephen-cox Think we still need to add the config for workflow as an update hook to get this merged.
And if its not needed, remove the initial draft state.

... We need to resolve the other revisions issues being raised, and I think #171 is critical as you can't schedule a revision if you can't view it.

[stephen-cox]

@andybroomfield I have added an update hook to apply configuration to existing alert banner installs, so I think this is ready for another look.

[finnlewis]

@andybroomfield are you ready approve this now?

[andybroomfield]

@finnlewis we need to resolve #171 first, @stephen-cox has partial fix in #172 which I will look to extend.

[finnlewis]

@andybroomfield #171 is resolved and tests pass, so I think we're ready to merge this one, no?

[finnlewis]

Yay! Nice one @andybroomfield @stephen-cox @ekes !