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
96 stars 70 forks source link

[VA Benefit Taxonomy] Benefit Description fields minimum character limits & other updates #18170

Open FranECross opened 3 months ago

FranECross commented 3 months ago

Status

[2024-08-07] [Daniel] Daniel pre-refined. [2024-07-31] [Fran] Asked Daniel via Slack to async refine. [2024-07-26] [Dave] Dave updated the ticket to reflect the updated direction. There are still some open questions / considerations, so definitely needs more refinement, but probably ready for Daniel to take an async pass. [2024-07-24] [Dave] @davidmpickett to take next pass at refining based on decisions [2024-07-19] [Fran] Moved back to Drafts pending a meeting Michelle M has with Danielle and Randi H on 7/24.

Description

There are three (existing) fields for providing Benefit Descriptions to Veterans, their family members, and/or caregivers:

When we first set up the taxonomy, we set maximum character limits on two of the fields, but did not make any field required or set minimum character limit.

screencapture-staging-cms-va-gov-admin-structure-taxonomy-manage-va-benefits-taxonomy-add-2024-07-26-11_14_31

Desired field configurations

✅ = already in place
🔲 = work to do in this ticket

Minimum characters

The module we use for character counters doesn't currently have a way to set/display/enforce minimum character limits. There's an open upstream issue. If we want to display minimum characters dynamically, we'd probably need to address that.

I believe we could set a minimum that's enforced on save, but that's not a great user experience to get an error message.

Help text updates

Fran to work with CAIA to provide content for the editorial interface for best practices:

User story

AS AN editor adding benefit descriptions in the editor interface I WANT to clearly understand the character limits for the Longer summary, Brief summary, and Teaser summary, as well aswhich fields are required SO THAT I'll can enter content appropriate to each field

Questions

Engineering notes / background

Analytics considerations

Quality / testing notes

Acceptance criteria

davidmpickett commented 2 months ago

The logic here seems a little backwards. Essentially the logic is ensuring that there always needs to be a summary that is under 300 characters. But rather than the primary place that the shortest description lives being the field with the 300 character limit (Teaser summary), it could be in field with a 500 character (Brief Summary) or the one with a larger character limit (Longer summary).

From the perspective of a FE developer querying these fields, if I wanted to get a description that was less than 300 characters I'd have to have a logic that always checks all three fields and possibly count the length of them.

Fields and possible responses

If we flipped the logic and made Teaser summary Required and added MIMIMUN character limits on the other two fields then the expected responses would map like this:

FranECross commented 2 months ago

@davidmpickett Thank you for your explanation and information! I'm tagging @DanielleThierryUSDSVA and @RLHecht for their review/agreement since (I believe) Dave C mentioned the requirements were reviewed with / came from them, and also @mmiddaugh as our new PO.

@DanielleThierryUSDSVA, @RLHecht and @mmiddaugh, regarding the Benefit Description fields (Long summery, Brief Summary, Teaser Summary), will you please review Dave Picket's suggestion in the the comment just above and let me know if we should alter the requirements to fit what he proposes (BTW I think it makes good cents - my 2 cents).

2024-07-09 Edit: Question posed in Slack for better visibility.

davidmpickett commented 1 month ago

@FranECross - FYI, I chatted with @mmiddaugh about this briefly in a 1on1 today. She is aligned with me on the direction of keeping the fields consistent lengths and having a more straightforward Required / Not Required implementation rather than the conditional Required logic proposed in the ticket body.

mmiddaugh commented 1 month ago

@FranECross @davidmpickett I will address this topic during a meeting with Danielle and Randi H on 7/24

davidmpickett commented 1 month ago

Moving out of next refinement until that meeting has happened

mmiddaugh commented 1 month ago

@davidmpickett @FranECross - Danielle and Randi approved your proposal to make only the teaser summary required with the character restrictions as written

Moving this into the refinement column

davidmpickett commented 1 month ago

@FranECross @dsasser FYI - I have updated the ticket to reflect the updated direction. There are still some open questions / considerations, so definitely needs more refinement, but probably ready for Daniel to take an async pass.

dsasser commented 1 month ago

Engineering Pre-Refinement

Point estimate: 5️⃣ (assumes we are going with the below approach):

The bulk of the work here is adding minmum length for Textfield Counter fields. We will try the patch to the Texfield Counter module to resolve this. This will promote and leverage community involvement. However the patch is sizeable, so we may need to spend a good amount of time time debugging and updating the patch.

Answers(?) to open questions

Are there any excluded symbols? If so, take into consideration: Should helper text indicate excluded symbols? Specific error messaging will need to display if excluded symbols are used.

That seems like a different validation that could be added later. Trying to keep issues lean but if this is a requirement that I'm not seeing please ignore.

Teaser summary is currently using string_long which means it can't do Text Format validation. Might want to switch it to text_long and use "Plain Text" Text Format

Not sure this belongs here. It would require a data migration/wonky config wrangling as storage types cannot be swapped out easily (it isn't a UI change, and there are no APIs for making this easy to do). This has my vote for being its own ticket.

Might also want to create a new "Plainer Text" text format since even "Plain Text" allows some HTML tags

I'm not sure what the use case would be? HTML tags are allowed in plain text fields because they aren't treated as HTML but their respective HTML entities. Either way, sounds like another thing we could possibly peel off to a separate ticket since there seems like a discussion/more context would be needed to flush this out.