enables on hold for user #9888

amprokop · 2019-03-11T18:50:35Z

This code is not yet fully tested nor does it have enough automated tests, just surfacing for feedback on the general approach.

Creates a new TimedHoldTask class that imports TimeableTask. To put a GenericTask on hold, we will now call GenericTask.place_on_hold(parent, current_user, params).

Colocated tasks are not changed for now, but I suggest we plan to migrate them when convenient.

I considered sending the TimedHoldTask to the frontend and then just updating the Case Timeline and all the frontend logic to handle this new TimedHoldTask correctly, but decided against it for the moment, out of a sense that the frontend shouldn't need to know about all tasks necessarily, and shouldn't be responsible for sorting through backend implementation details.

So, the smoothest past forward seemed to be to redefine the placed_on_hold_at and on_hold_duration methods on GenericTask, so we wouldn't have to change anything from the serializer or the frontend (theoretically). This has the effect that we need to filter out TimedHoldTasks from the list of tasks we return to the frontend.

Suggested path forward for future PRs if we continue with this approach:

Let this in production for awhile to gain confidence that it works as expected
Migrate new ColocatedTask holds to use TimedHoldTask (may require DB backfilling for holds that are already in progress)
Consider removing the following columns - "on_hold_duration", "placed_on_hold_at" — we should be able to just rely on the TaskTimer we create to keep track of the on hold duration, and the created_at field on the TimedHoldTask to determine when the parent task was placed on hold.
- Are those columns used anywhere else, though?

Open questions:

Where in the DB should we put the instructions field? On the TimedHoldTask row or on the parent task?
What's the best place to filter out TimedHoldTasks before we serialize tasks to send to the frontend?
Do we need to migrate JudgeTask and AttorneyTask to this approach as well? It doesn't seem like they can be placed on a timed hold.

Test plan (not ran yet):

placed task on hold
verified that it shows up in the queue
in the console, called when_timer_ends on the on hold task
verified that the appeal shows up in the queue again

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

…fairs/caseflow into amprokop-add-hold-ability

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

lowellrex · 2019-03-14T13:44:06Z

Did we explore creating a new OnHoldTask using the TimeableTask concern as the child of tasks we want to put on hold in order to avoid some of the problems we run into with the way ColocatedTasks implement the holding logic?

Quoting from a slack thread about one of the issues with the current implementation of the on hold ability:

One problem with the way we currently handle putting tasks on hold (by using fields of the task) is that we don't properly handle what happens if a task gets placed on hold a second time, but it is not a timed hold

So if we place a 5 day hold for a task on March 1st, hold expires on March 5th, then on March 6th we create a child task off of this task then the task will immediately be taken off hold when somebody loads there queue because the placed_on_hold_at value and the on_hold_days value indicate that the tasks hold ended already (which it did) without any indication that the new hold (caused by the child task being created) is different than the timed hold

In addition to not distinguishing from the different types of holds we don't currently respect the placed_on_hold_at value when cascading completion status from children tasks to parents.

lowellrex · 2019-03-14T13:45:57Z

app/models/tasks/generic_task.rb

@@ -34,6 +34,7 @@ def available_actions(user)
        Constants.TASK_ACTIONS.ASSIGN_TO_TEAM.to_h,
        Constants.TASK_ACTIONS.REASSIGN_TO_PERSON.to_h,
        Constants.TASK_ACTIONS.MARK_COMPLETE.to_h,
+        Constants.TASK_ACTIONS.PLACE_HOLD.to_h,


We override available_actions for a handful of classes that inherit from GenericTask so we'll need to add this option to those classes as well if we want them to be able to be placed on hold.

The PR that added the ability to cancel tasks might be a good reference for how we could go about accomplishing this.

Thanks! I'll update the available actions for those tasks.

amprokop · 2019-03-14T14:08:43Z

It may be that creating a new task class and migrating everything over to that is the best approach — or we could directly fix the bugs you mentioned with how we currently use the on_hold fields, I don't have a strong opinion yet.

I'm happy to not merge this and start exploring either approach — but I'm not confident that I'll have time to complete either of the alternative approaches this sprint, my shallow read is that both are much more of a lift. You and/or @laurjpeterson should make that call, is it better to merge this now with some known issues, or wait for a more perfect solution?

lpciferri · 2019-03-14T14:14:39Z

I'd prefer to merge this now so users can have the ability to place tasks on hold, which is "generic queue" functionality we trained them on, but haven't had. I know refactoring before merging might be the desired way forward, but if we don't have bandwidth now to do it, I worry about delaying this feature for another few months.

lowellrex · 2019-03-14T16:00:27Z

I am strongly opposed to merging this as is until we do more exploration about the approach I suggested above. I think 2 hours of exploration with a quick summary of findings if it won't pan out would be well worth that time.

I believe we will spend a significant amount of time supporting this approach, and if we eventually move over to a more robust approach that time spent will be negatively productive in getting the refactor in because it would be time spent on things other than developing the robust approach.

amprokop · 2019-03-14T16:20:24Z

@lowellrex — cool, I'm happy to commit 2 hours to this this sprint — I am confident that the approach you proposed will work, but my guess is that implementation and any subsequent fixes will take significantly more than 2 hours, and I can't commit to having it done before next Friday. Does that work for you?

lowellrex · 2019-03-14T17:27:10Z

I can't commit to having it done before next Friday. Does that work for you?

Yup! That works! I think as long as we can get this out the door before the end of the sprint (Fri 22 March) then we'll be good to go!

amprokop · 2019-03-14T17:52:14Z

@lowellrex — hmm I'm not sure I'm going to be able to do that, it will depend on how big of a task it winds up being (when I said next Friday, i meant 22 March). I'll give it a shot — is there some deadline for fixing this bug?

…fairs/caseflow into amprokop-add-hold-ability

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

amprokop · 2019-03-21T02:50:03Z

app/models/appeal.rb

@@ -705,6 +705,10 @@ def bgs
    BGSService.new
  end

+  def tasks_for_frontend


I think this method should probably be in the task model itself. Will move it. But the general idea is that we'll save the frontend from having to walk the task tree to match up on hold tasks with their parents to get those details.

