Track consent revoked by and display it on the file card #4857

dtgreiner · 2024-10-23T15:11:02Z

Merging this PR

use the squash-merge strategy for PRs targeting a release-X branch
use a merge-commit or rebase strategy for PRs targeting the stable branch

Description

Track consent revoked by and display the data on the file card the file has been revoked.

Type of change

New feature

Checklist before requesting review

I have performed a self-review of my code
I have run the code that is being changed under ideal conditions, and it doesn't fail
If adding a new endpoint / exposing data in a new way, I have:
- ensured the API can't leak data from other data sources
- ensured this does not introduce N+1s
- ensured permissions and visibility checks are performed in the right places
I have updated the documentation (or not applicable)
I have added spec tests (or not applicable)
I have provided testing instructions in this PR or the related issue (or not applicable)

eanders · 2024-10-24T15:17:23Z

app/controllers/clients/files_controller.rb

@@ -249,6 +252,11 @@ def batch_params
      params.require(:batch_download).permit(:file_ids)
    end

+    def set_user
+      # binding.pry


Suggested change

# binding.pry

eanders · 2024-10-24T15:25:08Z

app/models/grda_warehouse/client_file.rb

-    after_save :note_changes_in_consent, if: ->(m) { m.should_run_callbacks? }
-    after_commit :set_client_consent, on: [:create, :update], if: ->(m) { m.should_run_callbacks? }
+    after_create_commit :notify_users, if: ->(m) { m.should_run_callbacks? } # rubocop:disable Style/SymbolProc
+    before_update :adjust_revoked_by, if: ->(m) { m.should_run_callbacks? && consent_revoked_at_changed? }


Is there some reason we can't just call this from the controller? There are a bunch of callbacks, but the controller should know the user that made the change, so should be able to set consent_revoked_by_user_id: current_user.id when creating or updating the record. (There is significant complexity here, so I may be missing something.)

While we're in here, it might be worth adding has_paper_trail. I was going to suggest we pull the user out of the versions table, but it appears these aren't being logged.

It was mentioned in the ticket that this may be a good way to go. At the time, it felt like capturing it at a deeper level may good if we are working with files in multiple controllers, but I see what you are saying and it does feel less complex to just have the controller handle it. I'll adjust the PR.

eanders

Happy to discuss if you already looked at these ideas, but dropped a few questions inline.

dtgreiner · 2024-10-25T15:55:38Z

@eanders I just pushed an update to pull the logic out of the callbacks and into the controllers. It should be good for another look whenever you have the chance. Thanks!

Edit - missed adding paper_trail here. Working on that now.

eanders · 2024-10-25T19:44:30Z

app/controllers/clients/files_controller.rb

+      revoked_by_id = current_user.id if @file.consent_revoked_at.present?
+      @file.consent_revoked_by_user_id = revoked_by_id if @file.consent_revoked_at_changed?


Suggested change

revoked_by_id = current_user.id if @file.consent_revoked_at.present?

@file.consent_revoked_by_user_id = revoked_by_id if @file.consent_revoked_at_changed?

if @file.consent_revoked_at_changed?

@file.consent_revoked_by_user_id = if @file.consent_revoked_at.present?

current_user.id

else

nil

end

end

The above can go in a method on ClientFile so you can call @file.set_revocation_info(current_user)

eanders · 2024-10-25T19:46:19Z

app/controllers/clients/files_controller.rb

@@ -230,6 +233,7 @@ def file_params
          :effective_date,
          :expiration_date,
          :consent_revoked_at,
+          :consent_revoked_by_id,


Don't think you need this since we aren't passing it as a parameter.

eanders

Thanks @dtgreiner, this is way cleaner.

gigxz · 2024-10-29T01:35:58Z

app/models/grda_warehouse/file.rb

@@ -7,6 +7,7 @@
 module GrdaWarehouse
  class File < GrdaWarehouseBase
    acts_as_paranoid
+    has_paper_trail


This caused a test regression on HMIS, because duplicate versions are being created for HMIS files. https://github.com/greenriver/hmis-warehouse/actions/runs/11525170831/job/32086897713

The problem is that Hmis::File also calls has_paper_trail (and attaches some metadata).

Paper Trail has an unreleased (breaking) change that will actually cause this to fail https://github.com/paper-trail-gem/paper_trail/blob/master/CHANGELOG.md#breaking-changes / paper-trail-gem/paper_trail#1479. They acknowledge that this can create duplicates. Glad we caught it with the test first!

I don't have much context on this PR, so not sure what the best course of action is. This class is pretty lightweight, so one approach is for Hmis::File to just not subclass it. I did that here #4872 and the tests pass, but it needed some weirdness to get type set.

Good catch @gigxz ! I believe our intent was actually to add has_paper_trail to ClientFile. Not sure we need it in the base class. I'll give that a try and see if tests still fail.

Track consent revoked by and display it on the file card

a876e08

dtgreiner requested a review from eanders October 23, 2024 15:17

eanders reviewed Oct 24, 2024

View reviewed changes

eanders requested changes Oct 24, 2024

View reviewed changes

move revoked by user to controller

3c6af91

Add paper trail to files

80430d1

eanders reviewed Oct 25, 2024

View reviewed changes

dtgreiner added 2 commits October 25, 2024 16:38

move revokation sync to client file method

fc92fdb

Merge branch 'release-139' into dg/file_consent_revoked_by-6422

1a42f89

eanders approved these changes Oct 25, 2024

View reviewed changes

Merge branch 'release-139' into dg/file_consent_revoked_by-6422

1e977bf

eanders merged commit 7247110 into release-139 Oct 25, 2024
11 of 12 checks passed

eanders deleted the dg/file_consent_revoked_by-6422 branch October 25, 2024 20:44

gigxz reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track consent revoked by and display it on the file card #4857

Track consent revoked by and display it on the file card #4857

dtgreiner commented Oct 23, 2024 •

edited

Loading

eanders Oct 24, 2024

eanders Oct 24, 2024

dtgreiner Oct 25, 2024

eanders left a comment

dtgreiner commented Oct 25, 2024 •

edited

Loading

eanders Oct 25, 2024

eanders Oct 25, 2024

eanders Oct 25, 2024

eanders left a comment

gigxz Oct 29, 2024 •

edited

Loading

eanders Oct 29, 2024

		revoked_by_id = current_user.id if @file.consent_revoked_at.present?
		@file.consent_revoked_by_user_id = revoked_by_id if @file.consent_revoked_at_changed?

Track consent revoked by and display it on the file card #4857

Track consent revoked by and display it on the file card #4857

Conversation

dtgreiner commented Oct 23, 2024 • edited Loading

Merging this PR

Description

Type of change

Checklist before requesting review

eanders Oct 24, 2024

Choose a reason for hiding this comment

eanders Oct 24, 2024

Choose a reason for hiding this comment

dtgreiner Oct 25, 2024

Choose a reason for hiding this comment

eanders left a comment

Choose a reason for hiding this comment

dtgreiner commented Oct 25, 2024 • edited Loading

eanders Oct 25, 2024

Choose a reason for hiding this comment

eanders Oct 25, 2024

Choose a reason for hiding this comment

eanders Oct 25, 2024

Choose a reason for hiding this comment

eanders left a comment

Choose a reason for hiding this comment

gigxz Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

eanders Oct 29, 2024

Choose a reason for hiding this comment

dtgreiner commented Oct 23, 2024 •

edited

Loading

dtgreiner commented Oct 25, 2024 •

edited

Loading

gigxz Oct 29, 2024 •

edited

Loading