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

Ensure task paper trails are being saved when AMA appeals are cancelled #12617

Closed
1 task
hschallhorn opened this issue Nov 5, 2019 · 8 comments · Fixed by #13057
Closed
1 task

Ensure task paper trails are being saved when AMA appeals are cancelled #12617

hschallhorn opened this issue Nov 5, 2019 · 8 comments · Fixed by #13057
Labels
Priority: Medium Blocking issue w/workaround, or "second in" priority for new work. Source: Bat Team Team: Echo 🐬 Type: Tech-Improvement

Comments

@hschallhorn
Copy link
Contributor

hschallhorn commented Nov 5, 2019

Relavent slack.

It appears as if all changes to tasks are not creating a paper trail, specifically when AMA appeals are cancelled by removing the last request issue on the appeal.

To reproduce

  1. Log in as BVACASPER and find an AMA appeal assigned to you
a = Appeal.find_by(uuid: "3f62f5b2-8439-46e5-b097-4043d034d6a9")
puts a.structure_render(:id, :status, :assigned_to_id)
Appeal 60 [id, status, assigned_to_id]
└── RootTask 361, on_hold, 4
    └── JudgeDecisionReviewTask 362, on_hold, 3
        └── AttorneyTask 363, in_progress, 1
Task.find(363).versions
=> []
  1. Remove the last issue on an AMA appeal at the attorney task (this cancels the whole appeal) and confirm this cancels the parent judge task and the grandparent root task.
puts a.structure_render(:id, :status, :assigned_to_id)
Appeal 60 [id, status, assigned_to_id]
└── RootTask 361, cancelled, 4
    └── JudgeDecisionReviewTask 362, cancelled, 3
        └── AttorneyTask 363, cancelled, 1
Task.find(363).versions
=> [] # This should have at least one version but it does not

Notes

Fixing this would allow us to be able to find all the version records related to this change with the request id that caused it and quickly undo the change with

request_id = Task.find(363).versions.last.request_id
previous_tasks = PaperTrail::Version.where(request_id: request_id)
previous_tasks.each { |previous_task_state| previous_task_state.reify.save }

This could also support future work to create an undo function and the batteam fields at least one of these undo requests a day

AC

  • All task changes create a version in the paper trail table
@hschallhorn
Copy link
Contributor Author

Mildly related to #11963

@lomky
Copy link
Contributor

lomky commented Nov 14, 2019

What is this graph?

1 | |
2 | ||||||||
3 | |||||||||
5 | 
8 | 

The way we cancel AMA appeals with the ActiveRecord update_all, which skips callbacks. Papertrail uses these callbacks. Do we need only some callbacks, or was this being overly defensive? Unsure, but the update_all was intentional originally.

Can we explicitly save a version in papertrail? We think so, but don't know.

Scope this ticket to this explicit bugfix, but also search for anywhere else we update without callbacks and file follow-on issues to address those, unless the fix is trivial to do here.

Estimating at a 3. Might be a 1 if this is super straightfoward to call papertrail directly.

@hschallhorn
Copy link
Contributor Author

Where update_all is called

@jimruggiero jimruggiero added Priority: Medium Blocking issue w/workaround, or "second in" priority for new work. Type: Tech-Improvement labels Nov 26, 2019
@lomky
Copy link
Contributor

lomky commented Dec 10, 2019

Papertrail: Saving a new version manually - available in Papertrail 9+; we're on 8.

Saving a New Version Manually

You may want to save a new version regardless of options like :on, :if, or :unless. Or, in rare situations, you may want to save a new version even if the record has not changed.

my_model.paper_trail.save_with_version

@hschallhorn
Copy link
Contributor Author

hschallhorn commented Dec 10, 2019

hm. I'm getting a NoMethodError on this

