4052 disallow deleting transfers if audit #4057

MooseCowBear · 2024-01-24T13:01:02Z

Resolves #4052

Description

This PR adds a method deletable? to the Transfer model to check if any audits have occurred 1. since the transfer was created at either the transfer's to location or its from location, and 2. that the audit involved items that were transferred. It updates the transfer_row partial to only display a delete button if there are no such audits.

It also adds a check that the transfer is deletable in the DestroyTransferService so that the controller wouldn't destroy a transfer that shouldn't be deleted. (If you decide you don't want this, I'm happy to remove it.)

The query for audits is kind of beastly. I couldn't think of a better way that would keep the db queries to a minimum.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I added unit tests for correctness of the method and the change to the service, and a system test for the conditional delete button rendering.

Screenshots

…heck

dorner

Thanks for the contribution! I had some suggestions.

dorner · 2024-01-24T21:50:19Z

app/models/transfer.rb

@@ -56,6 +56,11 @@ def csv_export_attributes
    ]
  end

+  def deletable?
+    item_ids = line_items.pluck(:item_id)
+    Audit.where(storage_location_id: [to_id, from_id]).where(created_at: created_at..).joins(:line_items).where(line_items: {item_id: item_ids}).empty?


Can we split this line up to make it more readable?

Also, I'm not sure this belongs inside Transfer since it really isn't checking transfers at all. I think it'd make more sense to move this to the Audit class as a method, e.g. Audit.since?(self) and it can take an Itemizable as its parameter.

dorner · 2024-01-24T21:50:43Z

app/services/transfer_destroy_service.rb

@@ -4,6 +4,8 @@ def initialize(transfer_id:)
  end

  def call
+    raise StandardError.new "Transfer cannot be deleted" unless transfer.deletable?


You don't need to explicitly call out StandardError:

raise "Transfer cannot be deleted" unless transfer.deletable?

But can you add more information here? "Transfer cannot be deleted since an audit was performed after it."

dorner · 2024-01-24T21:53:43Z

spec/system/transfer_system_spec.rb

@@ -135,5 +135,21 @@ def create_transfer(amount, from_name, to_name)
    end

    it_behaves_like "Date Range Picker", Transfer
+
+    it "only displays delete button for deletable transfers" do


Since this doesn't have any interaction, it can be a request spec (which uses a lot less resources).

MooseCowBear · 2024-01-25T12:12:03Z

I'll get to work on the changes. One question @dorner, how do you feel about splitting up the query in Audit like:

def self.at(*location_ids)
  where(storage_location_id: location_ids)
end

def self.since?(itemizable)
  item_ids = itemizable.line_items.pluck(:item_id)
  where(created_at: itemizable.created_at..)
    .joins(:line_items)
    .where(line_items: {item_id: item_ids})
    .empty?
end

And then transfer's deletable method could be:

def deletable?
  Audit.at(to_id, from_id).since?(self)
end

Since I believe only transfers have both a to and from location?

dorner · 2024-01-25T13:38:19Z

I don't think we need to split it up since we only have one use case. But you can pass in a list of location IDs as an array.

…er test, add test for audit query method

…ormative

dorner · 2024-02-02T20:19:10Z

app/services/transfer_destroy_service.rb

@@ -4,6 +4,8 @@ def initialize(transfer_id:)
  end

  def call
+    raise "Transfer cannot be deleted because an audit was performed after it" unless transfer.deletable?


Hmm... I'm torn here because the name of the method is generic ("can this be deleted") but the error assumes there is only one possible reason the audit couldn't be deleted. I think it's OK to just have this service call Audit.since? directly rather than deletable?

You mean that the transfer couldn't be deleted, yes?

@dorner Originally, I was thinking that a delete button would only appear if the transfer met the criteria for deletability and that the view might look cleaner with a deletable? method, but since that's not the case there doesn't seem to be a reason for it.

dorner · 2024-02-02T20:27:43Z

app/views/transfers/_transfer_row.html.erb

@@ -5,7 +5,9 @@
  <td><%= transfer_row.comment || "none" %></td>
  <td><%= transfer_row.line_items.total %></td>
  <td>
