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
98 stars 69 forks source link

CMS: Events recurrence - if field_datetime_range_timezone[0][interval] field is set to 1, does not display to users on Node Edit #9453

Open jilladams opened 2 years ago

jilladams commented 2 years ago

Status

[2024-09-18] [Dave] Identified 2 upstream Module issues that would resolve this. I don't think it's worth trying to have one of our Drupal engineers try and push the suggested fixes through the upstream process, given that this is not a priority defect for our team. [2024-08-19] [Fran] We would like UX folks to evaluate to determine if help text or ? should be done for this issue. [2024-04-23] Since this behavior is confusing to editors, I'm (Fran) moving this to next Refinement to ask Jordan Wood to evaluate.

Describe the defect

In the CMS, a recurring event can be created with no value specified in "Repeat every: [# of]" field (field_datetime_range_timezone[0][interval] ).

On an event with recurrence that has been previously created, in the edit view, the field_datetime_range_timezone[0][interval] field is empty.

When this field is left empty, Event will save, and recurrence will assume field_datetime_range_timezone[0][interval] = 1.

Additional Information

  1. Although Swirt indicated the code might be fragile and he doesn't feel it's actually worth pursuing, Jill (and Fran) feel that maybe we could add help text or something that says an empty value will be set to 1? I think you're right that it's not a deep problem, but it is very confusing to come edit a node you did not create, and try to figure out exactly what's happening. Would like UX to evaluate.
  2. This missing recurrence appears to have possibly broken content release on 9/19/24: https://dsva.slack.com/archives/CT4GZBM8F/p1726766045895619?thread_ts=1726762852.899099&cid=CT4GZBM8F

To Reproduce

Steps to reproduce the behavior: To create an event in this state:

  1. https://staging.cms.va.gov/node/add/event
  2. Specify title, start/end day & time
  3. Click "Make recurring"
  4. Change days to weeks
  5. Select M for Monday
  6. Set Until to a future date several weeks from now.
  7. Save
  8. In the preview of the new node, see that Content > Date & Time list of recurrences has created occurrences 1x / week til the end date.

To view a live event in this state:

  1. Go to https://prod.cms.va.gov/node/46932/edit
  2. Under Date and time: Repeat every [ # of ] (field-datetime-range-timezone-0-interval) is empty
  3. Weeks (field_datetime_range_timezone[0][repeat]) is set
  4. Click "Edit event series" button to see that recurrences are set to weekly. (With some recurrences canceled.)

Expected behavior

EITHER:

  1. The form should not save with an empty field-datetime-range-timezone-0-interval, OR
  2. When no value is set, we populate field-datetime-range-timezone-0-interval with the value 1, to make the recurrence pattern clear to editors in future

Screenshots

Edit screen:

![Screen Shot 2022-06-14 at 1 45 43 PM](https://user-images.githubusercontent.com/85581471/173685551-dcb79fbd-3d52-4ac7-b0ce-a3a91a9e41bb.png)

Recurrence:

![Screen Shot 2022-06-14 at 1 45 51 PM](https://user-images.githubusercontent.com/85581471/173685569-9edb6a5b-d687-4668-9eaa-160dd27d543b.png)
swirtSJW commented 2 years ago

I don't think this one is actually a problem starting with image

results are what I would expect image

I don't think we want to get into requiring or forcing the data. That could give undesired results.

swirtSJW commented 2 years ago

Looking through the smartdate module issue queue I am not seeing any other reports suggesting "Repeat every" needs a required or forced value.

The logic for conditional fields and display can be a bit fragile in general. I'd prefer to avoid tinkering with it too much unless it is clearly needed.

jilladams commented 2 years ago

Maybe we could add help text or something that says an empty value will be set to 1? I think you're right that it's not a deep problem, but it is very confusing to come edit a node you did not create, and try to figure out exactly what's happening. (Curious @suzanne-gray 's opinion there, as CMS UX guidance. Or loop anyone else who should opine.)

davidmpickett commented 1 month ago

Our issue title here is misleading. It is not possible to submit the form with an empty value. The value defaults to 1. However, the behavior of the dropdown field if the value is set to 1 is to display "# of" instead of 1.

This is true even if you change the value to 1 and save Screenshot 2024-09-18 at 1 56 50 PM

When you edit the event again, this drop down seems like it is unset, even though it is set to 1.

Screenshot 2024-09-18 at 1 57 14 PM

davidmpickett commented 1 month ago

@FranECross @jilladams @mmiddaugh

Screenshot 2024-09-18 at 2 21 33 PM

Agile6MSkinner commented 1 month ago

Thank you for getting to the bottom of this. Per conversation with @mmiddaugh and @EWashb in helpdesk sync today, this resulted in a content build failure. @omahane or @davidmpickett, can we pursue the solution in this drupal issue?

davidmpickett commented 1 month ago

@Agile6MSkinner Can you provide more specifics on the Content Build failure? I don't see how this could issue could create a failure since it is purely a user-facing display issue

Agile6MSkinner commented 1 month ago

I can't, and in fact there's a chance I'm misspeaking there. @EWashb could you elaborate?

jilladams commented 1 month ago

~@Agile6MSkinner I think I'd mentioned that during planning but I was wrong. I was thinking of the issue where you could end up saving an event with an empty Location field (#17426 ) and that broke things.~

~In this case, recurrence is actually getting set, per Dave's note, it's just a weird UX. Looking over his notes, does seem like if @FranECross wants to pursue us fixing the underlying issue and upstreaming it to the module maintainers, we could, but agree that trying to inject better help text is prob not the best way forward, given the limitations of where it would appear.~

~I am wrong, this issue did break content-build, in Erika's linked Slack thread below.~

davidmpickett commented 1 month ago

Moving back to backlog

EWashb commented 1 month ago

failure

The context is in this thread:

https://dsva.slack.com/archives/CT4GZBM8F/p1726762852899099

Cc @Agile6MSkinner

jilladams commented 1 month ago

After reviewing that thread with DaveP, we determined the Event that broke prod was not a recurring event. That means this issue is not related to the prod outage and so far remains just an editor clarity / UX issue.

We've cut a new ticket, #19284 to track the Events related issue that led to the outage yesterday.