human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

[BUG]: Editing stuff from before a snapshot doesn't work correctly

Open dorner opened this issue 1 year ago • 11 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Current Behavior

When a snapshot is taken and we enable an organization for event reading, editing or deleting a previous event does not do what we expect (i.e. adding or removing inventory).

Need to figure out what to do here - add events for all itemizables in the last X number of months? Lock down editing once a snapshot is made? Make a temporary "fake event" when handling these changes?

dorner avatar Mar 31 '24 18:03 dorner

I'm not sure we can reasonably impose locking down the editing once a snapshot is made -- so let's put that at the bottom of the list of possibles. Let's pull info about updated versus created on the itemizables and see how often, if ever, we get edits past, say, a quarter? That'll inform our answer, yes?

cielf avatar Apr 04 '24 21:04 cielf

This definitely looks like something that does happen, particularly with distributions.

diaper_dev_public_distributions.csv diaper_dev_public_donations.csv

diaper_dev_public_purchases.csv

dorner avatar Apr 05 '24 19:04 dorner

Huh. The extent of that really surprises me. I don't know what the business story around doing that would be.

cielf avatar Apr 05 '24 21:04 cielf

I'm wondering if there are updates that aren't inventory changes (like comments or something). Will dig a bit further into the data. I'm concerned if our assumption that people "never" make corrections on previous itemizables wasn't true. Might need to do some customer communication if so.

dorner avatar Apr 07 '24 01:04 dorner

Yes there are updates that aren't inventory changes -- potentially comments and the purchase prices / monetary donations-- but that there are updates more than a quarter ago still seems strange.

cielf avatar Apr 07 '24 02:04 cielf

It does look like there are several cases where an org has gone in and changed a slew of them in one day. (Edit it might be worth going in and looking at some of those -- because what????!?)

cielf avatar Apr 07 '24 02:04 cielf

Wellll, Looking at organization 3, from the comments, it looks like they actually were changing the partners (WOT?!). A case where they had multiple similarly named partners and/ or were consolidating them or something.

I gotta say, our users do not cease to amaze.

cielf avatar Apr 07 '24 17:04 cielf

We only started keeping track of versions in July of 2023. (But I think we can ignore anything earlier than that anyway as irrelevant.)

Unfortunately, because of how we save line items, there is no way to tell if line items were updated or created at that time (line items are always blown away and recreated on update). But there are definitely some inventory changes happening.

dorner avatar Apr 12 '24 20:04 dorner

@cielf it might be worth it to reach out to some of the orgs that did changes after many months and find out why and how their changes interacted with inventory. I think it is almost a certainty that changing a 6-month-old distribution shouldn't be assumed that the changes are happening right now (and in fact that might be yet another cause of some of our errors).

This might go back to the idea of "make a correction" to put the event time at the right point.

This seems to indicate that the whole idea of snapshots might not work for our use case. If people can legitimately edit things that old, there will never be a good time to make a snapshot that's guaranteed to be correct.

In that case, we should scrap the idea of using snapshots except as "this is when event sourcing started" and optionally think about using view models / projections instead for performance purposes (if/when we need to).

And it also implies that for this particular problem of editing something that existed before we turned on event sourcing, all we need to do is:

  • Check to see if there are any events at all for this thing
  • If not, create a special "UpdateExisting" event (attached to the same thing) rather than the "normal" event
  • This would have diffs using the old and new values in the database, rather than absolute values like we'd usually have, and we could use that to manage the inventory.

Probably worth a chat on Sunday @cielf @awwaiid @scooter-dangle

dorner avatar Apr 12 '24 20:04 dorner

The UpdateExisting approach sounds plausible to me, FWIW.

cielf avatar Apr 13 '24 13:04 cielf

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

github-actions[bot] avatar May 14 '24 00:05 github-actions[bot]

Addressed by #4287

cielf avatar May 14 '24 14:05 cielf