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

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
281 stars 197 forks source link

[Spike] - Verify past appointment fetch logic #89552

Closed JunTaoLuo closed 1 week ago

JunTaoLuo commented 1 month ago

Background/Request

It looks like the logic for retrieving the list of past appointment first fetch "cancelled" appointments here which is defined here.

Later on, these fetched appointments are then filtered to remove all cancelled appointments via isValidPastAppointment in the selectors

I think it's worth evaluating whether we can simplify this logic by specifying not to retrieve cancelled appointments in the original fetch.

Goal

Determine if we can remove the filter for cancelled appointments in isValidPastAppointment by simply not fetching cancelled appointments in the first place when querying for past appointments.

Requirements to Consider


Tasks

Time Box

_ hours

Definition of Done


How to configure this issue

Bren22va commented 1 month ago

Hey team! Please add your planning poker estimate with Zenhub @cferris32 @jenniemc @JunTaoLuo @ryanshaw @simiadebowale @vbahinwillit

Bren22va commented 1 month ago

Please add your planning poker estimate with Zenhub @vbahinwillit

vbahinwillit commented 2 weeks ago

Question: Do we need to fetch cancelled appointments only to filter them out when retrieving past appointments?

Answer: No, we do not need to fetch 'canceled' past appointments when displaying the current past appointments page. There used to be a past appointment layout that displayed canceled appointments. This design decision boils down to:

I do not know what the metrics are for how users use the past appointments page but here are some use cases.

If the user navigates to the past appointments page from the upcoming appointments page and doesn't change the initial query (Past 3 months), advantage goes to select all appointment statuses at one time since all appointments statuses were returned when displaying the upcoming appointments page thus resulting in 1 api call.

If the user never goes to the past appointments page, advantage goes to select appointments by status since canceled appointments would not have been retrieved when displaying the upcoming appointments pages thus resulting in less data returned in the api call.

If the user visits the past appointments page and changes the initial query, advantage goes to select appointments by status since each date range change requires a new api call.

Will making this change simplify the code? Yes. Refactoring code is always a good thing IMO.

JunTaoLuo commented 2 weeks ago

Thanks @vbahinwillit for looking into this. I want to clarify one bit which is that the initial query logic for upcoming appointments is a little different.

Upcoming appointments calls fetchFutureAppointments which fetches appointments ('booked','arrived', 'fulfilled','cancelled') for the past 30 days and requests ('proposed','cancelled',) for the past 120 days. Both of these are different from the logic used for past appointments which calls fetchPastAppointments that initially fetches appointments ('booked','arrived', 'fulfilled','cancelled') for the past 3 months.

Basically I don't see any overlap in logic between upcoming and past appointment fetches at all since past 120 days != past 3 months anyways.

vbahinwillit commented 2 weeks ago

@JunTaoLuo - You are correct. I could have sworn that the initial request for appointments on the upcoming page was populating the past appointments collection as well but that actually doesn't make sense. So, removing the 'cancelled' appointments from the 'fetchPastAppointments' request should work just fine.

ldelacosta commented 1 week ago

@simiadebowale @JRRoof - I'm going to close this ticket. We could look to into this spike to see how we can improve fetching the Past appointment list.