task = ColocatedTask.last
parent_task = task.parent
tasks = Task.where(id: [parent_task.id, task.id])
=> [#<IhpColocatedTask:0x00007f9f8e012248
  id: 1141,
  appeal_id: 253,
  appeal_type: "Appeal",
  assigned_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  assigned_by_id: 1,
  assigned_to_id: 5,
  assigned_to_type: "Organization",
  closed_at: nil,
  created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  instructions: ["skd;fb"],
  on_hold_duration: nil,
  parent_id: 756,
  placed_on_hold_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  started_at: nil,
  status: "on_hold",
  type: "IhpColocatedTask",
  updated_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00>,
 #<IhpColocatedTask:0x00007f9f8e011cd0
  id: 1142,
  appeal_id: 253,
  appeal_type: "Appeal",
  assigned_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  assigned_by_id: 1,
  assigned_to_id: 16,
  assigned_to_type: "User",
  closed_at: nil,
  created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  instructions: ["skd;fb"],
  on_hold_duration: nil,
  parent_id: 1141,
  placed_on_hold_at: nil,
  started_at: nil,
  status: "assigned",
  type: "IhpColocatedTask",
  updated_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00>]
tasks.map(&:versions)
=> [[#<PaperTrail::Version:0x00007f9f83348030
   id: 847,
   created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
   event: "update",
   item_id: 1141,
   item_type: "Task",
   object:
    "---\nid: 1141\nappeal_id: 253\nappeal_type: Appeal\nassigned_at: &1 2019-12-05 20:51:25.235971000 Z\nassigned_by_id: 1\nassigned_to_id: 5\nassigned_to_type: Organization\nclosed_at: \ncreated_at: *1\ninstructions:\n- skd;fb\non_hold_duration: \nparent_id: 756\nplaced_on_hold_at: \nstarted_at: \nstatus: assigned\ntype: IhpColocatedTask\nupdated_at: *1\n",
   object_changes:
    "---\nplaced_on_hold_at:\n- \n- 2019-12-05 20:51:25.319857000 Z\nstatus:\n- assigned\n- on_hold\nupdated_at:\n- 2019-12-05 20:51:25.235971000 Z\n- 2019-12-05 20:51:25.320025000 Z\n",
   request_id: "585bdb20-3d73-42c7-9b61-729b399677d6",
   whodunnit: "1">],
 []]
tasks.update_all(status: "cancelled")
tasks
=> [#<IhpColocatedTask:0x00007f9f8072fef8
  id: 1141,
  appeal_id: 253,
  appeal_type: "Appeal",
  assigned_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  assigned_by_id: 1,
  assigned_to_id: 5,
  assigned_to_type: "Organization",
  closed_at: nil,
  created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  instructions: ["skd;fb"],
  on_hold_duration: nil,
  parent_id: 756,
  placed_on_hold_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  started_at: nil,
  status: "cancelled",
  type: "IhpColocatedTask",
  updated_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00>,
 #<IhpColocatedTask:0x00007f9f8072ee18
  id: 1142,
  appeal_id: 253,
  appeal_type: "Appeal",
  assigned_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  assigned_by_id: 1,
  assigned_to_id: 16,
  assigned_to_type: "User",
  closed_at: nil,
  created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
  instructions: ["skd;fb"],
  on_hold_duration: nil,
  parent_id: 1141,
  placed_on_hold_at: nil,
  started_at: nil,
  status: "cancelled",
  type: "IhpColocatedTask",
  updated_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00>]
tasks.map(&:versions)
=> [[#<PaperTrail::Version:0x00007f9f83348030
   id: 847,
   created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
   event: "update",
   item_id: 1141,
   item_type: "Task",
   object:
    "---\nid: 1141\nappeal_id: 253\nappeal_type: Appeal\nassigned_at: &1 2019-12-05 20:51:25.235971000 Z\nassigned_by_id: 1\nassigned_to_id: 5\nassigned_to_type: Organization\nclosed_at: \ncreated_at: *1\ninstructions:\n- skd;fb\non_hold_duration: \nparent_id: 756\nplaced_on_hold_at: \nstarted_at: \nstatus: assigned\ntype: IhpColocatedTask\nupdated_at: *1\n",
   object_changes:
    "---\nplaced_on_hold_at:\n- \n- 2019-12-05 20:51:25.319857000 Z\nstatus:\n- assigned\n- on_hold\nupdated_at:\n- 2019-12-05 20:51:25.235971000 Z\n- 2019-12-05 20:51:25.320025000 Z\n",
   request_id: "585bdb20-3d73-42c7-9b61-729b399677d6",
   whodunnit: "1">],
 []]
tasks.each { |task| task.paper_trail.save_with_version }
NoMethodError: undefined method `save_with_version' for #<PaperTrail::RecordTrail:0x00007f9f8978e4b8>

@lomky
Copy link
Contributor

lomky commented Dec 10, 2019

We're on PaperTrail 8. We could look into bumping the gem, or using some work arounds others have developed See StackOverflow

@hschallhorn
Copy link
Contributor Author

Doesn't seem to be saving the object or the changes

tasks.each { |task| task.paper_trail.record_update(force: true, in_after_callback: false) }
tasks.map(&:versions)
=> [[#<PaperTrail::Version:0x00007f9f83348030
   id: 847,
   created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
   event: "update",
   item_id: 1141,
   item_type: "Task",
   object:
    "---\nid: 1141\nappeal_id: 253\nappeal_type: Appeal\nassigned_at: &1 2019-12-05 20:51:25.235971000 Z\nassigned_by_id: 1\nassigned_to_id: 5\nassigned_to_type: Organization\nclosed_at: \ncreated_at: *1\ninstructions:\n- skd;fb\non_hold_duration: \nparent_id: 756\nplaced_on_hold_at: \nstarted_at: \nstatus: assigned\ntype: IhpColocatedTask\nupdated_at: *1\n",
   object_changes:
    "---\nplaced_on_hold_at:\n- \n- 2019-12-05 20:51:25.319857000 Z\nstatus:\n- assigned\n- on_hold\nupdated_at:\n- 2019-12-05 20:51:25.235971000 Z\n- 2019-12-05 20:51:25.320025000 Z\n",
   request_id: "585bdb20-3d73-42c7-9b61-729b399677d6",
   whodunnit: "1">,
  #<PaperTrail::Version:0x00007f9f836565b0
   id: 849,
   created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
   event: "update",
   item_id: 1141,
   item_type: "Task",
   object:
    "---\nid: \nappeal_id: \nappeal_type: \nassigned_at: \nassigned_by_id: \nassigned_to_id: \nassigned_to_type: \nclosed_at: \ncreated_at: \ninstructions: \non_hold_duration: \nparent_id: \nplaced_on_hold_at: \nstarted_at: \nstatus: \ntype: \nupdated_at: \n",
   object_changes: "--- {}\n",
   request_id: nil,
   whodunnit: nil>],
 [#<PaperTrail::Version:0x00007f9f835ed920
   id: 850,
   created_at: Thu, 05 Dec 2019 20:51:25 UTC +00:00,
   event: "update",
   item_id: 1142,
   item_type: "Task",
   object:
    "---\nid: \nappeal_id: \nappeal_type: \nassigned_at: \nassigned_by_id: \nassigned_to_id: \nassigned_to_type: \nclosed_at: \ncreated_at: \ninstructions: \non_hold_duration: \nparent_id: \nplaced_on_hold_at: \nstarted_at: \nstatus: \ntype: \nupdated_at: \n",
   object_changes: "--- {}\n",
   request_id: nil,
   whodunnit: nil>]]

@lomky lomky mentioned this issue Dec 10, 2019
1 task
va-bot pushed a commit that referenced this issue Dec 12, 2019
Bumps #12617 

### Description
Bumps PaperTrail to 9.1/9.2/10./10.3
(Going to push for the highest non-breaking!)

This gives us the ability to explicitly call for a papertrail creation without callbacks

### Acceptance Criteria
- [x] Papertrail continues to work!

### Testing Plan
1. Run the test suite
2. Manually test in console
@lomky
Copy link
Contributor

lomky commented Dec 23, 2019

It looks like the new function will save the object field, but not the object_changes field, which is a bit of a bummer for ease of analysis purposes. But PaperTrail's functionality does not rely on the object_changes field.

va-bot pushed a commit that referenced this issue Jan 16, 2020
Resolves #12617 

First pass adding cancellation version saves. Imperfect, showing some strain around our callbacks.

### Acceptance Criteria
- [x] Code compiles correctly
- [x] Tests pass
- [x] Ticket filed to document callbacks and stop skipping callbacks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium Blocking issue w/workaround, or "second in" priority for new work. Source: Bat Team Team: Echo 🐬 Type: Tech-Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants