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

Add pagination to user queues #14242

Closed
wants to merge 23 commits into from

Conversation

hschallhorn
Copy link
Contributor

@hschallhorn hschallhorn commented May 11, 2020

Resolves #14142

Description

Adds pagination to user queues

Acceptance Criteria

  • The tasks in user's queues are paginated

Testing Plan

  1. Create some dummy data to play with
3.times do 
  ColocatedTask.actions_assigned_to_colocated.map(&:to_sym).each do |action|
    appeal = FactoryBot.create(:appeal)
    parent = FactoryBot.create(
      :ama_task,
      assigned_to: User.first,
      assigned_by: User.first,
      appeal: appeal
    )
    task = FactoryBot.create(
      :ama_colocated_task,
      action,
      assigned_to: Colocated.singleton,
      appeal: appeal,
      assigned_by: User.first,
      parent_id: parent.id
    )
    task.children.first.on_hold!
  end
end
  1. Sign in as BVALSPORER
  2. TODO

User Facing Changes

  • Screenshots of UI changes added to PR & Original Issue
BEFORE AFTER

@hschallhorn hschallhorn self-assigned this May 11, 2020
@codeclimate
Copy link

codeclimate bot commented May 11, 2020

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

View more on Code Climate.

@@ -42,7 +42,7 @@ def set_application
# GET /tasks?user_id=xxx&role=attorney
# GET /tasks?user_id=xxx&role=judge
def index
tasks = QueueForRole.new(user_role).create(user: user).tasks
tasks = user.use_task_pages_api? ? [] : QueueForRole.new(user_role).create(user: user).tasks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only improves the initial load time of a queue. Will file followup tickets to improve load times after creating or updating tasks. These tickets would remove QueueForRole here

def create
return invalid_type_error unless task_classes_valid?
tasks = []
param_groups = create_params.group_by { |param| param[:type] }
param_groups.each do |task_type, param_group|
tasks << valid_task_classes[task_type.to_sym].create_many_from_params(param_group, current_user)
end
modified_tasks = [parent_tasks_from_params, tasks].flatten!
tasks_to_return = (QueueForRole.new(user_role).create(user: current_user).tasks + modified_tasks).uniq
render json: { tasks: json_tasks(tasks_to_return) }
and here
def update
tasks = task.update_from_params(update_params, current_user)
tasks.each { |t| return invalid_record_error(t) unless t.valid? }
tasks_to_return = (QueueForRole.new(user_role).create(user: current_user).tasks + tasks).uniq
render json: { tasks: json_tasks(tasks_to_return) }
end

and should exist outside of pagination

tasks: tasks, params: params, ama_serializer: WorkQueue::TaskSerializer
).call
end
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.

Pulled all this out of organization task pages controller to be reused in user task pages controller.

@@ -141,10 +141,6 @@ def completed_tasks_tab
::OrganizationCompletedTasksTab.new(assignee: self, show_regional_office_column: show_regional_office_in_queue?)
end

def ama_task_serializer
WorkQueue::TaskSerializer
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.

Pulled out here

def assignee
user
end
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.

New controller for requesting paged tasks assigned to a user

@@ -48,7 +48,7 @@ def attach_tasks_to_tab(tab)
# This allows us to only instantiate TaskPager if we are using the task pages API.
task_page_count: task_pager.task_page_count,
total_task_count: tab.tasks.count,
task_page_endpoint_base_path: "#{assignee_is_org? ? "#{assignee.path}/" : ''}#{endpoint}"
task_page_endpoint_base_path: "#{assignee_is_org? ? "#{assignee.path}/" : "users/#{assignee.id}/"}#{endpoint}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the endpoint the front end uses to request paged tasks

assigned_to_type: Organization.name,
appeal_type: LegacyAppeal.name,
type: ColocatedTask.subclasses.map(&: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.

Copies logic from AttorneyQueue. Only issue is that it will return multiple colocated tasks for one appeal. Debating on how to fix this. Currently affects 42 cases out of ~2500

legacy_colocated_tasks = ColocatedTask.open.where(appeal_type: LegacyAppeal.name, assigned_to: Colocated.singleton)
legacy_colocated_tasks.pluck(:appeal_id).uniq.count
=> 2593
legacy_colocated_tasks.group(:appeal_id).count.values.select { |count| count > 1 }.count
=> 42

@@ -23,7 +23,7 @@ def initialize(args)
end

def paged_tasks
sorted_tasks(filtered_tasks).page(page).per(TASKS_PER_PAGE)
@paged_tasks ||= sorted_tasks(filtered_tasks).page(page).per(TASKS_PER_PAGE)
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 were calling this again every time we called paged tasks

@@ -337,7 +337,7 @@ def queue_tabs
end

def self.default_active_tab
Constants.QUEUE_CONFIG.ASSIGNED_TASKS_TAB_NAME
Constants.QUEUE_CONFIG.INDIVIDUALLY_ASSIGNED_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.

Error from previous PRs

@@ -73,6 +73,7 @@ class AttorneyTaskListView extends React.PureComponent {
assignedTasks={this.props.workableTasks}
onHoldTasks={this.props.onHoldTasks}
completedTasks={this.props.completedTasks}
paginationOptions={this.props.paginationOptions}
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 pagination options though to queue table builder

@@ -124,7 +123,7 @@ AttorneyTaskListView.propTypes = {
resetErrorMessages: PropTypes.func,
showErrorMessage: PropTypes.func,
onHoldTasks: PropTypes.array,
organizations: PropTypes.array,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Orgs was unused

@@ -40,6 +40,7 @@ class ColocatedTaskListView extends React.PureComponent {
assignedTasks={this.props.assignedTasks}
onHoldTasks={this.props.onHoldTasks}
completedTasks={this.props.completedTasks}
paginationOptions={this.props.paginationOptions}
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 pagination options though to queue table builder

@@ -71,7 +70,6 @@ ColocatedTaskListView.propTypes = {
completedTasks: PropTypes.array,
hideSuccessMessage: PropTypes.func,
onHoldTasks: PropTypes.array,
organizations: PropTypes.array,
queueConfig: PropTypes.object,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

orgs and queue config were unused

resources :users, only: [:index, :update]
resources :users, only: [:index] do
resources :users, only: [:index, :update] do
resources :task_pages, only: [:index], controller: 'users/task_pages'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Route for user tasks pager


context "when using task pages api" do
before do
expect(QueueForRole).not_to receive(:new)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure we are not pulling tasks from QueueForRole

expect(response.status).to eq 200

queue_for_role_tasks = JSON.parse(response.body)["tasks"]["data"]
expect(queue_for_role_tasks.size).to be(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensure tasks are not being sent to the front end outside of queue config

expect(queue_for_role_tasks.size).to be(0)

paged_tasks = JSON.parse(response.body)["queue_config"]["tabs"].first["tasks"]
expect(paged_tasks.size).to be(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensure tasks are being sent though queue config

@@ -232,7 +232,8 @@
let!(:on_hold_tasks) { create_list(:ama_task, 5, :on_hold, assigned_to: assignee) }
let!(:completed_tasks) { create_list(:ama_task, 7, :completed, assigned_to: assignee) }

before { allow(assignee).to receive(:use_task_pages_api?).and_return(true) }
before { FeatureToggle.enable!(:user_queue_pagination) }
after { FeatureToggle.disable!(:user_queue_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.

let!(:other_folks_tasks) { create_list(:ama_task, 1) }
let!(:assignee_active_tasks) { create_list(:ama_task, 1, :assigned, assigned_to: assignee) }
let!(:assignee_on_hold_tasks) { create_list(:ama_task, 3, :on_hold, assigned_to: assignee) }
let!(:assignee_legacy_colocated_tasks) { create_list(:colocated_task, 5, assigned_by: assignee) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New model test to ensure we pick up attorney tasks

@@ -77,16 +77,20 @@ def filter_by_css_id_or_name
render json: { users: json_users(users) }
end

def id
@id ||= params[:id] || params[:user_id]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not for the life of me get a route were I could pass :id as the param, only :user_id. Very very open to suggestions 🙏

@@ -325,7 +325,7 @@ def update_status!(new_status)
end

def use_task_pages_api?
false
FeatureToggle.enabled?(:user_queue_pagination, user: self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checks if pagination is enabled for this user

/>
</div>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this is pulled out into client/app/queue/QueueTableBuilder.jsx

def attorney?
non_administered_judge_teams.any? || attorney_in_vacols?
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.

Rely on user being on a judge team before checking vacols

@@ -325,7 +333,7 @@ def update_status!(new_status)
end

def use_task_pages_api?
false
FeatureToggle.enabled?(:user_queue_pagination, user: self) && !attorney? && !judge?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot paginate attorney or judge queues as they will have legacy tasks that are not persisted in the database.

@@ -277,7 +277,7 @@ class QueueApp extends React.PureComponent {

return (
<OrganizationQueueLoadingScreen urlToLoad={`${url}/tasks`}>
<OrganizationQueue {...this.props} paginationOptions={querystring.parse(window.location.search.slice(1))} />
<OrganizationQueue {...this.props} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now done in queue table builder

@@ -660,7 +660,7 @@ HeaderRow.propTypes = FooterRow.propTypes = Row.propTypes = BodyRows.propTypes =
useTaskPagesApi: PropTypes.bool,
userReadableColumnNames: PropTypes.object,
tabPaginationOptions: PropTypes.shape({
[QUEUE_CONFIG.PAGE_NUMBER_REQUEST_PARAM]: PropTypes.number,
[QUEUE_CONFIG.PAGE_NUMBER_REQUEST_PARAM]: PropTypes.string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

suppress console warning

@@ -317,7 +317,7 @@ describe('ColocatedTaskListView', () => {
}
],
"tasks_per_page": 15,
"use_task_pages_api": true
"use_task_pages_api": false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure these tests still pass without pagination turned on (ensures we did not break anything). This was not being used on the front end until now.

allow_any_instance_of(::Organizations::TaskPagesController).to receive(:json_tasks).and_return([])

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

Pulled out to shared example below

@@ -76,7 +76,7 @@
let(:assignee) { user }

it "is the assigned tab" do
expect(subject[:active_tab]).to eq(Constants.QUEUE_CONFIG.ASSIGNED_TASKS_TAB_NAME)
expect(subject[:active_tab]).to eq(Constants.QUEUE_CONFIG.INDIVIDUALLY_ASSIGNED_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.

Bug from previous PRs

@hschallhorn
Copy link
Contributor Author

Closing to create a stacked PR!

@hschallhorn hschallhorn deleted the hschallhorn/14142-user-queue-pagination branch May 15, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defer the loading of user on_hold tasks
1 participant