Skip to content

SWSDEVOPS-150 - trustee profile update retry#79

Merged
mdyoung3 merged 112 commits into5.xfrom
SWSDEVOPS-150-trustee-profile-update-retry
Mar 18, 2025
Merged

SWSDEVOPS-150 - trustee profile update retry#79
mdyoung3 merged 112 commits into5.xfrom
SWSDEVOPS-150-trustee-profile-update-retry

Conversation

@mdyoung3
Copy link
Contributor

READY FOR REVIEW

Summary

  • Update to trustee profile, incorporating changes from Stanford Profile
  • Minor Drupal update (10.3 -> 10.4)

Review By (Date)

March 17

Criticality

  • How critical is this PR on a 1-10 scale? Also see Severity Assessment.
  • E.g., it affects one site, or every site and product?

Urgency

  • How urgent is this? (Normal, High)

Review Tasks

Setup tasks and/or behavior to test

  1. Review code.
  2. Review sites.

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

  • JIRA ticket(s)
  • Other PRs
  • Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
  • Anyone who should be notified? (@mention them here)

Resources

buttonwillowsix and others added 30 commits September 30, 2024 22:02
…#822)

* D8CORE-7564: removed decanter link and replaced with identity guide info.

* D8CORE-7564: added in a handy link
@@ -0,0 +1,12 @@
_core:
default_config_hash: xbYr66-g0FjNgVBkGypCuN46vBI2XHntXN1URawq1s4
Copy link
Contributor

@joegl joegl Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this configuration is for the upgrade_status module which is only enabled on dev/local. This configuration belongs in those config_split's then and not here.

@joegl
Copy link
Contributor

joegl commented Mar 13, 2025

It doesn't look like the config_split.config_split.stage.yml changes got pulled in (which most notably enable the shield module on staging).

@joegl joegl self-requested a review March 13, 2025 20:02
Copy link
Contributor

@joegl joegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few small things I noticed in the code. I left comments on both this and the ace-botgryphon PR (even though I know you're still working on that one).

I was able to successfully set up the repo locally, run a composer install, run a drush si trustee_profile and sync the trustees site to my local without issue. I also reviewed the trustees-dev site. The only thing I noticed during the site reviews was you may want to address this "warning" on the status report page (not a big deal though):

State cache flag $settings['state_cache'] is not set. It is recommended to be set to TRUE in settings.php unless there are too many state keys. Drupal 11 will default to having state cache enabled.```

All-in-all it looks good for such a big update.

@mdyoung3 mdyoung3 requested a review from joegl March 14, 2025 16:14
Copy link
Contributor

@joegl joegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mdyoung3 mdyoung3 merged commit a3b0e28 into 5.x Mar 18, 2025
7 of 13 checks passed
@mdyoung3 mdyoung3 deleted the SWSDEVOPS-150-trustee-profile-update-retry branch March 18, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants