Enable org queues to use task pagination API #11213

lowellrex · 2019-06-25T20:38:42Z

Connects #11054. Enables back-end pagination for certain organizations (VSOs, VLJ support staff, hearing management branch) when the current user has the use_task_pages_api feature toggle on. Still needs some cosmetic love, but it's far enough along that I'd like some feedback on how this approach can be improved.

codeclimate · 2019-06-25T20:41:39Z

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

View more on Code Climate.

lowellrex · 2019-06-25T21:54:16Z

app/controllers/organizations/tasks_controller.rb

@@ -17,6 +17,8 @@ def index
  private

  def tasks
+    return [] if queue_config[:use_task_pages_api]


If we are using the back-end pagination API then we should not bother with returning the tasks here since we will use the ones from the queue config for the first page and the API for all subsequent pages.

lowellrex · 2019-06-25T21:54:44Z

app/models/organization.rb

@@ -24,6 +24,10 @@ def show_regional_office_in_queue?
    false
  end

+  def use_task_pages_api?
+    false
+  end


By default organizations will continue using front-end pagination.

could we make this use the FeatureToggle rather than hardcoded bool?

FeatureToggle.enabled?(:use_task_pages_api)

FeatureToggle's are enabled on a per-user basis and this flag allows us to enable it on a per-organization basis which gives us another level of control as we roll this out. When this code is release live nothing should change. No requests will use the task pages API. However, we can roll this out in a phased fashion as follows:

We can enable the feature toggle for a few members of teams with this flag set to true (VSOs, VLJ support, Hearings management) so they can test that it works and doesn't have any unexpected bugs without affecting how the rest of that team is using the task queue.

After individual folks have confirmed that the task pages API is working well we can roll it out to a whole team that needs the task pages API (we'd actually need to move the enabled? function call inside the organization's use_task_pages_api? function definition) without affecting any of the teams who are not experiencing team queue loading problems.

After we've caught some bugs with heavy full-team use we can roll it out to all organization queues (by removing this function entirely and making it the default).

This function at the organization enables phase 2 which I think is fairly important. Team queues work for most teams right now, so I'd love to limit the pagination changes to just those teams that are experiencing team queue loading problems before rolling the changes out to everybody.

lowellrex · 2019-06-25T21:55:31Z

app/models/organizations/hearings_management.rb

+  def self.singleton
+    HearingsManagement.first || HearingsManagement.create(name: "Hearings Management", url: "hearings-management")
+  end
+


Moving the singleton definition to the top of the class definition so that it matches all of the other organizations.

HearingAdmin team could use same change.

Totally agree. Going to postpone rearranging that class to a later PR so that this changeset stays small.

lowellrex · 2019-06-25T21:55:56Z

app/models/queue_config.rb

    }
  end

  private

+  def use_task_pages_api?
+    # return true


My manual toggle for easy testing locally. Will remove before this is merged.

lowellrex · 2019-06-25T21:57:00Z

app/models/queue_config.rb

+
+    # Only return tasks in the configuration if we are using it to populate the first page of results.
+    # Otherwise avoid the overhead of the additional database requests.
+    tasks = use_task_pages_api? ? serialized_tasks_for_user(task_pager.paged_tasks, user) : []


Inverse of the optimization above. Don't make additional requests to the database if we are just going to be using the tasks we get from some other method.

lowellrex · 2019-06-25T21:57:45Z

app/models/queue_config.rb

-      label: format(COPY::ORGANIZATIONAL_QUEUE_PAGE_UNASSIGNED_TAB_TITLE, tasks.count),
-      name: name,
+      label: COPY::ORGANIZATIONAL_QUEUE_PAGE_UNASSIGNED_TAB_TITLE,
+      name: Constants.QUEUE_CONFIG.UNASSIGNED_TASKS_TAB_NAME,


Was able to consolidate all this duplicated logic in the new attach_tasks_to_tab method.

lowellrex · 2019-06-25T21:59:58Z

client/app/components/TablePagination.jsx

      currentPage,
+      numberOfPages,


Pass numberOfPages as an argument instead of computing it from the paginatedData argument.

lowellrex · 2019-06-25T22:01:09Z

client/app/components/TablePagination.jsx

-    // Create the entire summary
-    const paginationSummary = `Viewing ${currentCaseRange} of ${totalCasesCount} total cases`;
+
+    const paginationSummary = `Viewing ${beginningCaseNumber}-${endingCaseNumber} of ${totalCasesCount} total cases`;


No significant change here other than we were able to replace the paginatedData argument (which relied on an array of array of tasks to determine this summary) with the casesPerPage argument (which is a number).

Not sure if you looked at the Pagination component? I did something similar there for the /jobs pagination.

It's fairly easy to replace the only place we use TablePagination with the Pagination component so I'm going to go ahead and do that instead.

lowellrex · 2019-06-25T22:02:31Z

client/app/queue/OrganizationQueue.jsx

        { tabConfig.allow_bulk_assign && <BulkAssignButton /> }
        <TaskTable
-          customColumns={cols}
+          key={tabConfig.name}


Need to set a key on each TaskTable so that they can have their own state (React ended up sharing state across the sibling components without this key).

lowellrex · 2019-06-25T22:03:17Z

client/app/queue/OrganizationQueue.jsx

+          tasksPerPage={config.tasks_per_page}
+          numberOfPages={tabConfig.task_page_count}
+          totalTaskCount={tabConfig.total_task_count}
+          taskPagesApiEndpoint={tabConfig.task_page_endpoint_base_path}


Since we're adding 5 new arguments here maybe it makes sense to just make this a new component? What're y'all's thoughts?

a PaginatedTaskTable makes sense to me. I think the nonComp feature still uses the TaskTable so otherwise we'd want to make sure we don't introduce regressions.

If we believe we will ever want to use the PaginatedTaskTable in other views, I have a slight preference for creating it in its own file in client/app/queue/components.

lowellrex · 2019-06-25T22:03:58Z

client/app/queue/QueueTable.jsx

-    el.setAttribute('tabindex', '-1');
-  }
-  el.focus();
-};


I got sick of the browser taking control of the scroll when I was testing this. Happy to put it back if we want, but I really hate this.

hate is a good motivator.

lowellrex · 2019-06-25T22:05:04Z

client/app/queue/QueueTable.jsx

  }

+  requestNewPage = (newPage) => {


This is the heart of the new behaviour. We make an API request if we are using the back-end pagination API.

This method seems more than a little hacky to me. Very open to ideas for how to improve it (Maybe it should be injected from OrganizationQueue? How would we set local state? Maybe it should just be a separate component?).

This was the part I struggled with on the /jobs paginator too. I am not sure there is an elegant solution. In my case, I wasn't trying to adhere to SPA pattern so just reloaded the whole page.

https://github.com/department-of-veterans-affairs/caseflow/blob/master/client/app/asyncableJobs/pages/JobsPage.jsx#L210

This looks readable to me, which is what I would care about. The only gotchas are the off-by-one with index and page counts. You may also want to guard against clicking on the current page (not sure if that affects here as it did for me on /jobs).

I agree that this is readable, which is most important. The comments in the code are good. I think this general approach is definitely fine for now.

lowellrex · 2019-06-25T22:07:24Z

client/app/queue/utils.js

@@ -52,79 +52,85 @@ export const prepareMostRecentlyHeldHearingForStore = (appealId, hearing) => {
  };
 };

+const taskAttributesFromRawTask = (task) => {


Separating these functions into two steps each so we can get arrays for our task pages instead of the hashes that are sent to the global redux store.

lowellrex · 2019-06-25T22:08:00Z

app/models/queue_config.rb

      description: format(COPY::ALL_CASES_QUEUE_TABLE_TAB_DESCRIPTION, organization.name),
      columns: [
        Constants.QUEUE_CONFIG.CASE_DETAILS_LINK_COLUMN,
        Constants.QUEUE_CONFIG.ISSUE_COUNT_COLUMN,
        Constants.QUEUE_CONFIG.APPEAL_TYPE_COLUMN,
        Constants.QUEUE_CONFIG.DOCKET_NUMBER_COLUMN
      ],
-      task_group: Constants.QUEUE_CONFIG.TRACKING_TASKS_GROUP,


We never ended up using the task_group element.

va-bot · 2019-06-26T14:18:22Z

	1 Warning
⚠️	This is a Big PR. Try to break this down if possible.

Generated by 🚫 Danger

pkarman

I like the direction this is going!

pkarman · 2019-06-26T15:02:07Z

app/models/organization.rb

@@ -24,6 +24,10 @@ def show_regional_office_in_queue?
    false
  end

+  def use_task_pages_api?
+    false
+  end


could we make this use the FeatureToggle rather than hardcoded bool?

FeatureToggle.enabled?(:use_task_pages_api)

pkarman · 2019-06-26T15:02:21Z

app/models/organizations/colocated.rb

@@ -8,4 +8,8 @@ def self.singleton
  def next_assignee(options = {})
    ColocatedTaskDistributor.new.next_assignee(options)
  end
+
+  def use_task_pages_api?
+    true


same thing here about feature toggle.

pkarman · 2019-06-26T15:03:01Z

app/models/organizations/hearings_management.rb

-  def self.singleton
-    HearingsManagement.first || HearingsManagement.create(name: "Hearings Management", url: "hearings-management")
+  def use_task_pages_api?
+    true


feature toggle?

pkarman · 2019-06-26T15:03:31Z

app/models/organizations/vso.rb

-class Vso < Representative; end
+class Vso < Representative
+  def use_task_pages_api?
+    true


feature toggle?

pkarman · 2019-06-26T15:04:01Z

app/models/queue_config.rb

    }
  end

  private

+  def use_task_pages_api?
+    # return true
+    FeatureToggle.enabled?(:use_task_pages_api) && organization.use_task_pages_api?


pkarman · 2019-06-26T15:07:21Z

app/models/task_pager.rb

@@ -46,14 +46,22 @@ def paged_tasks
  #   tasks
  # end

+  def task_page_count


does the kaminari total_pages method work on tasks_for_tab here?

pkarman · 2019-06-26T15:10:46Z

client/app/components/TablePagination.jsx

-    // Create the entire summary
-    const paginationSummary = `Viewing ${currentCaseRange} of ${totalCasesCount} total cases`;
+
+    const paginationSummary = `Viewing ${beginningCaseNumber}-${endingCaseNumber} of ${totalCasesCount} total cases`;


Not sure if you looked at the Pagination component? I did something similar there for the /jobs pagination.

pkarman · 2019-06-26T15:12:02Z

client/app/queue/OrganizationQueue.jsx

+          tasksPerPage={config.tasks_per_page}
+          numberOfPages={tabConfig.task_page_count}
+          totalTaskCount={tabConfig.total_task_count}
+          taskPagesApiEndpoint={tabConfig.task_page_endpoint_base_path}


a PaginatedTaskTable makes sense to me. I think the nonComp feature still uses the TaskTable so otherwise we'd want to make sure we don't introduce regressions.

pkarman · 2019-06-26T15:12:24Z

client/app/queue/QueueTable.jsx

-    el.setAttribute('tabindex', '-1');
-  }
-  el.focus();
-};


hate is a good motivator.

pkarman · 2019-06-26T15:16:49Z

client/app/queue/QueueTable.jsx

  }

+  requestNewPage = (newPage) => {


This was the part I struggled with on the /jobs paginator too. I am not sure there is an elegant solution. In my case, I wasn't trying to adhere to SPA pattern so just reloaded the whole page.

https://github.com/department-of-veterans-affairs/caseflow/blob/master/client/app/asyncableJobs/pages/JobsPage.jsx#L210

This looks readable to me, which is what I would care about. The only gotchas are the off-by-one with index and page counts. You may also want to guard against clicking on the current page (not sure if that affects here as it did for me on /jobs).

pkarman · 2019-06-26T20:45:38Z

client/app/queue/QueueTable.jsx

@@ -286,6 +286,15 @@ export default class QueueTable extends React.PureComponent {
    return paginatedData;
  }

+  componentDidMount = () => {
+    this.setState({
+      tasksFromApiForPage: {


as I'm reading this, the idea seems to be cache each paged task set on the browser, rather than re-fetching on each page click?

I can see the attraction of that for performance reasons. My only concern would be once sorting/filtering are enabled, that the cache invalidation will be tricky. Also, if someone updates a task in a different browser, that the user in this browser won't see the latest when they page "back" to that task set.

Correct. I think we'll have to handle caching some other way when we do sorting/filtering.

And you're totally right about the different browser window/tab causing cached tasks to be in the incorrect state (though we already have this now with entirely front-end pagination).

Happy to back out the caching of paged tasks if you think that's better.

I think the caching falls under the category of premature optimization. Let's back it out. In theory the pagination itself should make things perform well enough that we don't need caching -- time will tell.

pkarman

love it. 🚢

kevmo

Looks good to go!

Enable org queues to use task pagination API

e57d639

lowellrex self-assigned this Jun 25, 2019

lowellrex and others added 3 commits June 25, 2019 17:19

Actually requesting pages from the API now

9849fdd

Add key to prevent sharing state across sibling components

3a050cb

Merge branch 'master' into lowell/11054_frontend_pagination_table

65769d3

lowellrex commented Jun 25, 2019

View reviewed changes

Rename task collections for clarity

e36abdb

lowellrex commented Jun 25, 2019

View reviewed changes

lowellrex and others added 5 commits June 25, 2019 18:10

Merge branch 'master' into lowell/11054_frontend_pagination_table

420d027

Update TablePagination test

e5cba68

Satisfy eslint

6aca957

Merge branch 'master' into lowell/11054_frontend_pagination_table

ef6c091

Update test because shape of config changed

219cb22

pkarman reviewed Jun 26, 2019

View reviewed changes

lowellrex added 2 commits June 26, 2019 12:02

Use built-in total_pages from kaminari

3ea371d

Replace TablePagination with Pagination

0650e67

lowellrex and others added 5 commits June 26, 2019 15:42

Merge branch 'master' into lowell/11054_frontend_pagination_table

6d268dc

Fix sloppy merge

322402e

Use QueueTable directly in OrganizationQueue

25090e4

Implement component-level caching

99c52ba

Increase test coverage

f631fa5

pkarman reviewed Jun 26, 2019

View reviewed changes

lowellrex and others added 3 commits June 27, 2019 08:52

Merge branch 'master' into lowell/11054_frontend_pagination_table

8d0172b

Remove caching

38722a7

Remove manual testing toggle

eccf405

pkarman approved these changes Jun 27, 2019

View reviewed changes

kevmo approved these changes Jun 27, 2019

View reviewed changes

Merge branch 'master' into lowell/11054_frontend_pagination_table

217f45e

lowellrex 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 Jun 27, 2019

va-bot added 2 commits June 27, 2019 11:20

Merge branch 'master' into lowell/11054_frontend_pagination_table

6d724d9

Merge branch 'master' into lowell/11054_frontend_pagination_table

7e872f9

va-bot merged commit 9495744 into master Jun 27, 2019

va-bot deleted the lowell/11054_frontend_pagination_table branch June 27, 2019 18:00

This was referenced Jun 27, 2019

Include tasks as part of the organization queue configuration #11049

Closed

Re-enable table filter names for org queues #11239

Merged

Enable org queues to use task pagination API #11213

Enable org queues to use task pagination API #11213

Conversation

lowellrex commented Jun 25, 2019 • edited

codeclimate bot commented Jun 25, 2019 • edited

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

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

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

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

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

Choose a reason for hiding this comment

va-bot commented Jun 26, 2019

pkarman 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

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

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

pkarman left a comment

Choose a reason for hiding this comment

kevmo left a comment

Choose a reason for hiding this comment

lowellrex commented Jun 25, 2019 •

edited

codeclimate bot commented Jun 25, 2019 •

edited