VACMS-17018 Resolve VAMC menu assignment issues. #16994

swirtSJW · 2024-01-24T01:36:19Z

Description

Relates to #17018 (or closes?)

Testing done

Screenshots

QA steps

What needs to be checked to prove this works?
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?

As user RS (VAMC Content creator and content publisher in one VAMC health system section)

Go to https://pr16994-ksy8sixki8oyk0owsscsp9ijgfxn7pfi.ci.cms.va.gov/node/5579/edit
- Validate that the menu parent is "Work with us" and is not locked
- Validate that you can save the node successfully.

Test creating new Go to https://pr16994-ksy8sixki8oyk0owsscsp9ijgfxn7pfi.ci.cms.va.gov/node/add/health_care_region_detail_page
- Validate that you can not pick "Work with us" as a menu parent option.
- fill out all required fields
- Validate it can be saved when some other parent choice is made.
Edit the page you just created and set the menu parent to "Select a value".
- Validate that php validation prevents the save
- Validate that you are able to select parent after the failed save. (This was an existing defect, separate from this work.)

Definition of Done

Documentation has been updated, if applicable.
Tests have been added if necessary.
Automated tests have passed.
Code Quality Tests have passed.
Acceptance Criteria in related issue are met.
Manual Code Review Approved.
If there are field changes, front end output has been thoroughly checked.

Select Team for PR review

Is this PR blocked by another PR?

DO NOT MERGE

Does this PR need review from a Product Owner

Needs PO review

CMS user-facing announcement

Is an announcement needed to let editors know of this change?

Yes, and it's written in issue ____ and queued for publication.
- Merge and ping the UX writer so they are ready to publish after deployment
Yes, but it hasn't yet been written
- Don't merge yet -- ping the UX writer to write and queue content
No announcement is needed for this code change.
- Merge & carry on unburdened by announcements

swirtSJW · 2024-01-24T01:41:50Z

docroot/modules/custom/va_gov_media/va_gov_media.module

@@ -80,4 +80,5 @@ function va_gov_media_clientside_validation_validator_info_alter(&$validators) {
  foreach ($validators as &$validator) {
    $validator['attachments']['library'][] = 'va_gov_media/cv.alt-text.validate';
  }
+  unset($validator);
 }


This is necessary to avoid the first pink warning on this page
https://www.php.net/manual/en/control-structures.foreach.php

A foreach by reference, must close the barn door by unsetting the referenced variable.

This was causing the last validation in the list of validations to appear twice, and get executed that way.

omahane

All but one of the pages check out.

Can you confirm whether it should be locked or not?

Validate Womens Veteran care https://pr16994-ksy8sixki8oyk0owsscsp9ijgfxn7pfi.ci.cms.va.gov/node/3084/edit is not locked.

Result

swirtSJW · 2024-01-24T15:48:15Z

Sorry, bad copy paste. This is compared to a 2 month old training environment. It should be locked

swirtSJW · 2024-01-24T15:48:53Z

There is one functional change coming out of this, but it is actually more in alignment with what shoud be happening.

EDIT: this is no longer an issue. I have restored the functionality to exactly match what we had before.

swirtSJW · 2024-01-24T15:54:32Z

There is a regression though that should not be allowed in. One more fix needed

EDIT: this is no longer an issue. I have restored the functionality to exactly match what we had before.

swirtSJW · 2024-01-24T23:10:15Z

docroot/modules/custom/va_gov_menu_access/src/Service/MenuReductionService.php

-          // to be enabled.
-          $menu_element_type = self::ENABLED;
-        }
+


The reverted temporary fix from #16975

swirtSJW · 2024-01-24T23:10:46Z

docroot/modules/custom/va_gov_menu_access/src/Service/MenuReductionService.php

@@ -624,7 +620,7 @@ protected function preventUnintendedChangeOfParent(array &$form) {
      return;
    }
    // This is not new so check the current parent exists in the reduced form.
