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.

Sign up for GitHub

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

Jump to bottom

feat(bookings): add configurable limits on booked assets #145

Merged
merged 14 commits into from
Jan 23, 2022
Merged

feat(bookings): add configurable limits on booked assets #145

merged 14 commits into from
Jan 23, 2022

Conversation

chillfox
Copy link
Contributor

Add configurable booking limits.

@chillfox chillfox linked an issue Jan 18, 2022 that may be closed by this pull request
add support for configurable booking limits on resources #142
Closed
@chillfox chillfox requested a review from tassja January 18, 2022 05:52
tassja
tassja reviewed Jan 19, 2022
View reviewed changes
spec/controllers/bookings_spec.cr Outdated Show resolved Hide resolved
src/controllers/bookings.cr Outdated Show resolved Hide resolved
src/controllers/bookings.cr Show resolved Hide resolved
@tassja
Copy link
Contributor

tassja commented Jan 19, 2022 •
edited
Loading

Also our PR names follow a similar format to commits, something like feat(bookings): add configurable limits on booked assets would be good for this one

@chillfox chillfox changed the title Feat/booking limits feat(bookings): add configurable limits on booked assets Jan 19, 2022
@chillfox
Copy link
Contributor Author

Also our PR names follow a similar format to commits, something like feat(bookings): add configurable limits on booked assets would be good for this one

Ok, I have updated the name.

@chillfox chillfox marked this pull request as ready for review January 19, 2022 04:52
@tassja tassja requested a review from stakach January 19, 2022 05:42
@caspiano caspiano added the type: enhancement new feature or request label Jan 19, 2022
caspiano
caspiano reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@caspiano caspiano left a comment

Choose a reason for hiding this comment

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

This looks really great!
I think the only thing missing is some validation of booking_limits.
You're handling the JSON::Any column safely, but I think catching unexpected JSON at the ORM level would be good.
This can be achieved by either traversing the JSON::Any at validation time or creating a type converter for the column (i.e. Hash(String, Int32))

@chillfox
Copy link
Contributor Author

@caspiano I tried using a converter, but it broke a bunch of tests, so I have added a method to validate.

caspiano
caspiano suggested changes Jan 20, 2022
View reviewed changes
src/models/tenant.cr Outdated Show resolved Hide resolved
src/models/tenant.cr Outdated Show resolved Hide resolved
src/models/tenant.cr Outdated Show resolved Hide resolved
chillfox and others added 3 commits January 20, 2022 16:21
@chillfox
refactor(bookings): booking_limits validation
0a85349 
Co-authored-by: Caspian Baska <caspian@place.tech>
@chillfox
docs(bookings): change comment
8bc5395 
Co-authored-by: Caspian Baska <caspian@place.tech>
@chillfox
refactor(bookings): move validate_booking_limits to earlier during va…
3993afb 
…lidation
caspiano
caspiano approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@caspiano caspiano left a comment

Choose a reason for hiding this comment

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

Great work!

stakach
stakach approved these changes Jan 23, 2022
View reviewed changes
Copy link
Member

@stakach stakach left a comment

Choose a reason for hiding this comment

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

LGTM

tassja
tassja approved these changes Jan 23, 2022
View reviewed changes
Copy link
Contributor

@tassja tassja left a comment

Choose a reason for hiding this comment

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

Awesome job!!

@stakach stakach merged commit 0a59bd6 into master Jan 23, 2022
@stakach stakach deleted the feat/booking_limits branch January 23, 2022 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stakach stakach stakach approved these changes

@tassja tassja tassja approved these changes

@caspiano caspiano caspiano approved these changes

Assignees

@chillfox chillfox

Labels
type: enhancement new feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add support for configurable booking limits on resources
4 participants
@chillfox @tassja @stakach @caspiano