- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15
feat: update create billing portal session api #860
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
Conversation
322882f    to
    8b303e8      
    Compare
  
    | logger.error(f"No stripe customer id associated to CheckoutIntent {checkout_intent}") | ||
| return Response(customer_portal_session, status=status.HTTP_404_NOT_FOUND) | ||
|  | ||
| if not checkout_intent: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting here and until the end of the function, there a lot of code that is common between the two new views.  I'd recommend making this DRY and consolidating as much common code into a single function, possibly an internal python API function stored in the customer_billing domain (e.g. enterprise_access/apps/customer_billing/api.py)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our goal was just to get into a functional state to unblock frontend development. There's a handful of things that can be cleaned up on these two views/actions.
| logger.exception( | ||
| f"StripeError creating billing portal session for CheckoutIntent {checkout_intent}: {e}", | ||
| ) | ||
| return Response(customer_portal_session, status=status.HTTP_422_UNPROCESSABLE_ENTITY) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
422 Unprocessable Entity usually indicates only something wrong with the request. It's not a guarantee that stripe.error.StripeError will get raised only for issues with the request, so always returning 422 would be misleading.
I'd recommend not catching the exception, and letting DRF's default error boundary convert this into a 500 server error response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if an exception is caught, customer_portal_session will be null still, so it doesn't make much sense to include it in the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning msg string instead of None. 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 422 was replaced from a 502 status for Stripe based exception and 500 status for general exceptions in an earlier implementation after seeking feedback.
I added a TODO to be more explicit when utilizing the Stripe error class that can be done in followup work. I think the Stripe specific exception is a good way to quickly whether the errors are happening internally or via the Stripe API and I am in leaning towards keeping it for now. Lmk if its a hard blocker and I can remove it 👍🏽
668c3da    to
    7ae2c78      
    Compare
  
    4b55824    to
    72a2ed9      
    Compare
  
    | CUSTOMER_BILLING_CREATE_PORTAL_SESSION_PERMISSION, | ||
| fn=lambda request, **kwargs: request.GET.get('enterprise_customer_uuid') or kwargs.get( | ||
| 'enterprise_customer_uuid') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iloveagent57 , @pwnage101  - Sanity check: The fn in @permission_required now pulls enterprise_customer_uuid from request.GET (query param) instead of only kwargs, so RBAC actually evaluates the right enterprise UUID instead of None (which caused blanket 403s before).
Without this, every call failed early with 403 because the permission check never saw the UUID, blocking view logic from running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, right, that makes sense.
| Nice job, I think we can improve this in the future to make it align with standard DRF patterns, namely moving the  | 
Description:
This pull request introduces significant improvements and new tests for the customer billing API endpoints, focusing on the creation of Stripe billing portal sessions for both enterprise admins and learners. The changes include new comprehensive API tests, refactoring of the view logic for portal session creation, improved permission handling, and minor admin interface enhancements.
Key changes include:
1. New and Improved API Tests
test_customer_billing.pyto cover all major cases for admin and checkout portal session creation, including success, authentication, permission, error handling, and edge cases.2. Customer Billing API View Refactor
create_enterprise_admin_portal_session) and one for learners/checkout (create_checkout_portal_session), each with dedicated permission checks and error handling.3. Permissions and Security
CheckoutIntentPermissionto ensure only the correct user can access their checkout intent for portal session creation, returning 403 when unauthorized.4. Django Admin Improvements
CheckoutIntentAdminconfiguration to includeenterprise_uuidandstripe_customer_idfields in the admin interface for better visibility and management of checkout intents. [1] [2]Jira:
ENT-XXXX
Merge checklist:
./manage.py makemigrationshas been runPost merge: