localgovdrupal / localgov_microsites_group

LocalGov Drupal Microsites Group integration
1 stars 2 forks source link

Feature/group sites #438

Closed ekes closed 7 months ago

ekes commented 8 months ago

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.

ekes commented 8 months ago

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 https://github.com/localgovdrupal/localgov_microsites_group/issues/429#issuecomment-1872992368

finnlewis commented 8 months ago

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.

  1. log in as user 1
  2. create controller user with microsites controller role
  3. log in as controller
  4. create microsite at https://localgov-microsites.ddev.site/admin/microsites/add/microsite
  5. note that the default owner is the controller user
  6. try to edit the microsite
  7. 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...

image

ekes commented 8 months ago

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 https://github.com/localgovdrupal/localgov_microsites_group/pull/438/commits/9c975ba7daa738c1a5142a65224e56118e706819

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:

finnlewis commented 8 months ago

Thanks @ekes - the install is working for me now since that commit https://github.com/localgovdrupal/localgov_microsites_group/commit/9c975ba7daa738c1a5142a65224e56118e706819

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 commented 8 months ago

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

finnlewis commented 8 months ago

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

finnlewis commented 8 months ago

@ekes mentioned : on https://github.com/localgovdrupal/localgov_microsites_group/issues/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 commented 8 months ago

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 commented 8 months ago

@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 commented 7 months ago

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 commented 7 months ago

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.

finnlewis commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

ekes commented 7 months ago

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 image Which is being done on install as noted by Adnan https://github.com/localgovdrupal/localgov_microsites_group/pull/438#discussion_r1481937679 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 commented 7 months ago

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? :confused:

ekes commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

ekes commented 7 months ago

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 https://github.com/localgovdrupal/localgov_microsites_group/pull/438/commits/700cf1a20d48a9c3e1f8b29b8d30f15cde5bb42d should fix that for you?

stephen-cox commented 7 months ago

If I do an initial install from https://github.com/localgovdrupal/localgov_microsites_group/pull/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 commented 7 months ago

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.