-    <%= delete_button_to transfer_path(transfer_row.id), { confirm: "Are you sure you want to permanently remove this transfer?" } %>
+    <% if transfer_row.deletable? %>


In general we don't do validations before we perform the action. @cielf would it be OK to only have this raise an error and not try to change the view beforehand? I think this is an edge case and I don't want to overthink this.

It's definitely an edge case. I'm ok with that -- we need to be pretty clear on why they can't in the error, though.

dorner · 2024-02-02T20:27:43Z

app/views/transfers/_transfer_row.html.erb

@@ -5,7 +5,9 @@
  <td><%= transfer_row.comment || "none" %></td>
  <td><%= transfer_row.line_items.total %></td>
  <td>
-    <%= delete_button_to transfer_path(transfer_row.id), { confirm: "Are you sure you want to permanently remove this transfer?" } %>
+    <% if transfer_row.deletable? %>


In general we don't do validations before we perform the action. @cielf would it be OK to only have this raise an error and not try to change the view beforehand? I think this is an edge case and I don't want to overthink this.

It's definitely an edge case. I'm ok with that -- we need to be pretty clear on why they can't in the error, though.

@cielf Is "Transfer cannot be deleted because an audit was performed after it" a clear enough message? Or maybe something more specific like "This transfer cannot be deleted because an audit involving its items has already been performed"? I can't tell if being more specific makes the error message clearer.

@MooseCowBear I don't want to spend too much time on the wording because this is a bit of an edge case. How about "We can't delete this transfer because its items were audited since you made the transfer."

…t.since?

dorner · 2024-02-07T16:24:31Z

Looks good - thanks!

github-actions · 2024-02-11T15:38:04Z

@MooseCowBear: Your PR 4052 disallow deleting transfers if audit is part of today's Human Essentials production release: 2024.02.11.
Thank you very much for your contribution!

MooseCowBear added 5 commits January 23, 2024 17:23

add tests for deletable method and appearance of delete buttons

84f09d9

add deletable method

74107f0

add conditional to render delete button only if deletable

1349fc8

add check for deletability in transfer destroy service and test for c…

6d5be80

…heck

Merge updated upstream

501301d

dorner requested changes Jan 24, 2024

View reviewed changes

MooseCowBear added 4 commits January 25, 2024 16:24

Change test for delete button presence to request spec, update transf…

7287e96

…er test, add test for audit query method

Move query to Audit class and update transfer deletable

5a59317

Remove unnecessary reference to standard error, make message more inf…

2e15129

…ormative

Update branch to upstream

82e795c

MooseCowBear requested a review from dorner February 1, 2024 18:19

dorner requested changes Feb 2, 2024

View reviewed changes

MooseCowBear added 2 commits February 5, 2024 12:57

remove deletable from transfer, update tests, replace usage with Audi…

c17cc12

…t.since?

Update branch from upstream

749e990

MooseCowBear requested a review from dorner February 5, 2024 18:09

dorner approved these changes Feb 7, 2024

View reviewed changes

dorner merged commit d5e0341 into rubyforgood:main Feb 7, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4052 disallow deleting transfers if audit #4057

4052 disallow deleting transfers if audit #4057

MooseCowBear commented Jan 24, 2024

dorner left a comment

dorner Jan 24, 2024

dorner Jan 24, 2024

dorner Jan 24, 2024

dorner Jan 24, 2024

MooseCowBear commented Jan 25, 2024

dorner commented Jan 25, 2024

dorner Feb 2, 2024

cielf Feb 2, 2024

MooseCowBear Feb 3, 2024

dorner Feb 2, 2024

cielf Feb 2, 2024

dorner Feb 2, 2024

cielf Feb 2, 2024

MooseCowBear Feb 3, 2024

cielf Feb 5, 2024

dorner commented Feb 7, 2024

github-actions bot commented Feb 11, 2024

4052 disallow deleting transfers if audit #4057

4052 disallow deleting transfers if audit #4057

Conversation

MooseCowBear commented Jan 24, 2024

Description

Type of change

How Has This Been Tested?

Screenshots

dorner 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

MooseCowBear commented Jan 25, 2024

dorner commented Jan 25, 2024

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

dorner commented Feb 7, 2024

github-actions bot commented Feb 11, 2024