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 68 forks source link

Strange Node View behavior for optional segments on CLP that have been enabled and then disabled #19165

Open davidmpickett opened 1 month ago

davidmpickett commented 1 month ago

Statuses

[2024-09-12] [Fran] Decision log added identifying Option 1 as the desired solution; AC added; moved to the bottom of Next Refinement.

Description

When sections are unchecked/disabled, clear out any text/values previously entered.

Background

This came up during Q&A for #18954 and was deemed out of scope.

Note that there are two distinct, but related related errors you might encounter while reproducing this issue

Reproduction steps

  1. Go to staging.cms.va.gov (or a Tugboat)
  2. Create a new Campaign Landing Page
    • [ ] Only fill in the required fields in the Hero banner, Why this matters, What you can do, and VA benefits sections
    • [ ] Save the node
    • [ ] Validate that the Node View matches the fields populated on Node Edit
      1. Edit the node and add an optional section
    • [ ] Enable one of the following optional sections, Video, Spotlight, Stories, Downloadable resources, Events, FAQs
    • [ ] Fill out required fields for the optional section
    • [ ] Save the node
    • [ ] Validate that the Node View updates with the new section
  3. Edit the node and disable the optional section
    • [ ] Uncheck the "Enable this page segment" box on the section you just added
    • [ ] Save the node
    • [ ] Notice that Node View still shows the content from the subfields of the section you disabled

Possible solution paths

Option 1

Option 2

Decision Log

Option 1 will be implemented, mimicking existing CMS functionality (e.g. Events).

Testing & QA

Scope / Impact analysis

What, if anything, could break as a result of this change? Engineer should assess this when approaching PR.

Roles / assignments

After functional testing, code review, accessibility review, and design review can happen in parallel.

Acceptance Criteria

FranECross commented 1 month ago

@davidmpickett @dsasser I'm looking at the options and thinking #2 would be a good solution, IF we have situations where an editor might follow the following scenario when entering information: They enter information into a CLP that they want to publish right away, but info they enter into a sub-field they want to get further approval on. They uncheck the box so it doesn't display, but they don't want their information to be cleared out because when they get approval, they will then edit, uncheck the box so that it WILL display, and then republish. Thoughts on feasibility and effort?

dsasser commented 1 month ago

In other areas of the CMS, Events for instance, we clear the data from fields that are hidden when making form selections.

They enter information into a CLP that they want to publish right away, but info they enter into a sub-field they want to get further approval on. They uncheck the box so it doesn't display, but they don't want their information to be cleared out because when they get approval, they will then edit, uncheck the box so that it WILL display, and then republish. Thoughts on feasibility and effort?

I would use a node revision in this case. Here the user would create the publish-able version and publish it. Then create a draft of the new changes that need approval.

In my opinion we should (always?) be clearing data when it is hidden dynamically.

davidmpickett commented 1 month ago

I agree with @dsasser that option 1 / clearing out the data is generally the more robust/performant option with dynamically hidden fields. It prevents weird validation glitches like #19161. It's also an established pattern within the CMS so would be relatively straightforward to implement.

Option 2 would be a whole new paradigm and would in someways us fighting against the grain to accommodate a small, hypothetical use case

There's also Option 0, which is to leave it as is. This is not breaking any functionality, just leading to possible confusion.

FranECross commented 1 week ago

@FranECross to create a spike to look at other fields and this ticket is restricted to this particular issue.