feat: checkout context BFF skeleton #778
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @iloveagent57! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
| # Eventually we'll use the BFF pattern to load and process data | ||
| # context_data, status_code = self.load_route_data_and_build_response( | ||
| # request, | ||
| # CheckoutContextHandler, | ||
| # CheckoutContextResponseBuilder | ||
| # ) |
There was a problem hiding this comment.
[Note] I wonder if request.user works with this BaseUnauthenticatedBFFViewSet? I think the answer is yes, but just wanted to note that subsequent PRs to actually implement this skeleton will depend on request.user getting set to the logged-in User in order to successfully extract lms_user_id to help calculate existing_customers_for_authenticated_user.
There was a problem hiding this comment.
Django gives us anonymous users out of the box, here's a breakpoint on an un-auth'd request:
(Pdb) request.user
<django.contrib.auth.models.AnonymousUser object at 0xf040653af920>
(Pdb) request.user.is_authenticated
False
There was a problem hiding this comment.
Gotcha, I wonder if setting viewset auth vars like this would help:
authentication_classes = [JwtAuthentication]permission_classes = [IsAuthenticated, AllowAny]
The idea is that unauthenticated requests would fail JwtAuthentication and get request.user = AnonymousUser but pass AllowAny permmission check, while authenticated requests would go through both JwtAuthentication -> IsAuthenticated.
Testing...
There was a problem hiding this comment.
Oh you only tested un-authed, so it might still work out of the box for authenticated users...I think we can address this in a separate PR.
There was a problem hiding this comment.
After local testing, it doesn't seem to work out of the box, but like I said I think we can address in a separate PR more specific to the BFF framework code.
I suspect that leveraging permission composability from DRF 3.9+ (we run 3.16 today) would make the solution quite simple with the | operator:
authentication_classes = [JwtAuthentication]
permission_classes = [IsAuthenticated | AllowAny]But this currently breaks internal logic within edx_rest_framework_extensions:
File "/Users/tsankey/.pyenv/versions/3.12.4/envs/enterprise-access/lib/python3.12/site-packages/edx_rest_framework_extensions/auth/jwt/middleware.py", line 333, in <genexpr>
issubclass(current_class, base_class) for current_class in iter_classes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: issubclass() arg 1 must be a class
| class FieldConstraintsSerializer(serializers.Serializer): | ||
| """ | ||
| Serializer for field constraints in checkout context. | ||
| """ | ||
| quantity = QuantityConstraintSerializer(help_text="Constraints for license quantity") | ||
| enterprise_slug = SlugConstraintSerializer(help_text="Constraints for enterprise slug") |
There was a problem hiding this comment.
Can you add a TODO to the docstring here to suggest that the implementer should expand the field constraints to more closely match the mins/maxes within this code block: https://github.com/edx/frontend-app-enterprise-checkout/blob/main/src/constants.ts#L13-L39
pwnage101
left a comment
pwnage101
left a comment
There was a problem hiding this comment.
Just a minor question and docstring suggestion---mostly stuff we can handle in subsequent PRs.
iloveagent57
force-pushed
the
aed/ssp-context-endpoint
branch
2 times, most recently
from
e1c5c6b to
573e4a8
Compare
iloveagent57
force-pushed
the
aed/ssp-context-endpoint
branch
from
573e4a8 to
579e6b3
Compare
ENT-10629
iloveagent57
force-pushed
the
aed/ssp-context-endpoint
branch
from
579e6b3 to
c3d72e9
Compare
ENT-10629
you'll need to set
ENABLE_CUSTOMER_BILLING_API = Truein yoursettings/private.pyfile to use this locally.Description:
Adds a skeleton
contextBFF view to support checkout contexts. Adds, but does not yet utilize, serializers for the expected response structure.Merge checklist:
./manage.py makemigrationshas been runPost merge: