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

[WIP]#3090 Allow essential banks to review their distribution before submitting it #3703

Closed
wants to merge 14 commits into from

Conversation

lokisk1155
Copy link
Collaborator

Resolves #3090

PR up early to take advantage of github action bot :)

@lokisk1155
Copy link
Collaborator Author

@dorner Early thoughts on my implementation here? It's "working", but still need to work on it to get it passing tests

@dorner
Copy link
Collaborator

dorner commented Jul 12, 2023

I'm a little concerned at storing it in the session - things like "comment" and line items could be really large and might blow past the cookie size. I wonder if we can instead do something like

  • Create the distribution, but add a state like "in creation" or "pending"
  • Update the default scope to exclude these
  • Send the ID forward to the confirmation page
  • On confirmation, update the state to "scheduled"

This does mean we might end up with some garbage data - maybe we could add a cleanup session to delete any "in creation" distributions that are more than a month old or something.

@cielf thoughts?

@lokisk1155
Copy link
Collaborator Author

@dorner ok that makes sense. Not exactly sure what a cleanup session would look like: to delete unscheduled distributions after a certain period of time

@cielf
Copy link
Collaborator

cielf commented Jul 12, 2023

One question being what is a reasonable amount of time for these to hang around in an unconfirmed state? I can (barely) see someone walking away over a weekend, but not over a week.

@cielf
Copy link
Collaborator

cielf commented Jul 12, 2023

Do we have any scopes that currently work with an exclusion that would have to be expanded to also exclude these?

@dorner
Copy link
Collaborator

dorner commented Jul 12, 2023

Well... technically we could leave it forever 😛 it's just garbage to clean up at some point.

It does not appear that there are any current scopes other than the ones already provided (scheduled, complete). We might need to update the one place that uses the scheduled scope 😄

@cielf
Copy link
Collaborator

cielf commented Jul 13, 2023

Yeah, technically we could leave it forever, but I'd prefer to make it a rather short time. What would the consequence be if someone walked away from the confirmation screen and the cleanup job ran in the meantime?

@dorner
Copy link
Collaborator

dorner commented Jul 14, 2023

That's why we should give it at least some time. If someone walked away and came back a week later, they'd have lost their entries, which I think is probably OK 😁

@lokisk1155
Copy link
Collaborator Author

Sorry I've been very busy - gonna get cooking on this again tmmr :)

@dorner
Copy link
Collaborator

dorner commented Mar 8, 2024

Closing this due to inactivity.

@dorner dorner closed this Mar 8, 2024
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.

[FEATURE] Allow essential banks to review their distribution before submitting it
3 participants