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

Implement next calculated event fixes from spike findings (#10176) #10716

Closed jtmst closed 11 months ago

jtmst commented 2 years ago

Description

Follow up to spike: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/10176

Recommendations to move forward are as follows:

In order to address the event staleness issue, we need to move logic that filters out past events to the browser. Currently graphQL handles that filtering, however this only occurs on daily build. Logic needs to be added to src/site/layouts/health_care_region_page.drupal.liquid and the associated liquid.js helper file to ensure that no past events are in the data that the featured event component pulls from.

Increase the limit for future events pulled to allow for more coverage of future events by health care region. Most regions have 10 or fewer future events, but the top 10 health care regions have more. As of today, the top 3 health care regions have more than 100 future events, with the top region (puget sound health care) having 722.

Increasing the limit to 1000 gives the leeway necessary to account for these outliers. I do not recommend removing the limit entirely as a region with a large number of recurring events could result in a payload that hinders performance

Acceptance Criteria

CMS Team

Please check the team(s) that will do this work.

jilladams commented 2 years ago

One thing I didn't think to ask on the SPIKE ticket: do we expect this to affect performance? (thinking: browser + 1000 events, etc)

jtmst commented 2 years ago

We discussed performance and didn't think performance would be dramatically affected, especially considering the very low number of facilities that actually have more than 100 future events.

jilladams commented 2 years ago

Perfect, thx!

wesrowe commented 1 year ago

@randimays, this ticket has been sitting in Refined for quite a while. It might be worth using a little bandwidth to try getting your head around it. (Unfortunately a spike is what led to this ticket, so I can't recommend doing another one.)

randimays commented 1 year ago

Dropping in some notes/observations here for pre-refinement:

TLDR for the two below collapsed sections:

I can't reproduce any of the problems commented on #10176. Were these fixed, or do we need to find facilities for which events are displaying incorrectly?

Featured events on Minneapolis home page

Specific date, Date range and Past events are all working as expected. I also checked two other facilities and all 4 filters were working as expected.

I checked this page Aug 10 at 12:51 PM CST and the featured event is for tomorrow.

Screenshot 2023-08-10 at 12 51 19 PM



I checked this page Aug 10 at 12:51 PM CST and upcoming events also seem to be correct.

Screenshot 2023-08-10 at 12 56 15 PM
Filtering for Aug 16 2022

In the original spike (#10176), Ryan indicated that Aug 16 2022 - Aug 16 2022 shows no results. It currently shows the one event on that day.

Screenshot 2023-08-10 at 2 36 20 PM



Filtering by All upcoming shows upcoming events as expected.

Screenshot 2023-08-10 at 2 37 25 PM



Events for Aug 8 2023 filtered by Date range:

Screenshot 2023-08-10 at 2 39 20 PM



These both show in Past events:

Screenshot 2023-08-10 at 2 39 28 PM

Remaining questions/comments

In the original spike (#10176), Ryan K. suggested we create a React widget for handling Featured Events. This suggestion is not part of this implementation ticket. Was that intentional?

Preliminary thought is that we need to move as much time calculation as possible to the browser. For example, the "Featured Event" is currently statically generated. This will always be delayed. It should likely be converted to a React widget that can calculate event timing relative to the exact moment a veteran loads the page.


Ryan's emphasis is on moving as much time calculation as possible to the browser. I don't know right now whether we have any Facility Event code in vets-website, but is this suggesting we create a React widget for all of it?

This description in the original spike (#10176) seems to point to an entirely React solution.

"Weekend staleness" - We need to consider the implications of calculating dates during the static build, and the timing concerns of doing so. For example, we don't build over the weekend, so the content that is live on a Sunday will have been generated on Friday. This could throw off dates.

jilladams commented 1 year ago

I think it'll help to name the flavors of event problems. So: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/1318

  1. Event list: Events in progress should show as Upcoming until the event is complete - fixed, 6/2023 (Daniel did it). #10307 In progress events don't become "Past" until the end time has passed. That addresses 1 of Ryan's original "August 16" bugs, where a now event didn't show as Upcoming, or as Past.
  2. Event list: Saturday event will show as Upcoming until the next static build on Monday. I think this will not repro, in light of #10307 changes, but we'd need to prove it
  3. Featured Event: a morning event that is Featured will stay Featured until the next static build, at night. -- we can verify this tomorrow (Fri 8/11) on https://www.va.gov/minneapolis-health-care/, at noon CT. At that time, the Featured Event you screenshotted will be in the past, but probably will still show as Featured.
    • Ticketed on its own as #10200 . On that, we said this ticket might fix, but we don't know. Need to have mock data to work with it.
  4. Featured Event: a Saturday event that is featured will show as Featured until the next static build on Monday. -- would need events data to test it (we could do that in a Tugboat maybe?)
    • Same issue, under the hood, #10200 , just a different timeframe in question.
  5. Event list: Specific date/custom date range filters don't show correct events: I can repro this today, screenshots in an accordion below -- I think this is happening due to recurrences being a weird data thing. Ryan's example was also related to a recurrence. (the yin/yang arrow circle is the indicator)
**5. Specific date filter doesn't show correct event - screenshots** ## Past events, showing 8/10 event The 8/10 event is a recurrence, which makes everything weird. ![Screenshot 2023-08-10 at 4 43 25 PM](https://github.com/department-of-veterans-affairs/va.gov-cms/assets/85581471/da722311-aa3e-4e2a-8d55-0e6c919f28c6) ## Specific date: 8/10, shows no events https://www.va.gov/minneapolis-health-care/events/?selectedOption=specific-date&startDateDay=10&startDateMonth=08&startDateYear=2023 ![Screenshot 2023-08-10 at 4 43 16 PM](https://github.com/department-of-veterans-affairs/va.gov-cms/assets/85581471/bfc0abdf-551f-47a5-b264-d2e790481b6f) ## Custom date range: 8/10, shows no events ![Screenshot 2023-08-10 at 4 44 37 PM](https://github.com/department-of-veterans-affairs/va.gov-cms/assets/85581471/d81d42a7-bb32-4a49-bdea-273002145aea)

Other stuff

Random thoughts:

  1. Dave wants to deprecate Featuring on Events altogether: #1318 Plan of record is:
    • Facilities figure out a better way to handle Featuring Stories
    • When that is ship-ready (no time soon), deprecate the old featuring method and Events can't be featured anymore.
    • So: we likely won't invest in fixing anything related to Featured Events, in light of this.
  2. limit of passing 10 events to the FE -- I don't understand why this matters (though I'm sure it does), and we don't have a good explanation I can find on the SPIKE or here. Randi, any ideas why this would make a difference? If so, let's spell it out.
randimays commented 1 year ago

Thanks @jilladams! I checked out the examples you gave this morning. The first one showing the recurring event is weird; the expectation is that a recurring event where the date has already passed wouldn't show, correct? Does the recurring part mean it happens every year on that date, every week ?? I haven't dug into the CMS to confirm.

Re: 1. - We should update the acceptance criteria in a team refinement, maybe. It still isn't totally clear to me what the outcome of this ticket should be (especially with regard to whether we're creating a React widget).

Re: 2. - I'm also not clear why we're increasing the limit. Looking at the GraphQL, it seems as though we're only fetching the first 10 future events. I'm not sure how this covers the outliers Josh referred to. I think I need more context.

I think, because this ticket is pretty stale, we need to revisit as a team and make the requirements more clear, and then we can do a deep-dive when we're actually working on this ticket. I haven't been able to reproduce many of the issues that are screenshot here or on the spike. I think we need test events and timely spelunking to make sense of this, and the picture is hazy to me because I've been doing a ton of context-switching this week.

jilladams commented 1 year ago

That makes sense, and I agree.

We have a KB that covers some top-line things about recurring events: https://prod.cms.va.gov/help/cms-basics/how-to-edit-an-event#cancel-or-reschedule. Basics are you can set recurrence by week / day / month, on a specific day of the week. (You can't do complex recurrence like every 4th of the month, or every other, etc.)

Screenshot 2023-08-11 at 9 43 08 AM

Then once you have recurrences, you can modify them within the CMS:

Screenshot 2023-08-11 at 9 44 25 AM

Screenshot 2023-08-11 at 9 46 05 AM

The data for recurrences is sort of weird, and we've had to massage it a lot. Each recurrence is rooted to the original occurrence, so if, say: event recurrence is monthly on the first Monday, starting 8/1: starting 8/2, the system naturally wants to put all the recurrences in the past. So we've had to doa lot of work to help the front-end parse recurrences specifically. History of most of those changes is in this epic: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/1956. Recurrences are the most fragile / brittle part of Events handling, imo.

And example recurring event you can see, here: https://staging.cms.va.gov/minneapolis-health-care/events/57181

randimays commented 1 year ago

@jilladams Thanks for the additional context, that helps. That does shed a little bit of light on expanding the events limit, though. For instance, there was a recurring event yesterday (Aug 10). If the next event takes place September 10, perhaps we aren't fetching it in the list of 10 upcoming events, so a person digging through past events would think "Ok, I see this is recurring - when is the next one?" Not sure that's the main case for expanding the limits, though.

jilladams commented 11 months ago

Additional PR: https://github.com/department-of-veterans-affairs/content-build/pull/1814

chriskim2311 commented 11 months ago

Verified in prod that we are pulling in all events for a specific health care region. If no featured event is selected we are using the most recent event and is working as expected. Closing!

jilladams commented 11 months ago

@chriskim2311 only because the verification note was specific to healthcare regions: can you confirm that the behavior applies to all Event listing pages? (e.g. Outreach Events, which isn't specific to a healthcare region.)

jilladams commented 11 months ago

(Though Outreach Events don't have Featured events.)

chriskim2311 commented 11 months ago

Thanks for the comment Jill. The code changes were specific for the health care region pages and the single event that shows on those pages. Other event pages were not affected. Outreach events is working as expected and are updated based on time of day since they use a vets-website widget.