5262. Disallow editing of inventory past a snapshot #5273
Conversation
|
Hey @dorner -- Let's discuss on Sunday whether consistency dictates that we disallow reclaims. (they are so simple) |
|
In the case where they've already had an audit and it's before the snapshot, we could happily suppress the message about the audit as useless (this was on purchase 4151 from MHM, but I think we do the same on donations and distributions). |
|
Problem (as opposed to thing that needs discussion) 1: Same thing happens if you change the comment. |
|
Problem 2: Problem 3: |
|
@cielf I've fixed the audit warnings and the disallowing of editing with no inventory changes (turns out if you disable everything, it doesn't send any parameters at all for it!) Did we discuss the reclaiming? I don't remember. |
|
It looks like we did, but I don't remember what we decided either. Adding it for this week. |
|
So, adding a note here to confirm what we discussed -- that we are disallowing the reclaiming of distributions entered before before the snapshot. |
|
I'm only part way through the testing -- I've noticed one thing that's jarring. Some, at least, of the error messaging is a bit awkward or misleading. Recording examples here to inform action; -- Donation 22692 failed to be removed because Cannot delete this donation because it has an intervening snapshot. Can we make it so that we have the same pattern for all of the itemizables? (Is that out of scope?) I'm trying to think about how to express it best to the users -- who might not know what a snapshot is, and think they've done something wrong. At the moment, I like the following pattern -- but I'm open to suggestions: "We can't delete Donation 22892. This donation was entered before the latest inventory snapshot (2024/05/26)." Or merely, "We can't delete Donations entered before [date of snapshot]. Or "We can't delete this Donation because it is too old" Thoughts? I like including the ID for support reasons. |
|
it's looking good other than that, though. |
|
@cielf tried to fix as much as I could. Let me know if there's anything else needed. |
|
I didn't get to this in today's session -- will look at it Monday -- I think we're at a point where we can pass it to @awwaiid for the technical pass, though. |
|
I need to apologize for calling out the error messaging - it distracted me from how we should actually be handling this. We should be consistent in how we handle distributions, donations, and purchases -- that is, we should also be disabling the item quantity edits on purchases and donations, right? Or am I missing something? |
|
@cielf if I'm reading this change correctly it has a core implementation on Event itself that prevents inventory edits on anything created before a snapshot, and most of the rest is making that friendly. So it should prevent purchases and donations too. Mind you I've only read the code, not run it yet :) |
|
@awwaiid I was referring to the UI -- IIRC when last I looked at this, the user was prevented from attempting inventory changes on old distributions, but not on old donations and purchases -- those were caught at save. |
|
@cielf this should work for all three types. |
…ntervening snapshot
dorner
force-pushed
the
5262-editing-old-inventory
branch
from
c6fe3e6 to
79ed827
Compare
app/events/snapshot_event.rb
Outdated
| # @param record [#organization_id, #created_at] | ||
| # @return [SnapshotEvent, nil] the intervening snapshot event or null if there aren't any | ||
| def self.intervening(record) | ||
| where(organization_id: record.organization_id, event_time: record.created_at..).first |
There was a problem hiding this comment.
Do we need a sort to reliably get the first snapshot after record create? Also, should we get the most recent? Or does neither matter and we are wanting to know if there is ANY intervening?
There was a problem hiding this comment.
Fixed! It should always get the most recent one.
|
|
||
| it "does not delete the donation if there is an intervening snapshot" do | ||
| data = EventTypes::Inventory.new(storage_locations: {}, organization_id: organization.id) | ||
| travel(-1.day) do |
There was a problem hiding this comment.
Is there any reason that sometimes we time-travel to back date the SnapshotEvent and sometimes we put an older timestamp as the created_at/event_time?
There was a problem hiding this comment.
Not particularly. Just how the tests were written.
3 participants
This PR makes it so that donations, distributions and purchases can no longer have inventory changes once a snapshot event exists between its creation date and the current time.
This includes both internal model validations as well as UI indicators that show that editing inventory can no longer happen, while still allowing editing of other values.