-    elseif (!$current_menu_item_is_present || $this->isCurrentMenuSettingDisabled($form)) {
+    elseif (!$current_menu_item_is_present || $this->isCurrentMenuParentDisabledAndLocked($form)) {


Renaming to make more accurate.

omahane · 2024-01-25T14:15:59Z

Existing defect: Selecting "- Select a value -" in Parent link

Note: I think this is a highly unlikely scenario.

Staging:

Saving as draft

If the page is under "Work with us":
- Change **Parent link" to "- Select a value -"
- Choose "Draft" under Save as
- Click Save
- PHP validation throws: "1 error has been found: Parent link (Can not be changed.)"
- Parent link, now labeled Parent link (Can not be changed.) is disabled with "- Select a value -" shown as selected
- Underneath Parent link (Can not be changed.) it reads "You can only change the parent menu link for the published version of this content."
- Change Save as to "Published"
- Click Save
- Node is saved under "Work with us"

Saving as published

If the page is under "Work with us":
- Change **Parent link" to "- Select a value -"
- Choose "Published" under Save as
- Click Save
- PHP validation throws: "1 error has been found: Parent link (Can not be changed.)"
- Parent link, now labeled Parent link (Can not be changed.) is disabled with "- Select a value -" shown as selected
- Underneath Parent link (Can not be changed.) it reads "Menu parent item cannot be empty."
- Click Save
- Node is saved under "Work with us"

Summary

The current defect is mostly in the presentation of the value in the Parent Link field, as saving with "- Select a value -" ultimately results in no change, though the error message and the disabled state and value of the Parent link are misleading and would likely cause an editor to back out of the change by closing the window or refreshing the page.

PR env:

Saving as draft

If the page is under Work with us:
- Change **Parent link" to "- Select a value -"
- Choose "Draft" under Save as
- Click Save
- PHP validation throws: "1 error has been found: [Parent link]"
- Parent link, now labeled Parent link is still enabled with "- Select a value -" shown as selected
- Underneath Parent link it reads "You can only change the parent menu link for the published version of this content."
- Though Parent link is still enabled, and the page can be moved under a child of Work with us, it cannot be placed under Work with us, where it belongs.
- Change Save as to "Published"
- Click Save
- PHP validation throws: "1 error has been found: [Parent link]"
- Underneath Parent link it reads "Menu parent item cannot be empty."

Saving as published

If the page is under "Work with us":
- Change **Parent link" to "- Select a value -"
- Choose "Published" under Save as
- Click Save
- PHP validation throws: "1 error has been found: Parent link"
- Parent link, now labeled Parent link is still enabled with "- Select a value -" shown as selected
- Underneath Parent link it reads "Menu parent item cannot be empty."
- Though Parent link is still enabled, and the page can be moved under a child of Work with us, it cannot be placed under Work with us, where it belongs.
- Click Save
- PHP validation throws: "1 error has been found: [Parent link]"
- Underneath Parent link it reads "Menu parent item cannot be empty."

Summary

The new defect does actually prevent the saving the node under "Work with us" if the editor has chosen "- Select a value -". However, similarly to the above, this would likely cause an editor to back out of the change by closing the window or refreshing the page.

omahane · 2024-01-25T14:54:17Z

Validate Whole health https://pr16994-ksy8sixki8oyk0owsscsp9ijgfxn7pfi.ci.cms.va.gov/node/59861/edit is locked

This one is not locked, and I'm not sure why it would be. It's under Programs. That doesn't appear to exclude children in VAMC Menu Custom Access

omahane

@swirtSJW All QA but one checks out. "Whole health" was not locked, but I don't think that it should be. Please confirm.

Regarding the defect, I have already commented. To reiterate, I think it's an edge case and neither the current behavior or the new behavior is likely preferred. I leave it to you to decide the appropriate next step.

swirtSJW · 2024-01-25T15:20:06Z

Validate Whole health https://pr16994-ksy8sixki8oyk0owsscsp9ijgfxn7pfi.ci.cms.va.gov/node/59861/edit is locked

This one is not locked, and I'm not sure why it would be. It's under Programs. That doesn't appear to exclude children in VAMC Menu Custom Access

Yep it was a faulty QA step. Confirmed with old demo environment I updated the QA step to reflect that.

swirtSJW · 2024-01-25T15:28:05Z

@swirtSJW
Regarding the defect, I have already commented. To reiterate, I think it's an edge case and neither the current behavior or the new behavior is likely preferred. I leave it to you to decide the appropriate next step.

That is a really good find. I think the draft tramples on things because there is contrib validation that will not allow change of menu with a draft when there is a published revision because menus didn't have revisions and workflow for a draft to associate with. So changing a menu on a draft save of a published is never possible, but I think the error messages might be trampling on each other and the lock state gets confused.

As you say, neither the existing or the new experience are ideal.
I have to ponder this one a bit more.

swirtSJW · 2024-01-25T15:48:31Z

@omahane I think the behaviour / bug of the draft -> published empty -> to not being able to put the page back where it belongs is ok. It prevents anything bad from happening AND still does not allow them to move the page.

As you said, this is a really slim edge case. There is a way where the form could look at the form_state to see what the value used to be, then allow that value, but there would be almost no way to detect it happening and only look up the default revision if needed. It would need to consult the default revision every time and I don't want to entangle that fix into this one.

This edge case defect is unlikely and tricky but right now it favors data protection and is less broken than it was where the field became locked just by setting it to empty and trying to save.

Created new defect ticket #17032

omahane

QA checks out. I like the comments about the client-side validation, as this is such a narrow needle to thread.

edmund-dunn

Nice work!

swirtSJW · 2024-01-25T18:30:45Z

This reverts commit 0ef8e57. This commit bypassed the root cause of the issue and hardcoded a bypass.

…menu.

swirtSJW self-assigned this Jan 24, 2024

va-cms-bot temporarily deployed to Tugboat January 24, 2024 01:36 Destroyed

swirtSJW commented Jan 24, 2024

View reviewed changes

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

va-cms-bot temporarily deployed to Tugboat January 24, 2024 06:41 Destroyed

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

omahane reviewed Jan 24, 2024

View reviewed changes

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 24, 2024

swirtSJW changed the title ~~VACMS-0000 Save test~~ VACMS-17018 Resolve VAMC menu assignment issues. Jan 24, 2024

swirtSJW force-pushed the VACMS-16959-fix-work-with-us-menu-lock branch from 124c170 to ffcdcfc Compare January 24, 2024 22:41

va-cms-bot temporarily deployed to Tugboat January 24, 2024 22:41 Destroyed

swirtSJW requested a review from omahane January 24, 2024 22:43

swirtSJW marked this pull request as ready for review January 24, 2024 22:57

swirtSJW commented Jan 24, 2024

View reviewed changes

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

swirtSJW added Facilities Facilities products (VAMC, Vet Center, etc) CMS Team CMS Product team that manages both editor exp and devops labels Jan 25, 2024

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

swirtSJW force-pushed the VACMS-16959-fix-work-with-us-menu-lock branch from ffcdcfc to 7ec149a Compare January 25, 2024 13:19

va-cms-bot had a problem deploying to Tugboat January 25, 2024 13:19 Error

swirtSJW requested review from edmund-dunn and a team and removed request for a team January 25, 2024 14:05

omahane reviewed Jan 25, 2024

View reviewed changes

omahane self-requested a review January 25, 2024 16:00

omahane approved these changes Jan 25, 2024

View reviewed changes

swirtSJW mentioned this pull request Jan 25, 2024

Detail page menu parent becomes unfixable - tiny edge case #17032

Open

11 tasks

olivereri approved these changes Jan 25, 2024

View reviewed changes

edmund-dunn approved these changes Jan 25, 2024

View reviewed changes

va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 25, 2024

swirtSJW added 4 commits January 25, 2024 13:30

Revert "VACMS-16959: enabled Work with us parent menu item (#16975)"

b45afd2

This reverts commit 0ef8e57. This commit bypassed the root cause of the issue and hardcoded a bypass.

VACMS-16959 Resolve duplicate validators.

b4efe5a

VACMS-16959 Prevent clientside validation from being too greedy with …

5c58145

…menu.

VACMS-16959 Fix bug of empty being locked down.

bd08d10

swirtSJW force-pushed the VACMS-16959-fix-work-with-us-menu-lock branch from 7ec149a to bd08d10 Compare January 25, 2024 18:30

swirtSJW merged commit f8508b2 into main Jan 25, 2024
14 checks passed

swirtSJW deleted the VACMS-16959-fix-work-with-us-menu-lock branch January 25, 2024 18:31

va-cms-bot temporarily deployed to Tugboat January 25, 2024 18:31 Destroyed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VACMS-17018 Resolve VAMC menu assignment issues. #16994

VACMS-17018 Resolve VAMC menu assignment issues. #16994

swirtSJW commented Jan 24, 2024 •

edited by omahane

Loading

swirtSJW Jan 24, 2024 •

edited

Loading

omahane left a comment •

edited

Loading

swirtSJW commented Jan 24, 2024

swirtSJW commented Jan 24, 2024 •

edited

Loading

swirtSJW commented Jan 24, 2024 •

edited

Loading

swirtSJW Jan 24, 2024

swirtSJW Jan 24, 2024

omahane commented Jan 25, 2024 •

edited

Loading

omahane commented Jan 25, 2024 •

edited

Loading

omahane left a comment •

edited

Loading

swirtSJW commented Jan 25, 2024

swirtSJW commented Jan 25, 2024 •

edited

Loading

swirtSJW commented Jan 25, 2024 •

edited

Loading

omahane left a comment

edmund-dunn left a comment

swirtSJW commented Jan 25, 2024

VACMS-17018 Resolve VAMC menu assignment issues. #16994

VACMS-17018 Resolve VAMC menu assignment issues. #16994

Conversation

swirtSJW commented Jan 24, 2024 • edited by omahane Loading

Description

Testing done

Screenshots

QA steps

Definition of Done

Select Team for PR review

Is this PR blocked by another PR?

Does this PR need review from a Product Owner

CMS user-facing announcement

swirtSJW Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

omahane left a comment • edited Loading

Choose a reason for hiding this comment

Result

swirtSJW commented Jan 24, 2024

swirtSJW commented Jan 24, 2024 • edited Loading

swirtSJW commented Jan 24, 2024 • edited Loading

swirtSJW Jan 24, 2024

Choose a reason for hiding this comment

swirtSJW Jan 24, 2024

Choose a reason for hiding this comment

omahane commented Jan 25, 2024 • edited Loading

Existing defect: Selecting "- Select a value -" in Parent link

Staging:

Saving as draft

Saving as published

Summary

PR env:

Saving as draft

Saving as published

Summary

omahane commented Jan 25, 2024 • edited Loading

omahane left a comment • edited Loading

Choose a reason for hiding this comment

swirtSJW commented Jan 25, 2024

swirtSJW commented Jan 25, 2024 • edited Loading

swirtSJW commented Jan 25, 2024 • edited Loading

omahane left a comment

Choose a reason for hiding this comment

edmund-dunn left a comment

Choose a reason for hiding this comment

swirtSJW commented Jan 25, 2024

swirtSJW commented Jan 24, 2024 •

edited by omahane

Loading

swirtSJW Jan 24, 2024 •

edited

Loading

omahane left a comment •

edited

Loading

swirtSJW commented Jan 24, 2024 •

edited

Loading

swirtSJW commented Jan 24, 2024 •

edited

Loading

omahane commented Jan 25, 2024 •

edited

Loading

omahane commented Jan 25, 2024 •

edited

Loading

omahane left a comment •

edited

Loading

swirtSJW commented Jan 25, 2024 •

edited

Loading

swirtSJW commented Jan 25, 2024 •

edited

Loading