SHS-5904: Add new "log out" block to existing sites #1696

codechefmarc · 2024-12-10T18:16:02Z

READY FOR REVIEW

Summary

Adds new logout button to existing HumSci sites on both HumSci Colorful and HumSci Traditional themes.

Need Review By (Date)

2024-12-11

Urgency

medium

Steps to Test

Login to the Colorful site: https://hs-colorful-1fwsletf5ghnmd1kmk0ngkockhhydj2r.tugboatqa.com
Verify that there is a logout button in the footer

Login to the Traditional site: https://hs-traditional-1fwsletf5ghnmd1kmk0ngkockhhydj2r.tugboatqa.com
Verify that there is a logout button in the footer

PR Checklist

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208977450116179

…-5904--add-logout-button

cienvaras

@codechefmarc Looks good! I just found a couple of minor things to fix, please take a look at the comments and let me know if you have any question.

cienvaras · 2024-12-13T22:56:17Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+ * Add new logout block to existing sites.
+ */
+function su_humsci_profile_update_9719() {
+  $config_storage = new FileStorage(Settings::get('config_sync_directory'));


Please use config.storage.sync service here:

Suggested change

$config_storage = new FileStorage(Settings::get('config_sync_directory'));

$config_storage = \Drupal::service('config.storage.sync');

For this one, I did try that initially, but it fails:
> [error] The configuration <em class="placeholder">block.block.humsci_colorful_samlsunetidlogoutblock</em> was not found in the sync directory.
I found what worked here:

suhumsci/docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.install

Line 19 in 416ecc8

$config_directory = new FileStorage(Settings::get('config_sync_directory'));

My theory is that this is dealing with theme config? But not sure if that is correct. If we're set on using the service, I'll have to poke at that again next week.

That's weird. We have both versions in the site, but in the past we've had kickbacks because SWS prefers to use the service. Let's keep the FileStorage() version for now and come back if needed after SWS review.

\Drupal::service('config.storage.sync') works fine for me. Using the service is preferred.

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.profile

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

codechefmarc · 2024-12-14T02:06:15Z

@cienvaras - I added in the other fixes from your review, thank you! All testes pass except for Mathematics site and the error is similar to what I've seen before:

Error: >  [error]  Adding non-existent permissions to a role is not allowed. The incorrect permissions are "access webform overview", "administer webform", "create webform", "edit own webform".

IIRC, this may have been the fix, but I could be incorrect:
#1662 (comment)
It was where there was a dependencies hook that was being run to make sure that one of the profile hooks ran before this one. I get why it was removed, but, perhaps Mathematics never ran that 9714 update? Not sure. Anyways, we can discuss this next week. Thanks!

cienvaras

Don't worry about the mathematics site test fail. SWS usually resolves that if the fail is not related with your code. Also let's a reply to your comment about the issue using the config.store.sync service.

@ahughes3 This is ready for you to review. If everything looks as expected we can ask SWS for review and fix any pending issue after that.

cienvaras · 2024-12-16T03:49:42Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+ * Add new logout block to existing sites.
+ */
+function su_humsci_profile_update_9719() {
+  $config_storage = new FileStorage(Settings::get('config_sync_directory'));


That's weird. We have both versions in the site, but in the past we've had kickbacks because SWS prefers to use the service. Let's keep the FileStorage() version for now and come back if needed after SWS review.

ahughes3

looks good!

pookmish · 2024-12-17T19:50:29Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+
+    if (!$existing_config) {
+      // Change the logout link text to match what the existing login text is.
+      $login_blocks = $entity_type_manager->getStorage('block')->loadByProperties([


This is looping through blocks multiple times and doing the same thing to produce a "log out" text. Why not do that prior to creating the config because we don't need to do it multiple times. Just once for one block.
Or maybe you want to also add the theme property condition?

pookmish · 2024-12-17T19:52:26Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+        }
+      }
+
+      $config_data['settings']['link_text'] = $log_out;


$log_out may not be established. Set a default value above as a fallback.

pookmish · 2024-12-17T19:53:35Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+ * Add new logout block to existing sites.
+ */
+function su_humsci_profile_update_9719() {
+  $config_storage = new FileStorage(Settings::get('config_sync_directory'));


\Drupal::service('config.storage.sync') works fine for me. Using the service is preferred.

pookmish · 2024-12-17T19:54:08Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+    // Load the configuration data from the sync directory.
+    $config_data = $config_storage->read($config_name);
+
+    if (!$config_data) {


You could use $config_storage->exists('name')

pookmish · 2024-12-17T19:55:24Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+    $block_storage = $entity_type_manager->getStorage('block');
+    $existing_config = $block_storage->load($config_data['id']);
+
+    if (!$existing_config) {


isn't it guaranteed there will not be existing config? These are new configs and blocks are prefixed if they are custom on sites.

pookmish · 2024-12-17T21:03:19Z

docroot/profiles/humsci/su_humsci_profile/su_humsci_profile.install

+/**
+ * Add new logout block to existing sites.
+ */
+function su_humsci_profile_update_9719() {


Thinking about this a bit more, this update hook is not needed. We force import configs prefixed with block.block.humsci_colorful_ and block.block.humsci_traditional_. So any changes to those configs would be reverted during config import anyways.

ref: https://github.com/SU-HSDO/suhumsci/blob/develop/config/default/config_ignore.settings.yml#L10

… locally with lando, testing on Tugboat

codechefmarc · 2024-12-18T23:27:42Z

@pookmish - Thank you for the assistance! Your PR worked great for this work. I tested locally and on Tugboat and we have logout blocks showing for both Colorful and Traditional. I'm requesting one final code pass from you and @cienvaras but I think we're good now.

* 11.6.1 * Update lock file. * SHS-5911: Add indicator of 3rd level shortcut menu (#1681) * SHS-5911: Add indicator of 3rd level shortcut menu. * fix(shs-5912): change layout of ckeditor styles dropdown list (#1688) Co-authored-by: joegl <[email protected]> * SHS:5892: Site managers can perform actions on user accounts that don't make sense with SSO (#1682) * feat(shs-5892): remove actions from people view * feat(shs-5892): remove elements from used edit form * fix(shs-5892): fix issues in user edit form alter * fix(shs-5892): remove delete actions only for non admin users * Updated dependencies * fix(shs-5911): use absolute path for image url to prevent error when drupal aggregates image (#1693) * HSD8-1700: Increase Tugboat Max Allowed Packet size to 512MB. (#1695) Co-authored-by: Seth Hendrick <[email protected]> * SHS-5808: Uninstall Webform module (#1625) * SHS-5905: Rework social media footer (#1662) * feat(SHS-5905): Recreate social media block as a custom block plugin * feat(SHS-5905): Add composer.lock * feat(SHS-5905): Enable multivalue form element module * feat(SHS-5905): Add update hook for permissions change * fix(SHS-5905): Fix error for default config and add cache context for user * fix(SHS-5905): Contextual menu still not rendering on Tugboat for non-admins * fix(SHS-5905): Fix attached library for contextual menu * fix(SHS-5909): Strict checking of the array seemed to fix the contextual links on local * fix(SHS-5905): Remove strict checking * fix(SHS-5905): Remove old permissions before adding social media block permissions * fix(SHS-5905): Linting fixes * chore(SHS-5905): WIP TEST ONLY - see if removing the code block in the profile works on tugboat * fix(SHS-5905): Fix contextual menu for non-admins * feat(SHS-5905): Remove permission update hook and add dependency on one in profile * feat(shs-5906): icon logic, templates and styles for social media footer block (#1673) * chore(SHS-5905): Temp remove code to test other contextual menus * feat(SHS-5905): Add custom contextual link * fix(SHS-5905): Get contextual links working (finally) for Social Media block * fix(shs-5905): fixes in social media block * fix(shs-5905): replace multivalue_form_element with custom version in social media block * fix(shs-5905): remove multivalue_form_element module * fix(shs-5905): update social media block links help text * fix(shs-5905): social media footer block fixes --------- Co-authored-by: Andrés Díaz Soto <[email protected]> * Update hook to delete webform submissions * SHS-5958: Bug: Inconsistent "Add Above" feature (#1692) * fix(SHS-5958): Fix issue with paragraphs features to allow add buttons on non gin/claro themes * feat(SHS-5958): Add config and update hook to allow add paragraphs in between * refactor(SHS-5958): Change how we are updating the add in between to config * test(SHS-5958): Update test to refelect new buttons * test(SHS-5958): Update testPostCard test for new add in between buttons * test(SHS-5958): Update other tests for new add in between buttons * test(SHS-5958): Add a wait in test to make sure JS add in between buttons load * test(SHS-5958): Fix photo album test - photo album can come before or after text component, making tests work sometimes but not all the time * fix(shs-5958): fix video embed test * fix(shs-5958): fix private page tests * fix(shs-5958): fix video embed test * fix(shs-5958): fix private page tests * fix(shs-5958): fixes in tests * fix(shs-5958): fix private page tests * fix(shs-5958): fix private page tests * chore(SHS-5958): Remove unnecessary layout builder config * chore(SHS-5958): Revert hidden layout builder layout * chore(SHS-5958): Revert composer.lock to 11.6.1 version --------- Co-authored-by: Andrés Díaz Soto <[email protected]> * Removed unintended underscore in update hook name * fixup update function name * HSD8-1707: Removed edit button from default people view to resolve button display to anonymous users. (#1701) * SHS-5904: Add new "log out" block to existing sites (#1696) * HSD8-1664: Updated PR template (#1685) * Updated PR template. * Updated sanitiztion line. * dropped ready for review title. added review tasks heading. --------- Co-authored-by: joegl <[email protected]> * Revoke any webform permissions before uninstall module * Removed assert_options from settings files * HSD8-1702: delete mediterraneanstudies --------- Co-authored-by: Github Actions <[email protected]> Co-authored-by: joegl <[email protected]> Co-authored-by: Mariana Núñez <[email protected]> Co-authored-by: Andrés Díaz Soto <[email protected]> Co-authored-by: Mike Decker <[email protected]> Co-authored-by: Seth <[email protected]> Co-authored-by: Seth Hendrick <[email protected]> Co-authored-by: Marc Berger <[email protected]> Co-authored-by: Joe Gilliland-Lloyd <[email protected]>

codechefmarc added 2 commits December 9, 2024 17:32

feat(SHS-5904): Add logout block to existing sites

e824148

fix(SHS-5904): Fix issue during updb reading config storage

afbdf9a

ahughes3 temporarily deployed to Tugboat December 10, 2024 18:16 Destroyed

codechefmarc self-assigned this Dec 10, 2024

codechefmarc requested a review from cienvaras December 10, 2024 18:33

codechefmarc added the Needs review label Dec 10, 2024

codechefmarc marked this pull request as ready for review December 10, 2024 18:33

codechefmarc mentioned this pull request Dec 11, 2024

SHS-5811: Add logout button to sites #1641

Closed

Merge branch '11.6.1-release' of github.com:SU-HSDO/suhumsci into SHS…

5ae139b

…-5904--add-logout-button

ahughes3 temporarily deployed to Tugboat December 13, 2024 22:45 Destroyed

cienvaras requested changes Dec 14, 2024

View reviewed changes

fix(SHS-5904): Minor fixes from code review

7167595

ahughes3 temporarily deployed to Tugboat December 14, 2024 01:35 Destroyed

codechefmarc requested a review from cienvaras December 14, 2024 02:06

cienvaras approved these changes Dec 16, 2024

View reviewed changes

cienvaras assigned cienvaras and ahughes3 and unassigned codechefmarc and cienvaras Dec 16, 2024

cienvaras requested a review from ahughes3 December 16, 2024 03:53

ahughes3 approved these changes Dec 17, 2024

View reviewed changes

ahughes3 assigned pookmish and unassigned ahughes3 Dec 17, 2024

ahughes3 requested review from pookmish and sethprime December 17, 2024 06:13

pookmish requested changes Dec 17, 2024

View reviewed changes

codechefmarc added 2 commits December 17, 2024 15:53

Merge branch '11.6.1-release' into SHS-5904--add-logout-button

3347d63

refactor(SHS-5904): Use service instead of FileStorage, does not work…

0c6f20c

… locally with lando, testing on Tugboat

ahughes3 temporarily deployed to Tugboat December 18, 2024 00:08 Destroyed

codechefmarc and others added 3 commits December 17, 2024 17:39

refactor(SHS-5904): Change to theme-based config import

a9dfee8

Simplify update hook to create logout blocks

6765439

Merge branch 'SHS-5904-simple-update' into SHS-5904--add-logout-button

fb00382

ahughes3 temporarily deployed to Tugboat December 18, 2024 23:00 Destroyed

codechefmarc requested review from pookmish and cienvaras December 18, 2024 23:27

pookmish approved these changes Dec 19, 2024

View reviewed changes

pookmish merged commit d087ab8 into 11.6.1-release Dec 19, 2024
17 checks passed

pookmish deleted the SHS-5904--add-logout-button branch December 19, 2024 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHS-5904: Add new "log out" block to existing sites #1696

SHS-5904: Add new "log out" block to existing sites #1696

codechefmarc commented Dec 10, 2024 •

edited by ahughes3

Loading

cienvaras left a comment

cienvaras Dec 13, 2024

codechefmarc Dec 14, 2024

cienvaras Dec 16, 2024

pookmish Dec 17, 2024

codechefmarc commented Dec 14, 2024

cienvaras left a comment

cienvaras Dec 16, 2024

ahughes3 left a comment

pookmish Dec 17, 2024

pookmish Dec 17, 2024

pookmish Dec 17, 2024

pookmish Dec 17, 2024

pookmish Dec 17, 2024

pookmish Dec 17, 2024

codechefmarc commented Dec 18, 2024

	$config_storage = new FileStorage(Settings::get('config_sync_directory'));
	$config_storage = \Drupal::service('config.storage.sync');

SHS-5904: Add new "log out" block to existing sites #1696

SHS-5904: Add new "log out" block to existing sites #1696

Conversation

codechefmarc commented Dec 10, 2024 • edited by ahughes3 Loading

READY FOR REVIEW

Summary

Need Review By (Date)

Urgency

Steps to Test

PR Checklist

cienvaras 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

codechefmarc commented Dec 14, 2024

cienvaras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahughes3 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

codechefmarc commented Dec 18, 2024

codechefmarc commented Dec 10, 2024 •

edited by ahughes3

Loading