Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable org queues to use task pagination API #11213

Merged
merged 23 commits into from Jun 27, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e57d639
Enable org queues to use task pagination API
lowellrex Jun 25, 2019
9849fdd
Actually requesting pages from the API now
lowellrex Jun 25, 2019
3a050cb
Add key to prevent sharing state across sibling components
lowellrex Jun 25, 2019
65769d3
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 25, 2019
e36abdb
Rename task collections for clarity
lowellrex Jun 25, 2019
420d027
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 25, 2019
e5cba68
Update TablePagination test
lowellrex Jun 25, 2019
6aca957
Satisfy eslint
lowellrex Jun 25, 2019
ef6c091
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 26, 2019
219cb22
Update test because shape of config changed
lowellrex Jun 26, 2019
3ea371d
Use built-in total_pages from kaminari
lowellrex Jun 26, 2019
0650e67
Replace TablePagination with Pagination
lowellrex Jun 26, 2019
6d268dc
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 26, 2019
322402e
Fix sloppy merge
lowellrex Jun 26, 2019
25090e4
Use QueueTable directly in OrganizationQueue
lowellrex Jun 26, 2019
99c52ba
Implement component-level caching
lowellrex Jun 26, 2019
f631fa5
Increase test coverage
lowellrex Jun 26, 2019
8d0172b
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 27, 2019
38722a7
Remove caching
lowellrex Jun 27, 2019
eccf405
Remove manual testing toggle
lowellrex Jun 27, 2019
217f45e
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 27, 2019
6d724d9
Merge branch 'master' into lowell/11054_frontend_pagination_table
Jun 27, 2019
7e872f9
Merge branch 'master' into lowell/11054_frontend_pagination_table
Jun 27, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/organizations/tasks_controller.rb
Expand Up @@ -17,6 +17,8 @@ def index
private

def tasks
return [] if queue_config[:use_task_pages_api]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Rails.logger.debug("starting GenericQueue tasks")

# Temporarily limit hearings-management tasks to AOD tasks, because currently no tasks are loading
Expand Down
4 changes: 4 additions & 0 deletions app/models/organization.rb
Expand Up @@ -24,6 +24,10 @@ def show_regional_office_in_queue?
false
end

def use_task_pages_api?
false
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default organizations will continue using front-end pagination.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FeatureToggle.enabled?(:use_task_pages_api)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. 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.


def non_admins
organizations_users.includes(:user).non_admin.map(&:user)
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/organizations/colocated.rb
Expand Up @@ -8,4 +8,8 @@ def self.singleton
def next_assignee(options = {})
ColocatedTaskDistributor.new.next_assignee(options)
end

def use_task_pages_api?
true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here about feature toggle.

end
end
8 changes: 6 additions & 2 deletions app/models/organizations/hearings_management.rb
@@ -1,6 +1,10 @@
# frozen_string_literal: true

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HearingAdmin team could use same change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def can_bulk_assign_tasks?
true
end
Expand All @@ -9,7 +13,7 @@ def show_regional_office_in_queue?
true
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature toggle?

end
end
6 changes: 5 additions & 1 deletion app/models/organizations/vso.rb
@@ -1,3 +1,7 @@
# frozen_string_literal: true

class Vso < Representative; end
class Vso < Representative
def use_task_pages_api?
true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature toggle?

end
end
63 changes: 30 additions & 33 deletions app/models/queue_config.rb
Expand Up @@ -17,17 +17,22 @@ def initialize(args)
end

def to_hash_for_user(user)
serialized_tabs = tabs.each { |tab| tab[:tasks] = serialized_tasks_for_user(tab[:tasks], user) }

{
table_title: format(COPY::ORGANIZATION_QUEUE_TABLE_TITLE, organization.name),
active_tab: Constants.QUEUE_CONFIG.UNASSIGNED_TASKS_TAB_NAME,
tabs: serialized_tabs
tasks_per_page: TaskPager::TASKS_PER_PAGE,
use_task_pages_api: use_task_pages_api?,
tabs: tabs.map { |tab| attach_tasks_to_tab(tab, user) }
}
end

private

def use_task_pages_api?
# return true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FeatureToggle.enabled?(:use_task_pages_api) && organization.use_task_pages_api?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

end

def tabs
[
include_tracking_tasks_tab? ? tracking_tasks_tab : nil,
Expand All @@ -37,6 +42,22 @@ def tabs
].compact
end

def attach_tasks_to_tab(tab, user)
task_pager = TaskPager.new(assignee: organization, tab_name: tab[:name])

# 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) : []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


tab.merge(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

label: format(tab[:label], task_pager.total_task_count),
tasks: tasks,
task_page_count: task_pager.task_page_count,
total_task_count: task_pager.total_task_count,
task_page_endpoint_base_path: "#{organization.path}/task_pages?tab=#{tab[:name]}"
)
end

def serialized_tasks_for_user(tasks, user)
return [] if tasks.empty?

Expand All @@ -54,33 +75,24 @@ def include_tracking_tasks_tab?
end

def tracking_tasks_tab
name = Constants.QUEUE_CONFIG.TRACKING_TASKS_TAB_NAME
tasks = TaskPager.new(assignee: organization, tab_name: name).tasks_for_tab

{
label: COPY::ALL_CASES_QUEUE_TABLE_TAB_TITLE,
name: name,
name: Constants.QUEUE_CONFIG.TRACKING_TASKS_TAB_NAME,
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never ended up using the task_group element.

tasks: tasks,
allow_bulk_assign: false
}
end

# rubocop:disable Metrics/AbcSize
def unassigned_tasks_tab
name = Constants.QUEUE_CONFIG.UNASSIGNED_TASKS_TAB_NAME
tasks = TaskPager.new(assignee: organization, tab_name: name).tasks_for_tab

{
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

description: format(COPY::ORGANIZATIONAL_QUEUE_PAGE_UNASSIGNED_TASKS_DESCRIPTION, organization.name),
# Compact to account for the maybe absent regional office column
columns: [
Expand All @@ -93,21 +105,14 @@ def unassigned_tasks_tab
Constants.QUEUE_CONFIG.DAYS_ON_HOLD_COLUMN,
Constants.QUEUE_CONFIG.DOCUMENT_COUNT_READER_LINK_COLUMN
].compact,
task_group: Constants.QUEUE_CONFIG.UNASSIGNED_TASKS_GROUP,
tasks: tasks,
allow_bulk_assign: organization.can_bulk_assign_tasks?
}
end
# rubocop:enable Metrics/AbcSize

# rubocop:disable Metrics/AbcSize
def assigned_tasks_tab
name = Constants.QUEUE_CONFIG.ASSIGNED_TASKS_TAB_NAME
tasks = TaskPager.new(assignee: organization, tab_name: name).tasks_for_tab

{
label: format(COPY::QUEUE_PAGE_ASSIGNED_TAB_TITLE, tasks.count),
name: name,
label: COPY::QUEUE_PAGE_ASSIGNED_TAB_TITLE,
name: Constants.QUEUE_CONFIG.ASSIGNED_TASKS_TAB_NAME,
description: format(COPY::ORGANIZATIONAL_QUEUE_PAGE_ASSIGNED_TASKS_DESCRIPTION, organization.name),
# Compact to account for the maybe absent regional office column
columns: [
Expand All @@ -120,20 +125,14 @@ def assigned_tasks_tab
Constants.QUEUE_CONFIG.DOCKET_NUMBER_COLUMN,
Constants.QUEUE_CONFIG.DAYS_ON_HOLD_COLUMN
].compact,
task_group: Constants.QUEUE_CONFIG.ASSIGNED_TASKS_GROUP,
tasks: tasks,
allow_bulk_assign: false
}
end
# rubocop:enable Metrics/AbcSize

def completed_tasks_tab
name = Constants.QUEUE_CONFIG.COMPLETED_TASKS_TAB_NAME
tasks = TaskPager.new(assignee: organization, tab_name: name).tasks_for_tab

{
label: COPY::QUEUE_PAGE_COMPLETE_TAB_TITLE,
name: name,
name: Constants.QUEUE_CONFIG.COMPLETED_TASKS_TAB_NAME,
description: format(COPY::QUEUE_PAGE_COMPLETE_TASKS_DESCRIPTION, organization.name),
# Compact to account for the maybe absent regional office column
columns: [
Expand All @@ -146,8 +145,6 @@ def completed_tasks_tab
Constants.QUEUE_CONFIG.DOCKET_NUMBER_COLUMN,
Constants.QUEUE_CONFIG.DAYS_ON_HOLD_COLUMN
].compact,
task_group: Constants.QUEUE_CONFIG.COMPLETED_TASKS_GROUP,
tasks: tasks,
allow_bulk_assign: false
}
end
Expand Down
9 changes: 9 additions & 0 deletions app/models/task_pager.rb
Expand Up @@ -46,6 +46,14 @@ def paged_tasks
# tasks
# end

def task_page_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the kaminari total_pages method work on tasks_for_tab here?

(total_task_count / TASKS_PER_PAGE.to_f).ceil
end

def total_task_count
@total_task_count ||= tasks_for_tab.count
end

def tasks_for_tab
case tab_name
when Constants.QUEUE_CONFIG.TRACKING_TASKS_TAB_NAME
Expand All @@ -67,6 +75,7 @@ def tracking_tasks
TrackVeteranTask.includes(*task_includes).active.where(assigned_to: assignee)
end

# TODO: Rename this function so that it makes sense for users and organizations
def unassigned_tasks
Task.includes(*task_includes)
.visible_in_queue_table_view.where(assigned_to: assignee).active
Expand Down
39 changes: 17 additions & 22 deletions client/app/components/TablePagination.jsx
Expand Up @@ -36,33 +36,28 @@ class TablePagination extends React.PureComponent {

render() {
const {
paginatedData,
casesPerPage,
currentPage,
numberOfPages,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

totalCasesCount
} = this.props;
const numberOfPages = paginatedData.length;

// Render the pagination summary
// Get previous number of cases so we can calculate the case range in
// the pagination summary
let previousCaseCount = 0;
let beginningCaseNumber = 0;
let endingCaseNumber = 0;

for (let i = 0; i < currentPage; i += 1) {
previousCaseCount += paginatedData[i] ? paginatedData[i].length : 0;
if (totalCasesCount) {
// Assume we are in one of the middle pages.
beginningCaseNumber = 1 + (currentPage * casesPerPage);
endingCaseNumber = (currentPage + 1) * casesPerPage;

// Correct for us being on the last page.
if (endingCaseNumber > totalCasesCount) {
endingCaseNumber = totalCasesCount;
}
}
// If there are no pages, there is no data, so the range should be 0-0.
// Otherwise, the beginning of the range is the previous amount of cases + 1
const beginningCaseNumber = numberOfPages > 0 ? previousCaseCount + 1 : 0;
// If there are no pages, there is no data, so the range should be 0-0.
// Otherwise, the end of the range is the previous amount of cases +
// the amount of data in the current page.
const endingCaseNumber = numberOfPages > 0 && paginatedData[currentPage] ?
previousCaseCount + paginatedData[currentPage].length :
0;
// Create the range
let currentCaseRange = `${beginningCaseNumber}-${endingCaseNumber}`;
// Create the entire summary
const paginationSummary = `Viewing ${currentCaseRange} of ${totalCasesCount} total cases`;

const paginationSummary = `Viewing ${beginningCaseNumber}-${endingCaseNumber} of ${totalCasesCount} total cases`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Render the page buttons
let pageButtons = [];
let paginationButtons = [];
Expand Down Expand Up @@ -137,7 +132,7 @@ class TablePagination extends React.PureComponent {

TablePagination.propTypes = {
currentPage: PropTypes.number.isRequired,
paginatedData: PropTypes.arrayOf(PropTypes.array).isRequired,
numberOfPages: PropTypes.number.isRequired,
totalCasesCount: PropTypes.number.isRequired,
updatePage: PropTypes.func.isRequired
};
Expand Down
21 changes: 14 additions & 7 deletions client/app/queue/OrganizationQueue.jsx
Expand Up @@ -18,6 +18,7 @@ import {
getCompletedOrganizationalTasks,
trackingTasksForOrganization
} from './selectors';
import { tasksWithAppealsFromRawTasks } from './utils';
import { clearCaseSelectSearch } from '../reader/CaseSelect/CaseSelectActions';
import { fullWidth } from './constants';
import QUEUE_CONFIG from '../../constants/QUEUE_CONFIG.json';
Expand Down Expand Up @@ -86,19 +87,25 @@ class OrganizationQueue extends React.PureComponent {
return mapper[tabName];
}

taskTableTabFactory = (tabConfig) => {
const { label, description } = tabConfig;
const cols = this.columnsFromConfig(tabConfig);
const tasks = this.tasksForTab(tabConfig.name);
taskTableTabFactory = (tabConfig, config) => {
const tasks = config.use_task_pages_api ?
tasksWithAppealsFromRawTasks(tabConfig.tasks) :
this.tasksForTab(tabConfig.name);

return {
label,
label: tabConfig.label,
page: <React.Fragment>
<p className="cf-margin-top-0">{description}</p>
<p className="cf-margin-top-0">{tabConfig.description}</p>
{ tabConfig.allow_bulk_assign && <BulkAssignButton /> }
<TaskTable
customColumns={cols}
key={tabConfig.name}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

customColumns={this.columnsFromConfig(tabConfig)}
tasks={tasks}
useTaskPagesApi={config.use_task_pages_api}
tasksPerPage={config.tasks_per_page}
numberOfPages={tabConfig.task_page_count}
totalTaskCount={tabConfig.total_task_count}
taskPagesApiEndpoint={tabConfig.task_page_endpoint_base_path}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>
</React.Fragment>
};
Expand Down