It might make sense to follow this pattern, where we hide tasks from various views with methods that return booleans. So we have hide_from_case_timeline and hide_from_task_snapshot; you could add something like hide_from_task_list and then filter on that value (whether it's here in the Appeal model, on the Task model, or on the frontend).

oh wait — i think what i actually want is to return true for hide_from_case_timeline and hide_from_task_snapshot here

codeclimate · 2019-03-21T02:50:10Z

Code Climate has analyzed commit 85a15e8 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

View more on Code Climate.

amprokop · 2019-03-21T02:51:26Z

app/models/tasks/generic_task.rb

  # rubocop:enable Metrics/MethodLength
  # rubocop:enable Metrics/AbcSize

+  def placed_on_hold_at


So we're overriding the methods that would automatically be accessing DB fields (see the task schema) in GenericTask for now — eventually, I'd favor getting rid of the placed_on_hold_at and on_hold_duration columns entirely.

Maybe we can move this into the base Task class and override these methods only in ColocatedTask since that is the only task class that uses timed holds? Either way, can we write up a ticket that includes a migration plan for abandoning the old (ColocatedTask) way of doing timed holds in favor of this new strategy?

amprokop · 2019-03-21T02:51:56Z

app/models/tasks/generic_task.rb

+    def place_on_hold(parent, current_user, params)
+      transaction do
+        # Remove any other hold tasks so we don't wind up with more than one
+        if parent.timed_hold_task


My guess is that we should only permit one hold on a task at a time, and any new hold should blow away the old one. Open to other suggestions though.

tomas-nava

The overall approach looks good to me!

tomas-nava · 2019-03-21T18:06:00Z

app/models/appeal.rb

@@ -705,6 +705,10 @@ def bgs
    BGSService.new
  end

+  def tasks_for_frontend


It might make sense to follow this pattern, where we hide tasks from various views with methods that return booleans. So we have hide_from_case_timeline and hide_from_task_snapshot; you could add something like hide_from_task_list and then filter on that value (whether it's here in the Appeal model, on the Task model, or on the frontend).

tomas-nava · 2019-03-21T18:37:31Z

app/models/tasks/generic_task.rb

+          on_hold_duration: params[:on_hold_duration]
+        )
+      end
+    end


I think this approach is right. I have some syntactical/implementation feedback which you may've been considering already, so I'll be brief.

the task parameter should be named task instead of parent

this (plus the timed_hold_task getter) will only get the most recent active TimedHoldTask child. I know it shouldn't be possible to have more than one active TimedHoldTask as a child of a GenericTask, but maybe we can get all active tasks and destroy them all to be sure?

can we cascade destruction of the TaskTimer when the TimedHoldTask is destroyed (or vice versa)?

I don't think you need to explicitly set the parent on_hold, this hook should do it for you.

Actually, do we want to destroy tasks or should we cancel them so there's a record?

Updated this to cancel them.

…fairs/caseflow into amprokop-add-hold-ability

lowellrex · 2019-03-27T13:47:41Z

app/models/tasks/timed_hold_task.rb

+class TimedHoldTask < GenericTask
+  include TimeableTask
+
+  validates :on_hold_duration, presence: true, inclusion: { in: 1..100 }


Why did we pick an upper limit of 100 days?

I believe the UI goes up to 90 days. Open to changing it to whatever you like.

Makes sense to me!

lowellrex · 2019-03-27T13:59:51Z

app/models/tasks/timed_hold_task.rb

+  end
+
+  def timer_ends_at
+    Time.zone.today + on_hold_duration.days


Can we update this function to use a static date (maybe the task's creation date?) instead of the current date? It looks like we only call this function when we create the TaskTimer, but we may forget about that convention in the future and I think it would make sense to have this function always return the same value regardless of whether we call this function the day the task was created or 3 days later.

Sure, that sounds like a good tweak. Thanks!

lowellrex · 2019-03-27T14:10:04Z

app/models/tasks/timed_hold_task.rb

+# A nightly job queries for and expires
+
+class TimedHoldTask < GenericTask
+  include TimeableTask


Might we need an available_actions item that ends the hold early?

This work should probably be completed in another PR, along with the front-end work to add the modal to place tasks on hold.

I'm not sure, do we currently have an option in the UI to cancel holds on tasks?

We can't currently cancel holds so this definitely makes sense to be part of a different PR if we want to do it.

lowellrex · 2019-03-27T14:15:42Z

app/controllers/tasks_controller.rb

+              GenericTask.place_on_hold(task, update_params)
+            else
+              task.update_from_params(update_params, current_user)
+            end


Since the only field of the parent task we're updating here is the "status" field by way of the after_create hook perhaps it would be more precise to have this be a create action since we are creating a new instance of the TimedHoldTask?

If we use that approach we would have to move the logic to cancel existing task timers associated with the task to the TimedHoldTask class, but it would allow us to not change TasksController at all.

…fairs/caseflow into amprokop-add-hold-ability

lowellrex · 2019-05-06T18:26:10Z

Closing this out in favor of other PRs (linked in #9207) that will continue this work.

adds to list of actions

90e4fc0

ghost assigned amprokop Mar 11, 2019

ghost added the In-Progress label Mar 11, 2019

amprokop changed the title ~~adds to list of actions~~ enables on hold for user Mar 11, 2019

amprokop added 7 commits March 11, 2019 16:14

fix specs

4deebb6

Merge branch 'master' into amprokop-add-hold-ability

67f57a4

fix spec

9d9fb8a

Merge branch 'amprokop-add-hold-ability' of https://github.com/depart…

3055279

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

Merge branch 'master' into amprokop-add-hold-ability

85900e2

Merge branch 'master' of https://github.com/department-of-veterans-af…

305bee7

…fairs/caseflow into amprokop-add-hold-ability

Merge branch 'amprokop-add-hold-ability' of https://github.com/depart…

84759f4

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

lowellrex reviewed Mar 14, 2019

View reviewed changes

amprokop added 2 commits March 14, 2019 14:06

Merge branch 'master' into amprokop-add-hold-ability

b4fd3ec

Merge branch 'master' of https://github.com/department-of-veterans-af…

ca2ea0f

…fairs/caseflow into amprokop-add-hold-ability

pkarman mentioned this pull request Mar 18, 2019

Intake Edit | Decision review removal confirmation alert #9734

Closed

amprokop added 4 commits March 20, 2019 11:41

Merge branch 'master' of https://github.com/department-of-veterans-af…

814d17d

…fairs/caseflow into amprokop-add-hold-ability

sketches out approach

26cd2ef

Merge branch 'amprokop-add-hold-ability' of https://github.com/depart…

08cd06e

…ment-of-veterans-affairs/caseflow into amprokop-add-hold-ability

adds on hold to list

3c7aeb3

amprokop commented Mar 21, 2019

View reviewed changes

tomas-nava reviewed Mar 21, 2019

View reviewed changes

amprokop added 7 commits March 22, 2019 10:18

Merge branch 'master' of https://github.com/department-of-veterans-af…

fa71bbe

…fairs/caseflow into amprokop-add-hold-ability

fixes frontend filtering

16da1ce

refactor

c28cabc

lint

869ea34

cancel not on hold

773c123

refactor and add nonworking tests

c47bc3d

update

fc124b7

lowellrex reviewed Mar 27, 2019

View reviewed changes

amprokop and others added 2 commits April 8, 2019 10:31

Merge branch 'master' of https://github.com/department-of-veterans-af…

91f1b85

…fairs/caseflow into amprokop-add-hold-ability

Merge branch 'master' into amprokop-add-hold-ability

8545a31

ghost assigned lowellrex Apr 19, 2019

Fix rubocop nits

85a15e8

lowellrex closed this May 6, 2019

ghost removed the In-Progress label May 6, 2019

pkarman deleted the amprokop-add-hold-ability branch May 14, 2019 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enables on hold for user #9888

enables on hold for user #9888

amprokop commented Mar 11, 2019 •

edited

Loading

lowellrex commented Mar 14, 2019

lowellrex Mar 14, 2019

lowellrex Mar 14, 2019

amprokop Mar 14, 2019

amprokop commented Mar 14, 2019 •

edited

Loading

lpciferri commented Mar 14, 2019

lowellrex commented Mar 14, 2019

amprokop commented Mar 14, 2019

lowellrex commented Mar 14, 2019

amprokop commented Mar 14, 2019 •

edited

Loading

amprokop Mar 21, 2019

tomas-nava Mar 21, 2019

amprokop Mar 22, 2019

amprokop Mar 22, 2019

codeclimate bot commented Mar 21, 2019 •

edited

Loading

amprokop Mar 21, 2019

lowellrex Mar 27, 2019

amprokop Mar 21, 2019

tomas-nava left a comment

tomas-nava Mar 21, 2019

tomas-nava Mar 21, 2019

tomas-nava Mar 21, 2019

amprokop Mar 27, 2019

lowellrex Mar 27, 2019

amprokop Mar 27, 2019

lowellrex Mar 27, 2019

lowellrex Mar 27, 2019

amprokop Mar 27, 2019

lowellrex Mar 27, 2019

lowellrex Mar 27, 2019

amprokop Mar 27, 2019

lowellrex Mar 27, 2019

lowellrex Mar 27, 2019

lowellrex commented May 6, 2019

enables on hold for user #9888

enables on hold for user #9888

Conversation

amprokop commented Mar 11, 2019 • edited Loading

lowellrex commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amprokop commented Mar 14, 2019 • edited Loading

lpciferri commented Mar 14, 2019

lowellrex commented Mar 14, 2019

amprokop commented Mar 14, 2019

lowellrex commented Mar 14, 2019

amprokop commented Mar 14, 2019 • edited Loading

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 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomas-nava 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

Choose a reason for hiding this comment

lowellrex commented May 6, 2019

amprokop commented Mar 11, 2019 •

edited

Loading

amprokop commented Mar 14, 2019 •

edited

Loading

amprokop commented Mar 14, 2019 •

edited

Loading

codeclimate bot commented Mar 21, 2019 •

edited

Loading