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

1.8.x Refactor #338

Open
wants to merge 43 commits into
base: 1.x
Choose a base branch
from
Open

1.8.x Refactor #338

wants to merge 43 commits into from

Conversation

andybroomfield
Copy link
Contributor

@andybroomfield andybroomfield commented Jun 25, 2024

Current blockers

Propose we handle in patch release as needs more discussion

Don't know

millnut and others added 11 commits June 17, 2024 19:16
…-type-fixes

fix: additional strict type fixes
Fix #323

Refactor the alert banner entitiy to use a more standard approach to revisions,
using \Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider
instead of custom routes, and using standard revsion routes instead of custom
versions in routing.yml.

This also groups all the none view crud permissions into the manage alert
banner permission in alertBannerEntityAccessControlHandler.
Normally not required as the view is in place, but sometimes needed if the
view is disabled.
Note: this may still get removed later in the refactor, but for now restore
route admin/structure/alert-banner-types
Ref #334

Flaged as deprecated because of the {% spaceless %} tag, which should have
been removed with the move to Drupal 9?
However this template is simply not used, so removing.
Ref #334

Remove view default_argument_skip_url config key which is being removed.
See https://www.drupal.org/node/3382316
Fix #261

Use a new AlertBannerManager service and move the getCurentBanners method from the
block to the manager service.

Adds an $options array for setting the type and future options.
Small refactor to use loadMultiple.
…ctor

Initial refactor of revision provider and access
…anager

Move getCurrentBanners to a service
@andybroomfield andybroomfield marked this pull request as draft June 25, 2024 12:25
stephen-cox and others added 12 commits June 25, 2024 13:30
…fault-argument

Remove default_argument_skip_url from group manage alert banners view
Fix #229

Implements hook_gin_content_form_routes for both the alert banner creation
and edit form. This makes editing the alert banner like nodes.

Renames the publishing options group meta to auto pick up gin theming.
Moves the revision log and new revision into the meta group
Add a flag if this is the Gin theme, if it is add the extra class to meta
so it is themed like nodes.
Fix #342

Use the standard drupal paths (entity_type/manage) for the alert banner
entity type routes, and add a local task for the edit link.
This restores expected behaviour with edit, fields, manage form,
manage display,  permissions.

Remove unneed entity type route provider and remove todo comment
from entity type list builder.
…theme

Opt into the Gin content form routes (edit theme)
…entity-type

Fix alert banner entity type paths and local tasks
…ermissions-test

Fix routes to localgov_alert_banner_types in permissions test
Fix #271

Small refactor to remove the js-cookie dependancy by using native javascript
cookie read and write methods.
@andybroomfield
Copy link
Contributor Author

Suggest re rename this branch becuase having a 1.8.x branch seems to be the root of the test failures.
See localgovdrupal/localgov_shared_workflows#3

@andybroomfield
Copy link
Contributor Author

andybroomfield commented Jul 10, 2024

and yet renaming this branch causes this one to pass?

Adnan-cds and others added 4 commits July 30, 2024 12:05
…#362)

* Fix the contrast on the type of alert text on full page alert banners

Fix #361

Use color: inherit instead of overriding with a value

* Remove a and p tags from full page alert colour inherit

These are providing redundent overrides so can be removed
* Mark support for Drupal 11 and drop Drupal 9

In main module only.

* Mark localgov alert banner full page as Drupal 11 ready

js-cookie dependancy removed.

* Mark group_alert_banner as supported on Drupal 11

* Update composer.json for Drupal 11

- Set core version as 10 or 11.
- Amend the suggestion for localgov_core
millnut and others added 10 commits August 18, 2024 12:52
* Add access toolbar, view administation theme perms to emergeny publisher role

* Add access content overview permission

This is so the alert banners menu option is avalible in the toolbar

* Remove access administration pages permission from emergency publisher

This permission isn't nessecary to access these pages.

* Add update hook to grant these permissions

* cs: install updates fix

* Refactor how some permissions all granted

Use a function to install default permissions on install for
emergency publisher based on if node or toolbar is installed.

Also move the default anon / authenticated users to the
set_default_permissions() function.

Also only proceed during first install, not on config sync operations.

* Add the missing roleInterface
* Fix todo in admin view functional test to move the admin user creation into the setup function.

* Add kernel test to confirm that the admin url for the alert banners is the url set on the page display in the view.

* Remove todo comments and redundant line of code.

* Tweak comment

* Update function comment

* Fix coding standards.
…ository) (#369)

* Deprecate old constructor signiture of alert banner block

This may not be totally nescerry, but to be on the safe side in case the block has been
extended, this deprecates the old signiture with current_user and entity.repository
and marks alert banner manager as required.

* Add the deprecatedProperttTrait to the alert banner block

Allows block extensions to still have a currentUser and entityRepository
Fix #247

This provides the details in the readme, will open a new issue for the config
option.
See #327.

Extends visibility test by providing actual pages for the banner to display on.
This also provides an extra banner to test the correct banner is visible per page.
…ble-check

Move the visible check to alert banner manager
@andybroomfield andybroomfield marked this pull request as ready for review September 16, 2024 20:57
@andybroomfield andybroomfield changed the title 1.8.x 1.8.x Refactor Sep 16, 2024
@andybroomfield
Copy link
Contributor Author

List of PRs that make up the refactor.

@andybroomfield
Copy link
Contributor Author

What does this change?

Refactors the Alert banner entity, alert banner block and adds an alert banner manager service.
Adds Drupal 11 support and drops Drupal 9 support.

How to test

  • Check no more Drupal 11 deprecations
  • Check installable on Drupal 11.
  • Check alert banners can be created, viewed, dismissed.

How can we measure success?

Alert banner should function as before, now with Drupal 11 support and the deperacation warnings should be gone.
This release does not contan functional changes.

Have we considered potential risks?

Check that refactor does not have any side effects.

Images

n/a

Accessibility

n/a (follow up PRs to be added in 1.8.1)

@finnlewis finnlewis self-requested a review September 17, 2024 11:21
millnut and others added 2 commits September 23, 2024 21:18
…s-from-drupal-coder-update

fix: coding standards introduced by latest drupal coder rules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants