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

Flatten Pickup into Reservation #1489

Merged
merged 4 commits into from
May 20, 2024
Merged

Conversation

jim
Copy link
Member

@jim jim commented May 11, 2024

What it does

This is a stacked PR that should be merged after #1488 lands. The new code is in 44e09e6.

After looking more closely at how both Reservation and Pickup had status columns and thinking about how we'd model the logic between the two of them, I decided that it's going to be best to have a single status field track the entire reservation lifecycle. Accordingly, the Pickup model doesn't provide much except a level of indirection.

Note that I went the brute force route in the migration. It doesn't seem worth it to write a data migration for any existing Pickup or ReservationLoan data this early in development.

jim added 3 commits May 9, 2024 23:21
After looking more closely at how both Reservation and Pickup had status columns and thinking about how we'd model the logic between the two of them, I decided that it's going to be best to have a single status field track the entire reservation lifecycle.
@jim jim requested a review from phinze May 11, 2024 03:28
@jim jim changed the title Flatten pickup into Reservation Flatten Pickup into Reservation May 11, 2024
Copy link
Contributor

@phinze phinze left a comment

Choose a reason for hiding this comment

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

This makes sense to me - it seems more straightforward to model this as just one state machine as opposed to two interlocking ones.

@jim jim merged commit 16ab700 into main May 20, 2024
9 checks passed
@jim jim deleted the jim/flatten-pickup-into-reservation branch May 20, 2024 23:46
jim added a commit that referenced this pull request May 22, 2024
# What it does

This is a stacked PR that should be merged after #1489. The new code is:

* Initial Reservation state machine added in
a6f1d6e
* Updates to the UI to drive it from the state machine in
a62dbef and
52b0feb)

# Implementation notes

I wanted to try to model the states and transitions for a reservation in
a single place. It's a somwhat linear path, with a few points of
branching. We'll always want to preserve an escape hatch for strange
situations, but I think this might be a good way to consolidate the
logic of what can happen when throughout a Reservation's lifecycle.

I'm fairly happy with this approach so far. It remains to be seen how
the state machine and other conditionals will interact- for example,
what if you have to have added a certain number of associated items
before it's OK to transition to a new state?
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

2 participants