department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
99 stars 69 forks source link

Resolve content model tentacles on 10089 Events checkbox #15641

Closed jilladams closed 1 year ago

jilladams commented 1 year ago

Description

10089 added an Events checkbox to the Events form.

This change shipped and broke the content model in the front end in 2 known places:

  1. Outreach Materials -- not displaying Publications in VA.gov frontend
  2. VAMC Stories (example) -- "See all stories" link is missing at the foot of the page.
  3. We also need to re-verify Lovell, per Tim, though still no smoking gun for what that might mean

Slack threads

Topline for DaveC

Engineering notes

Feature Toggle may still be "ON" in prod, despite the revert of our code which created it. We need to be careful about the `FEATURE_EVENT_OUTREACH_CHECKBOX` feature toggle being deployed as "enabled". The latest CMS mirror is showing it as enabled, but unsure if this was because the snapshot was taken before the revert was deployed.
Everywhere field_listing is used in templates/queries ``` 10 results - 6 files src/site/layouts/news_story.drupal.liquid: 45
46: {% if fieldListing.entity.entityUrl.path %} 47: See all stories 48 {% endif %} src/site/layouts/publication_listing.drupal.liquid: 87 {% for entity in outreachAssetsDataArray.entities %} 88 {% for entity in outreachAssetsDataArray.entities %} 89: {% if entity.fieldListing.targetId == entityId %} 89 {% assign topicsString = entity.fieldLcCategories | buildTopicsString %} 90 {% assign topicsString = entity.fieldLcCategories | buildTopicsString %} src/site/stages/build/drupal/process-lovell-pages.js: 69 70 71: if (page?.fieldListing?.entity?.entityUrl) { 72: page.fieldListing.entity.entityUrl.path = getLovellVariantOfUrl( 73: page.fieldListing.entity.entityUrl.path, 73 variant, 74 variant, src/site/stages/build/drupal/graphql/newStoryPage.graphql.js: 33 } 34: fieldListing { 35 entity { src/site/stages/build/drupal/graphql/nodeEvent.graphql.js: 114 } 115 } 116: fieldListing { 116 entity { 117 entity { 284 } 285 } 286: fieldListing { 286 entity { 287 entity { src/site/stages/build/drupal/graphql/file-fragments/outreachAssets.graphql.js: 13 fieldDescription 14: fieldListing { 15 targetId ```

ACs.

Deployment Plan

jilladams commented 1 year ago

Estimate doesn't factor Lovell complexity. Daniel to talk to AP folks and update as needed with Lovell in mind.

dsasser commented 1 year ago

Mid-sprint Update 10/19/23

When taking on this ticket, we were planning no architectural changes on the backend, and instead making the frontend (content-build) robust enough to handle both a single value and an array of values. However, this approach had a lot of technical debt built in, and little risk mitigation was possible.

Because of this, we are pivoting to a different architecture:

Backend Changes

Frontend (content-build) Changes

Overall this approach is much safer, since we aren't touching the structure of an existing field, but adding another field which is brand new to the ecosystem.

jilladams commented 1 year ago

@dsasser I like this and also: it smells like we may get questions at code review time. Would it be worth bouncing this off any other Drupal folks for smell testing before we fully commit? Or feels very safe / uncontroversial as a fix?

dsasser commented 1 year ago

@dsasser I like this and also: it smells like we may get questions at code review time. Would it be worth bouncing this off any other Drupal folks for smell testing before we fully commit? Or feels very safe / uncontroversial as a fix?

Getting feedback during review time is adequate in this case.

jilladams commented 1 year ago

Testing notes: Thread related to testing: https://dsva.slack.com/archives/C52CL1PKQ/p1697837268281799?thread_ts=1697835404.910689&cid=C52CL1PKQ

Tugboat with feature toggle enabled: https://cms-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/admin/config/system/feature_toggle FE: https://web-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/

Editor login, roles, section Event type Expected behavior Pass/fail in CMS Pass/fail in FE
Timothy.Chavis@va.gov, Content creator - VAMC / Content publisher event VAMC event AK, not Natl, recurring Shoudn't see national checkbox 🟒 checkbox does not appear 🟒 Event published normally; AK Events listing fine
edward.mcevoy@va.gov; Content creator - outreach hub, Content publisher event Outreach event, recurring Shouldn't see the checkbox 🟒 checkbox does not appear 🟒 Event publishes correctly, appears on pg 7 of Outreach events, in correct sequence
Winfield.DanielsonIII@va.gov - shortlisted beta editor, Content creator - VAMC Bedford & Boston / Content publisher event VAMC event, Boston, not recurring. National. Should see / select National checkbox. Event should publish to Natl calendar 🟒 checkbox appears, is selected /s aved. 🟒 Event single page; Boston listing; Outreach calendar page 5 of results
Winfield.DanielsonIII@va.gov - shortlisted beta editor, Content creator - VAMC Bedford & Boston / Content publisher event VAMC event, Bedford, not recurring. National. Should see / select National checkbox. Event should publish to Natl calendar 🟒 checkbox appears, is selected /s aved. - 🟒 single page
- 🟒 Bedford listing - first occurrence appears.
- 🟒 Outreach events listing first occurrence appears pg 7, 11/6; no further occurrences appear (11/13, pg 11).

Other FE tests:

Listings:

Recurring events: expected behavior

In production today, for a recurring event, only the next upcoming instance will appear in the Upcoming filter. https://www.va.gov/minneapolis-health-care/events/48206/ https://www.va.gov/minneapolis-health-care/events/?selectedOption=upcoming

https://www.va.gov/outreach-and-events/events/61561/ https://www.va.gov/outreach-and-events/events/

dsasser commented 1 year ago

Deployment Plan

Jill meddled here

I moved this list as checkboxes into the body of the ticket, so it's easy to see next to ACs for modern state of truth.

dsasser commented 1 year ago

@laflannery when you get a chance, could you review this PR for a11y + breadcrumb signoff? Tugboat is hit or miss today, but you should be able to get in eventually if you keep trying. There is a list of existing FE pages that Jill created while testing that might be of interest to you.

laflannery commented 1 year ago

@dsasser I got in for a bit but wasn't able to test everything yet. However I do have one general QA item I wanted to mention - I saw this issue that I had flagged as a D10 issue and thought had been resolved in a previous ticket. Is it a problem that it's here in this tugboat? image

jilladams commented 1 year ago

@dsasser @laflanner I see what Laura is seeing here: https://cms-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/node/62493/edit

I tried to repro that, using this new event: https://cms-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/node/62522/edit

Then got a 502. πŸ˜‚ but will check when it's back up, re: whether the 2nd date fields appear.

dsasser commented 1 year ago

@dsasser @laflanner I see what Laura is seeing here: https://cms-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/node/62493/edit

I tried to repro that, using this new event: https://cms-ytipldihujdcko41uemzx3w18e31hyof.ci.cms.va.gov/node/62522/edit

Then got a 502. πŸ˜‚ but will check when it's back up, re: whether the 2nd date fields appear.

I'm still not clear how to reproduce what you and @laflannery are seeing w/ the 2nd set of date fields. Noting that this would be an unexpected change, since we didn't change those fields in any way intentionally.

laflannery commented 1 year ago

@dsasser and @jilladams some notes:

jilladams commented 1 year ago

The D10 note is interesting - I know the CMS team was most recently planning to ship that 1st week of November. So I just realized: wow, we need to take that into account with this launch. I'll ping CMS channelt o confirm timing.

dsasser commented 1 year ago

@laflannery what is the behavior on staging/prod?

laflannery commented 1 year ago

Staging has this same behavior and shows me the extra fieldset: image

So that being said I feel like I (or someone?) should just ignore this entirely here and ticket this separately right?

jilladams commented 1 year ago

Yes, good call. If in Staging, officially not tied to this feature and we can/should handle separately.

laflannery commented 1 year ago

So back to what I was actually supposed to be reviewing, using the table from Jill above:

Scenario 1

Scenario 2

Scenario 3

Scenario 4

CMS

jilladams commented 1 year ago

( Noting: all test data at tugboat links i comments above got blown away by Tugboat refresh. )

laflannery commented 1 year ago

Everything is approved by me, breadcrumbs look good and not a11y issues!