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

Allow editing of records pre-snapshot #4287

Merged
merged 6 commits into from
May 12, 2024
Merged

Allow editing of records pre-snapshot #4287

merged 6 commits into from
May 12, 2024

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Apr 19, 2024

This will create a new UpdateExisting event when a donation, purchase or distribution is updated if there isn't an existing event to compare against.

Ultimately we're going to want to remove this logic and lock down inventory line items from being edited more than X period of time from when they were created.

Need to let tests run, not sure if this causes failures. Will address next week if so (have to go now)

@dorner dorner requested a review from cielf April 19, 2024 20:58
@cielf cielf requested a review from awwaiid April 25, 2024 22:25
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has held up under light testing. I haven't managed to run the full gamut of all the event sourcing tests as I had intended to. Needs technical review still, @awwaiid.

@cielf
Copy link
Collaborator

cielf commented Apr 30, 2024

I should, however, check that when you edit a from-before itemizable a second time, it works properly. First thing on my next work session (probably tomorrow)

@cielf
Copy link
Collaborator

cielf commented May 1, 2024

I just ran into a very strange problem while editing a pre-snapshot on production data. Investigating further - but it came up complaining about an item that isn't in the distribution (though it looks like it was in the request). So tenatively withdrawing my ok on this one until we get that sussed out. Will put details in the spreadsheet.

@cielf
Copy link
Collaborator

cielf commented May 1, 2024

Never mind. it's an error re-running events issue. Which means I have to find someone that hasn't dropped down below zero on any inventory since the end of March to test this.

@cielf
Copy link
Collaborator

cielf commented May 1, 2024

@dorner Now I've got a real issue though. If you edit a pre-snapshot distribution, then edit it again, unexpected things happen. This was using prod data -- See recreation instructions in the event sourcing testing spreadsheet - currently line 111. When changing the second time, the inventory is dropping by the amount entered in the quantity rather than by the difference. I haven't checked the analagous on donation or purchase yet, but I would expect the same problem.

@cielf
Copy link
Collaborator

cielf commented May 1, 2024

For clarification -- that's dropping the quantity entered from the initial inventory level, not the inventory level after the initial edit. Which I suspect is a clue.

@dorner
Copy link
Collaborator Author

dorner commented May 3, 2024

OK - I have a test verifying this. Looks like I need to continue with the UpdateExisting flow if there's any existing UpdateExisting events. :( I'm going to be happy when we can finally kill this flow because it's a lot of unnecessary complexity.

@dorner
Copy link
Collaborator Author

dorner commented May 3, 2024

Pushed the fix.

@cielf
Copy link
Collaborator

cielf commented May 6, 2024

Seems to work now! @awwaiid Can you take a look from a technical pov?

@awwaiid
Copy link
Collaborator

awwaiid commented May 10, 2024

Looks good but I did fix a merge conflict; will approve/merge if that passes (or investigate otherwise).

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good :)

@awwaiid awwaiid merged commit 7dd5c81 into main May 12, 2024
20 checks passed
@awwaiid awwaiid deleted the 4243-edit-old-stuff branch May 12, 2024 16:49
@cielf
Copy link
Collaborator

cielf commented May 12, 2024

Also Hurrah!

Copy link
Contributor

@dorner: Your PR Allow editing of records pre-snapshot is part of today's Human Essentials production release: 2024.05.26.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants