Feature/group sites #438

ekes · 2024-01-16T13:54:56Z

Switches to use group_sites in place of domain_group; and removes dependency on group_permissions switching to controlling access on routes per domain for creating content.

…pes.

…crosite.

Tests now use domains: group-0.{base domain} group-1.{base domain} group-2.{base domain} group-3.{base domain} in place of the previous a1 b1 etc.

Fix admin mode to be on in the control domain at all times.

…tall and uninstall modules)

ekes · 2024-01-16T14:01:56Z

To test this probably easiest to checkout the feature/group_sites branch of localgovdrupal/localgov_microsites_project which will then install the appropriate branches down via localgovdrupal/localgov_microsites#feature/group_sites and it's composer.json

More details on upgrade testing on the issue #429 (comment)

finnlewis · 2024-01-17T17:19:25Z

My steps to test:

composer create-project --stability dev localgovdrupal/localgov_microsites_project:dev-feature/group_sites MY_PROJECT  --no-install; 
cd MY_PROJECT
ddev start
ddev composer install
ddev drush si localgov_microsites -y

The install goes fine, but I am having problems with permissions.

log in as user 1
create controller user with microsites controller role
log in as controller
create microsite at https://localgov-microsites.ddev.site/admin/microsites/add/microsite
note that the default owner is the controller user
try to edit the microsite
403 access denied at https://localgov-microsites.ddev.site/group/3/members or https://localgov-microsites.ddev.site/group/3/edit

It is odd, as I can see the buttons to edit, manage members etc...

#438 (comment)

ekes · 2024-01-20T13:56:33Z

So my hunch that it was something I'd done right at the end was correct. It was my attempt to make the control site permanently in 'group_sites admin mode'. Seems it was reporting that it is in admin mode in the toolbar, a brief check for access control in the debugger also saw it confirming admin mode, but somewhere, I've not found, it was returning an access denied because it wasn't it seems.

This means with your site as it stands above you should be able to go to the domain itself, login, and administer the microsite from there.

To try and fix this I've now pushed a change that from cursory testing seemed to fix it for me 9c975ba

But if that doesn't work for you - quite possible it's just a punt that seems to work - to temporarily disable this all and test everything else:

comment out the line further down in that file

localgov_microsites_group/src/EventSubscriber/SiteAdminMode.php

Line 41 in 9c975ba

$this->adminMode->setAdminModeOverride(TRUE);
then the 'Group sites: Admin Mode OFF/ON' button in the admin menu at the top will become toggleable, you need it to be ON.

finnlewis · 2024-01-26T14:35:57Z

Thanks @ekes - the install is working for me now since that commit 9c975ba

If anyone else wants to test this branch, this sets it up quickly for me:

composer create-project --stability dev localgovdrupal/localgov_microsites_project:dev-feature/group_sites MY_PROJECT  --no-install; 
cd MY_PROJECT
ddev start
ddev composer install
ddev drush si localgov_microsites -y
ddev drush uli

Adnan-cds · 2024-01-26T15:57:49Z

I am finally on it. Hopefully should be able to complete the review before Tuesday's Merge Monday meeting :D

finnlewis · 2024-02-01T15:07:58Z

@nccchris it would be great if you could test this against your multiple microsites.

finnlewis · 2024-02-01T15:22:30Z

@ekes mentioned : on #429

The upgrade path isn't quite there yet. It works, but you need to get the group_sites and group_context_domain enabled before switching to the branch, and you need to have the domain_group and domain_permissions modules there long enough to disable them after switching. I also got an error from localgov_search block, but that might be separate.

So Potential steps to test an upgrade: (this needs testing / confirming)

