Avoid VACOLS calls on already scheduled veterans page - Part 1 #13668

ferristseng · 2020-03-11T17:58:22Z

Description

Fixes some performance issues on the already scheduled veterans page that were reported to batteam

Create special serializer that returns only the necessary attributes for the already scheduled veterans table
Fix issue where hearing time on already scheduled veterans table wasn't accurate
- Refactor daily docket and already scheduled veterans table to use shared HearingTime component
Refactor time-related hearing code to concern
Fix some prop type warnings
Ensure an attribuet called aod is always passed to the frontend

…gs table row

ferristseng · 2020-03-11T17:59:02Z

app/serializers/hearings/hearing_serializer.rb

@@ -4,10 +4,9 @@ class HearingSerializer
  include FastJsonapi::ObjectSerializer
  include HearingSerializerBase

+  attribute :aod, &:aod?


The frontend expects a field named aod, but it wasn't being returned before for AMA hearings

ferristseng · 2020-03-11T18:00:14Z

client/app/util/DateUtil.js

 };

-export const getTimeInDifferentTimeZone = (date, timeZone) => {


Removing getTime and getTimeInDifferentTimeZone since they aren't used. I also think the server is responsible for doing a lot of the time conversions

Is the server converting the time to the timezone of the hearing location? What if that differs from the timezone in which the user is currently viewing Caseflow? That could lead to incorrect time displays depending on if the user is in a different TZ than the hearing, right?

Is the server converting the time to the timezone of the hearing location?

Yep, the time the server sends down is in the timezone of the regional office (although it sends the time as a string).

What if that differs from the timezone in which the user is currently viewing Caseflow? That could lead to incorrect time displays depending on if the user is in a different TZ than the hearing, right?

It always show the time in the timezone of the RO. I think this makes sense, since the people viewing this page should either be in the RO of the hearing or in the CO (users are judge, representative, or hearing coordinator). This is getting blurrier though now because hearings can be conducted across state lines.

codeclimate · 2020-03-11T18:03:07Z

Code Climate has analyzed commit be866fd and detected 0 issues on this pull request.

View more on Code Climate.

…nge expected)

… data that needs to be cached

jcq

I think this is great — huge performance increases that should come of this, and I love that it lets us remove the artificial limit on entries returned (this likely will open up lots more possibilities for UI improvements, like fast in-browser pagination, etc).

What is the best way to test it? I was looking at it locally, but I'm not sure which regional offices are supposed to have decent amounts of seed data. Are there specs that might populate in tons of data to be able to gauge performance?

jcq · 2020-03-11T23:01:46Z

client/app/hearings/components/HearingTime.jsx

