Relocate #get_facility_timezone to appointments_service.rb #16720
Conversation
| @@ -297,17 +287,6 @@ def get_clinic_memoized(location_id, clinic_id) | |||
| end | |||
| memoize :get_clinic_memoized | |||
|
|
|||
| def get_facility_memoized(location_id) | |||
There was a problem hiding this comment.
I also relocated this function as #get_facility_timezone depended on it as well. The appointments service already had get_facility so I'm not sure why the memoized version wasn't already in there as well
| @@ -460,26 +504,7 @@ def convert_utc_to_local_time(date, tz) | |||
| end | |||
| end | |||
|
|
|||
| # Returns the facility timezone id (eg. 'America/New_York') associated with facility id (location_id) | |||
| def get_facility_timezone_memoized(facility_location_id) | |||
There was a problem hiding this comment.
I made both get_facility_timezone_momoized and get_facility public as they're used outside of the service. Lmk if there are any considerations I'm missing by doing that
There was a problem hiding this comment.
I don't think you necessarily need to do this. I think ideally the fields that use these functions would be returned in the appts list. For example, get_facility_timezone_memoized is always used to set facility_timezone but if that data was just available in the appts list returned from the service then the downstream controllers and serializers wouldn't need access to get_facility_timezone_memoized. Similar logic goes for get_facility. That being said I'm fine with doing this for now and making a new ticket to follow up with the concept above if get_facility is fairly complex
There was a problem hiding this comment.
Hmm do you think there's any concern with automatically doing another upstream fetch? Is there any scenario where we'd want the appointment info separate from facility info? I'm happy to make that change if you'd rather just have it done in this pr
There was a problem hiding this comment.
Let's just leave it as is, but please make another ticket to follow up with the engineers from the appts team about baking this into the service response because I think at the end of all of this migration stuff the ideal situation is that the controllers (web and mobiles) are doing little to nothing other than serializing the data received from the service
Summary
In an ongoing effort to keep the business logic of appointments the same between Web and Mobile, this PR relocates the
get_facility_timezoneto the appointments_serviceRelated issue(s)
department-of-veterans-affairs/va-mobile-app#8571
Testing done
Existing tests cover the use cases
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
Acceptance criteria
Requested Feedback
(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?