Assuming we have an existing installation on Drupal 10, (the 3.x branch of https://github.com/localgovdrupal/localgov_microsites)

ddev drush pm:uninstall localgov_microsites_permissions
ddev composer require drupal/group_sites drupal/group_context_domain 
ddev composer require drupal/geofield_map
ddev drush en group_sites group_context_domain 
composer require "localgovdrupal/localgov_microsites:dev-feature/group_sites as 2.1.0-beta6" -W 
composer require drupal/domain_group drupal/group_permissions
drush updb
drush cr
composer remove drupal/domain_group drupal/domain_permissions

nccchris · 2024-02-05T13:35:02Z

First of all there's a small typo :
ddev composer require geofield_map should be ddev composer require drupal/geofield_map

I'm probably doing something silly, but I hit a couple of problems.

At composer require "localgovdrupal/localgov_microsites:dev-feature/group_sites as 2.1.0-beta6" -W
I get a warning:

drupal/group_permissions has modified files: 
M src/Plugin/Validation/Constraint/UniqueReferenceFieldValidator.php 
Discard changes [y,n,v,d,?]?"

'Y' allows me to proceed, but then at drush updb I hit an error :

[error]  The installed version of the /LocalGov Microsites Group/ module is too old to
update. Update to an intermediate version first (last removed version: 9003,
installed version: 9002).

ekes · 2024-02-06T10:49:06Z

@nccchris I'm making a first guess here, but is it possible that the site you're upgrading hadn't yet fully upgraded / run updb localgov_microsites_group before updating the code.

finnlewis · 2024-02-06T12:59:29Z

Note: on my local testing, I can now manage the sites on the control site as a controller.

But I cannot see any content on each microsite, I just get access denied.

I will re-test from scratch and report steps to reproduce.

Adnan-cds · 2024-02-07T18:27:52Z

My steps to test:

Thanks for the test steps Finn. All works good on a fresh install. I will now try the upgrade path.

Drupal logs are getting flooded with PHP 8.2 deprecation notices from the autosave_form module. A patch is available which we may want.

Adnan-cds

Some code was move directly out of domain_group into localgov_microsites_group

If these can be marked as such, that may help for future reference.

Adnan-cds · 2024-02-07T18:44:15Z

composer.json

+        "localgovdrupal/localgov_events": "For events content type in microsites"
+    },
+    "extra": {
+        "enable-patching": true,


As far as I remember, enable-patching is only relevant for root level composer.json files.

We're doing this all over localgov. This is possibly something that could go into clear up with localgovdrupal/localgov#597

Can maybe remove it here already, needs checking though, I think there's some unexpected buggyness with patching from dependencies already.

Adnan-cds · 2024-02-07T18:50:19Z

localgov_microsites_group.install

+  // Group sites should be installed aleady. It provides configuration, so we
+  // can't, but must update its default config.
+  $group_sites = \Drupal::configFactory()->getEditable('group_sites.settings');
+  $group_sites->set('context_provider', '@group_context_domain.group_from_domain_context:group');


Not sure if this is relevant. But this is what I see at /admin/group/sites/settings:

Not related to the install line. That was actually missing from the upgrade, so nice to have it highlighted. The dupe plugin is because one is deprecated. Changing the title of that so it now shows

Adnan-cds · 2024-02-08T10:49:51Z

modules/localgov_microsites_group_term_ui/src/Controller/GroupTermUiController.php

+   *   The access result.
+   */
+  protected function accessCreateGroupTerm(AccountInterface $account, GroupInterface $group, string $plugin_id) {
+    $plugin_manager = \Drupal::service('group_relation_type.manager');


Perhaps this can also be initialized in GroupTermUiController::create().

Adnan-cds · 2024-02-08T10:54:55Z

localgov_microsites_group.services.yml

+    arguments: ['@domain.negotiator', '@group_sites.admin_mode']
+    tags:
+      - { name: event_subscriber }
+


It will be good to have some comments here clarifying the domain_group module as the original source of these services.

Adnan-cds · 2024-02-08T10:55:49Z

localgov_microsites_group.routing.yml

@@ -15,3 +15,14 @@ localgov_microsites_group.microsite_admin:
    _controller: '\Drupal\localgov_microsites_group\Controller\MicrositeAdminController::redirectToMicrositeAdmin'
  requirements:
    _custom_access: '\Drupal\localgov_microsites_group\Controller\MicrositeAdminController::access'
+


It will be good to have some comments here clarifying the domain_group module as the original source of this route.

Any particular reason? There's nothing funky with the namespacing or so in this particular case? And it does now become just how LocalGov Microsites works (or rather has its admin interface).

Adnan-cds · 2024-02-08T12:17:57Z

src/Controller/DomainGroupAddController.php

-      $form = $this->formBuilder()->getForm(DomainGroupConfigAdd::class, $group, $extra);
+      // Only create a new domain if we have nothing stored.
+      if (!$domain = $store->get("$store_id:domain")) {
+        $values['type'] = $group_type->id();


Do we really need this?

Ah! You mean specifically line 133, yer that's a c&p isn't it. 180dc1e

Adnan-cds · 2024-02-08T12:22:26Z

src/DomainGroupResolver.php

+
+    if (!empty($groups)) {
+      foreach ($groups as $group) {
+        if ($group) {


Perhaps if ($group implements GroupInterface) { would be better.

Guessing checking instanceof would be consistent with the method. 9cfc24f

Adnan-cds · 2024-02-08T12:37:26Z

src/GroupDefaultContent.php

@@ -72,6 +62,8 @@ public function generate(GroupInterface $group): ?NodeInterface {
      $plugin_id = 'group_node:' . $default->bundle();
      if ($group->getGroupType()->hasPlugin($plugin_id)) {
        $node = $this->replicator->replicateEntity($default);
+        assert($node instanceof Node);


assert($node instanceof NodeInterface); perhaps?

Yep. 735c4f7

Adnan-cds · 2024-02-08T13:04:14Z

tests/src/Functional/ModuleSettingsFormTest.php

@@ -124,6 +125,7 @@ public function testGroupManagementAccess() {

    // Test member access.


Do group members need access to the Group Domain settings page?

Ah the difference between the two users here is that one is a member with no role but permission to 'use group_sites_admin_mode' permission only, and the other has a 'microsites-admin' role.

Adnan-cds · 2024-02-08T13:09:51Z

tests/src/Traits/InitializeGroupsTrait.php

+/*
+    // Creating domain site settings.
+    foreach ($this->allTestGroups as $group) {
+      $domain_id = 'group_' . $group->id();
+      $config_id = 'domain.config.' . $domain_id . '.system.site';
+      $config = $this->getConfigFactory()->getEditable($config_id);
+      $config->set('name', $group->label());
+      $config->set('slogan', $group->label() . ' Slogan');
+      $config->set('mail', 'group-' . $group->id() . '@user.com');
+      $config->set('page.front', '/group/' . $group->id());
+      $config->set('page.403', '/denied');
+      $config->set('page.404', '/not-found');
+    }
+    $config->save();
+*/


Please ⌫ :)

finnlewis · 2024-02-08T15:46:22Z

I tested upgrading our example sites from https://demo.microsites.localgovdrupal.org/

I copied the db down locally, installed, then ran the upgrade.

After disabling localgov_search_block, and updaing the domains to match the local test domains, I am seeing 'access denied' on all content on all microsites.

Even logging in as a 'LocalGov Admin' to site 1, creating a new node (event node in my case) gives access denied to both the logged in 'LocalGov Admin' user and anonymous.

@stephen-cox has just replicated the same behaviour on a fresh install of https://github.com/localgovdrupal/localgov_microsites/releases/tag/3.0.0-alpha1, creating a single site, then flipping to this branch and running the updgrade.

@Adnan-cds have you tried your upgrade yet and if so, do you get similar results?

@ekes is aware.... sorry it doesn't just work @ekes !

Adnan-cds · 2024-02-08T15:48:59Z

have you tried your upgrade yet and if so, do you get similar results?

I am at it now. Hoping to get a clear idea by the end of the day.

Adnan-cds · 2024-02-08T19:16:49Z

Sorry Finn, still working on it. I am having to update our site to Drupal 10 first! That's taking sometime.

ekes · 2024-02-13T11:43:01Z

I tested upgrading our example sites from https://demo.microsites.localgovdrupal.org/
...
Even logging in as a 'LocalGov Admin' to site 1, creating a new node (event node in my case) gives access denied to both the logged in 'LocalGov Admin' user and anonymous.

@stephen-cox has just replicated the same behaviour on a fresh install of https://github.com/localgovdrupal/localgov_microsites/releases/tag/3.0.0-alpha1, creating a single site, then flipping to this branch and running the updgrade.
...
@ekes is aware.... sorry it doesn't just work @ekes !

Ah! The upgrade has 'Group from URL' ticked and needs changing to the (correct) group from Domain

Which is being done on install as noted by Adnan #438 (comment) We are of course not installing here. The dupe is there for another reason (the second comes from the legacy context supplied by group domain - I think this to remain just in case people are using it - but needs re-labeling to make clear it is deprecated - this is already set so in the code).

Fixes on the way, and running through the issues Adnan raises.

ekes · 2024-02-13T13:51:53Z

After disabling localgov_search_block,

Are the search indexes getting lost. Why are the search indexes getting lost. We're not doing anything to them are we? 😕

#438 (comment)

Just resaving them while both modules domain_group and the upgraded localgov_microsites_group seems good enough.

ekes · 2024-02-13T16:26:02Z

After disabling localgov_search_block,

Are the search indexes getting lost. Why are the search indexes getting lost. We're not doing anything to them are we? 😕

It'll be because they depend on group_domain because of the plugin (that moves into localgov_microsites_group).

Need to update all the dependencies in config and this seems to do it. ad44bea

ekes · 2024-02-13T16:27:32Z

OK. I'd say that's ready for review again @stephen-cox @nccchris Oh! And @Adnan-cds I think this might upgrade directly from 2.x you know, it seems I'd forked it from there not 3.x! Tests just done were from 3.x though.

Adnan-cds · 2024-02-15T18:44:35Z

I was following Finn's update instructions given above. This is what happened at the drush updb stage:

------------- ----------- --------------- -----------------------------------
  Module        Update ID   Type            Description
 ------------- ----------- --------------- -----------------------------------
  localgov_mi   9004        hook_update_n   9004 - Migrate microsite domains
  crosites_gr                               to group_context_domain from
  oup                                       domain_group.
  localgov_mi   9005        hook_update_n   9005 - Update configuration
  crosites_gr                               dependencies.
  oup
  localgov_mi   9006        hook_update_n   9006 - Disable domain_group and
  crosites_gr                               group_permissions.
  oup
  localgov_mi   9007        hook_update_n   9007 - Set group sites context as
  crosites_gr                               would be done on install.
  oup
 ------------- ----------- --------------- -----------------------------------


 Do you wish to run the specified pending updates? (yes/no) [yes]:
 >

>  [notice] Update started: localgov_microsites_group_update_9004
>  [notice] Update completed: localgov_microsites_group_update_9004
>  [notice] Update started: localgov_microsites_group_update_9005
>  [notice] Update completed: localgov_microsites_group_update_9005
>  [notice] Update started: localgov_microsites_group_update_9006
>  [error]  The following reasons prevent the modules from being uninstalled: There is content for the entity type: Group permission. <a href="/admin/modules/uninstall/entity/group_permission">Remove group permission entities</a>.
>  [error]  Update failed: localgov_microsites_group_update_9006
 [error]  Update aborted by: localgov_microsites_group_update_9006 
 [error]  Finished performing updates.

This is on a site using localgovdrupal/localgov_microsites:3.0.0-alpha1 with more than one existing microsite.

#438 (comment)

ekes · 2024-02-20T11:04:56Z

I was following Finn's update instructions given above. This is what happened at the drush updb stage:
...
>  [notice] Update started: localgov_microsites_group_update_9006
>  [error]  The following reasons prevent the modules from being uninstalled: There is content for the entity type: Group permission. <a href="/admin/modules/uninstall/entity/group_permission">Remove group permission entities</a>.
This is on a site using localgovdrupal/localgov_microsites:3.0.0-alpha1 with more than one existing microsite.

Guessing 700cf1a should fix that for you?

stephen-cox · 2024-02-20T11:45:35Z

If I do an initial install from #439, create a microsite and run through the upgrade instructions, the upgrade works without any errors.

We do need to make the upgrade process as painless as possible.

Adnan-cds · 2024-02-20T16:51:43Z

I can confirm that following Finn's update instructions results in a successful update and microsites appear to be okay on the updated instance.

I have noticed just one error message in the Drupal log: RuntimeException while trying to render item entity:node/NNN:en with view mode search_index for search index Sitewide search: Failed to start the session because headers have already been sent by "/path/to/codebase/vendor/symfony/http-foundation/Response.php" at line 1317. in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() (line 132 of /path/to/codebase/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php). Not sure if output buffering, which we had to activate, has anything to do with this.

finnlewis

Testing has been going well and we discussed this in our tech group meeting yesterday. The general agreement to merge this and tag a 4.x-alpha release to gain visibility and make further testing easier. Nice work on this so far everyone, thank you!

ekes added 9 commits December 27, 2023 15:52

WIP

7205c88

WIP: Working except pre-domain (permissions) / en-dis-able content ty…

70c55ae

…pes.

Alternative system to enable/disable modules and content types per mi…

16a3279

…crosite.

Fix tests, and related bugs.

8c80dd0

Tests now use domains: group-0.{base domain} group-1.{base domain} group-2.{base domain} group-3.{base domain} in place of the previous a1 b1 etc.

Need group_sites for additional (controller) permission.

40e18be

Fix search_api processor.

f8ae6e6

First stab at doing something with the admin mode.

3d6843c

Fix admin mode to be on in the control domain at all times.

Correct redirect at end of microsite creation.

03ccb95

A slightly imperfect upgrade path (need to work out how to handle ins…

83723a2

…tall and uninstall modules)

ekes requested a review from finnlewis January 16, 2024 14:02

Set group_sites admin mode really early, in a hope to catch all access.

9c975ba

#438 (comment)

finnlewis requested a review from Adnan-cds January 23, 2024 12:46

stephen-cox self-requested a review January 23, 2024 12:46

finnlewis requested a review from nccchris February 1, 2024 15:07

Adnan-cds reviewed Feb 8, 2024

View reviewed changes

finnlewis mentioned this pull request Feb 8, 2024

Check index is loaded before checking index->status localgovdrupal/localgov_search#70

Open

ekes mentioned this pull request Feb 13, 2024

Feature/update with 2.x #439

Merged

ekes added 12 commits February 13, 2024 15:08

Clarify deprecated version of the context plugin.

0e3f4f9

#438 (comment)

Use injected GroupRelationTypeManager. Resolves #438 (comment)

228f194

Tidy namespaced RolesHelper. #438 (comment)

f714308

Possibly better wording for constants. #438 (comment)

78c8e02

Coding standards.

1ee0718

Coding standards.

64c0dab

Remove extra c&p line. #438 (comment)

180dc1e

Coding style consistency. #438 (comment)

9cfc24f

Coding standards. #438 (comment)

735c4f7

Code tidyup. #438 (comment)

80aedc9

Fix upgrade. Include (install) config to set group sites context.

2348e33

#438 (comment)

Prevent config entites getting deleted by updating dependencies.

ad44bea

Just resaving them while both modules domain_group and the upgraded localgov_microsites_group seems good enough.

Remove group_permisison entities before removing module.

700cf1a

#438 (comment)

finnlewis approved these changes Feb 22, 2024

View reviewed changes

finnlewis merged commit b85a74b into 4.x Feb 22, 2024

		@@ -124,6 +125,7 @@ public function testGroupManagementAccess() {

		// Test member access.

Feature/group sites #438

Feature/group sites #438

Conversation

ekes commented Jan 16, 2024

ekes commented Jan 16, 2024

finnlewis commented Jan 17, 2024 • edited Loading

ekes commented Jan 20, 2024

finnlewis commented Jan 26, 2024

Adnan-cds commented Jan 26, 2024

finnlewis commented Feb 1, 2024

finnlewis commented Feb 1, 2024 • edited Loading

nccchris commented Feb 5, 2024

ekes commented Feb 6, 2024

finnlewis commented Feb 6, 2024

Adnan-cds commented Feb 7, 2024

Adnan-cds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finnlewis commented Feb 8, 2024

Adnan-cds commented Feb 8, 2024

Adnan-cds commented Feb 8, 2024

ekes commented Feb 13, 2024 • edited Loading

ekes commented Feb 13, 2024

ekes commented Feb 13, 2024

ekes commented Feb 13, 2024

Adnan-cds commented Feb 15, 2024

ekes commented Feb 20, 2024

stephen-cox commented Feb 20, 2024

Adnan-cds commented Feb 20, 2024

finnlewis left a comment

Choose a reason for hiding this comment

finnlewis commented Jan 17, 2024 •

edited

Loading

finnlewis commented Feb 1, 2024 •

edited

Loading

ekes commented Feb 13, 2024 •

edited

Loading