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

feat(tenants): expose booking limits in the API #164

Merged
merged 10 commits into from
Mar 16, 2022

Conversation

chillfox
Copy link
Contributor

@chillfox chillfox commented Mar 15, 2022

  • return booking_limits in #index
  • add #current_limits, #show_limits, #update_limits
  • add limit_override query param to Bookings #create and #update

@chillfox chillfox self-assigned this Mar 15, 2022
@chillfox chillfox added the type: enhancement new feature or request label Mar 15, 2022
@chillfox chillfox linked an issue Mar 15, 2022 that may be closed by this pull request
@@ -39,6 +39,22 @@ class Tenants < Application
Tenant.find!(params["id"].to_i64).delete
end

get "/current_limits", :current_limits do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tassja how can I expose this route to all logged-in users? (#161)

Copy link
Contributor

@tassja tassja Mar 15, 2022

Choose a reason for hiding this comment

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

Is that route not exposed to all users? There isn't any auth or log in checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would assume that the before_action :admin_only applies to all routes in the controller, so I guess my question is, how do I exempt the route from that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry missed that

Copy link
Contributor

Choose a reason for hiding this comment

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

before_action :admin_only, except: :current_limits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@chillfox chillfox marked this pull request as ready for review March 15, 2022 23:14
@@ -1,7 +1,7 @@
class Tenants < Application
base "/api/staff/v1/tenants"

before_action :admin_only
before_action :admin_only, except: [:current_limits, :show_limits]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a test for this to ensure any user can access

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I make a request as a normal user?
I noticed that all of the mock tokens have got permissions: UserJWT::Permissions::Admin, so I tried creating one that was set to User but that didn't make any difference.

Copy link
Member

Choose a reason for hiding this comment

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

probably no need to test for this as it's being tested at the spider-gazelle level

@@ -81,8 +81,10 @@ class Bookings < Application
booking.booked_by_name = user.name

# check concurrent bookings don't exceed booking limits
booking_limits = check_booking_limits(tenant, booking)
render :conflict, json: booking_limits if booking_limits
unless query_params["limit_override"]? == "true"
Copy link
Member

Choose a reason for hiding this comment

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

As per the comment https://github.com/place-technology/suncorp-ntt/issues/175#issuecomment-1068824084
we want to allow a per-user custom limit using this param

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

@stakach stakach merged commit 3bff5d0 into master Mar 16, 2022
@stakach stakach deleted the feat/expose_booking_limits branch March 16, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose tenant booking limits in the API
3 participants