+  );
+  const coTime = getDisplayTime(hearing.centralOfficeTimeString, 'America/New_York');
+
+  if (hearing.readableRequestType === 'Central') {


I only see a couple of differences between what's being rendered in this block and the main one... might be better to just have one return and simply add conditionals for the couple of places where they diverge?

jcq · 2020-03-11T23:02:37Z

client/app/hearings/components/assignHearings/AssignHearingsFields.jsx

 import DocketTypeBadge from '../../../components/DocketTypeBadge';

-export const HearingTime = ({ hearing, isCentralOffice }) => {


Huzzah to splitting this out into its own file!

jcq · 2020-03-11T23:02:58Z

client/app/hearings/components/assignHearings/AssignHearingsFields.jsx

@@ -87,7 +66,7 @@ export const HearingDocketTag = ({ hearing }) => {
 HearingDocketTag.propTypes = {
  hearing: PropTypes.shape({
    docketName: PropTypes.string,
-    docketNumber: PropTypes.string
+    docketNumber: PropTypes.number


Love fixing up PropTypes. :)

jcq · 2020-03-11T23:08:13Z

client/app/util/DateUtil.js

 };

-export const getTimeInDifferentTimeZone = (date, timeZone) => {


Is the server converting the time to the timezone of the hearing location? What if that differs from the timezone in which the user is currently viewing Caseflow? That could lead to incorrect time displays depending on if the user is in a different TZ than the hearing, right?

ferristseng · 2020-03-12T14:56:46Z

What is the best way to test it? I was looking at it locally, but I'm not sure which regional offices are supposed to have decent amounts of seed data. Are there specs that might populate in tons of data to be able to gauge performance?

Unfortunately, there isn't a great way to test the performance of this locally without modifying the seeds.rb file. That was a big part of why I tested performance directly in production, and just tested for basic functionality locally.

These RO's should have data (but probably enough to be able to gauge performance):

Central
St. Petersburg, FL
Columbia, SC
St. Louis, MO
Oakland, CA
Phoenix, AZ

If you do want to go with the route of modifying seeds.rb, you could potentially modify this method

caseflow/db/seeds.rb

Lines 288 to 317 in dcb3886

    
           def create_hearing_days 
        
             user = User.find_by(css_id: "BVATWARNER") 
        
             # Set the current user var here, which is used to populate the 
        
             # created by field. 
        
             RequestStore[:current_user] = user 
        
             %w[C RO17 RO19 RO31 RO43 RO45].each do |ro_key| 
        
               (1..5).each do |index| 
        
                 day = HearingDay.create!( 
        
                   regional_office: (ro_key == "C") ? nil : ro_key, 
        
                   room: "1", 
        
                   judge: User.find_by_css_id("BVAAABSHIRE"), 
        
                   request_type: (ro_key == "C") ? "C" : "V", 
        
                   scheduled_for: Time.zone.today + (index * 11).days, 
        
                   created_by: user, 
        
                   updated_by: user 
        
                 ) 
        
                 case index 
        
                 when 1 
        
                   create_ama_hearing(day) 
        
                 when 2 
        
                   create_case_hearing(day, ro_key) 
        
                 when 3 
        
                   create_case_hearing(day, ro_key) 
        
                   create_ama_hearing(day) 
        
                 end 
        
               end 
        
             end

and have it fill up each hearing day, as opposed to just generating a 1-2 hearings for each day.

jcq

Seems like it works locally, and seems like good code improvements in addition to GREAT performance improvements!

jcq · 2020-03-11T23:17:09Z

app/jobs/warm_bgs_caches_job.rb

@@ -70,14 +70,19 @@ def warm_participant_caches

  def warm_ro_participant_caches(ro_ids)
    start_time = Time.zone.now
+    start_range = Time.zone.today.beginning_of_day


Just for my own curiosity, which TZ would these be using? I haven't really looked at how we're storing/handling time data

These would be using the UTC TZ in prod. It's slightly different in UAT because we have been testing some TZ changes there.

The web server does not run under UTC, which is a whole other story 🙁

jcq · 2020-03-11T23:18:12Z

app/models/concerns/hearing_time_concern.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module HearingTimeConcern


Good refactoring

…#13644)" (#13670) This reverts commit 5a0833d.

…b.com/department-of-veterans-affairs/caseflow into ftseng-scheduled-veterans-performance

ferristseng added 6 commits March 11, 2020 11:39

move time-related code to a concern

4d27fcf

ensure an aod field gets returned by the API

ed64019

share HearingTime component with daily docket row and upcoming hearin…

97f0685

…gs table row

add specialized serializer to serialize hearings for a hearing day

75a9444

fix proptype warnings

40923d9

use new serializer class

004cbde

ferristseng commented Mar 11, 2020

View reviewed changes

ferristseng changed the title ~~Avoid VACOLS calls on already scheduled veterans page~~ Avoid VACOLS calls on already scheduled veterans page - Part 1 Mar 11, 2020

ferristseng added Product: caseflow-hearings Team: Tango 💃 labels Mar 11, 2020

ferristseng added 6 commits March 11, 2020 14:09

address code climate issues

369585d

add back necessary call

b7443c2

fix tests that use open_hearing_days_with_hearings_hash

c5ed81e

serialize appeal_id for tests that use it (no serious performance cha…

f3e31e2

…nge expected)

update cache job to call full hearing serializer since it touches the…

b82aa93

… data that needs to be cached

address codeclimate nested iterators

8469fcb

jcq reviewed Mar 11, 2020

View reviewed changes

ferristseng added 2 commits March 12, 2020 11:07

remove redundant code

25d3585

address linting issues

bb5b931

jcq approved these changes Mar 12, 2020

View reviewed changes

ferristseng added 4 commits March 12, 2020 18:06

Revert "Reduce default hearing day range size for index_with_hearings (…

13f169b

…#13644)" (#13670) This reverts commit 5a0833d.

Merge branch 'master' into ftseng-scheduled-veterans-performance

fc8e87e

fix lint errors from merge

a76c4f9

Merge branch 'ftseng-scheduled-veterans-performance' of https://githu…

be866fd

…b.com/department-of-veterans-affairs/caseflow into ftseng-scheduled-veterans-performance

ferristseng added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Mar 12, 2020

ferristseng merged commit 7b13dd6 into master Mar 12, 2020

pkarman deleted the ftseng-scheduled-veterans-performance branch March 31, 2020 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid VACOLS calls on already scheduled veterans page - Part 1 #13668

Avoid VACOLS calls on already scheduled veterans page - Part 1 #13668

ferristseng commented Mar 11, 2020

ferristseng Mar 11, 2020

ferristseng Mar 11, 2020

jcq Mar 11, 2020

ferristseng Mar 12, 2020

codeclimate bot commented Mar 11, 2020 •

edited

Loading

jcq left a comment

jcq Mar 11, 2020

jcq Mar 11, 2020

jcq Mar 11, 2020

jcq Mar 11, 2020

ferristseng commented Mar 12, 2020 •

edited

Loading

jcq left a comment

jcq Mar 11, 2020

ferristseng Mar 12, 2020

jcq Mar 11, 2020

		};

		export const getTimeInDifferentTimeZone = (date, timeZone) => {

		import DocketTypeBadge from '../../../components/DocketTypeBadge';

		export const HearingTime = ({ hearing, isCentralOffice }) => {

		@@ -0,0 +1,12 @@
		# frozen_string_literal: true

		module HearingTimeConcern

Avoid VACOLS calls on already scheduled veterans page - Part 1 #13668

Avoid VACOLS calls on already scheduled veterans page - Part 1 #13668

Conversation

ferristseng commented Mar 11, 2020

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 11, 2020 • edited Loading

jcq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferristseng commented Mar 12, 2020 • edited Loading

jcq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 11, 2020 •

edited

Loading

ferristseng commented Mar 12, 2020 •

edited

Loading