only respond with enrolled contract on enrollment code redemption#3551
only respond with enrolled contract on enrollment code redemption#3551
Conversation
…only return the contract you were enrolled in or null
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| else None | ||
| ) | ||
|
|
||
| if contracts_attached: | ||
| return Response(serialized_contracts, status=status.HTTP_201_CREATED) | ||
| return Response(serialized_contract, status=status.HTTP_201_CREATED) | ||
|
|
||
| if contract_full: |
There was a problem hiding this comment.
Bug: The _get_eligible_contracts function is missing an active=True filter, allowing users to join inactive contracts and causing the API to return a null response, violating its schema.
Severity: HIGH
Suggested Fix
Add an active=True filter to the contract query within the _get_eligible_contracts function. This will ensure users can only be attached to active contracts, aligning the business logic with the response generation and preventing the schema violation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: b2b/views/v0/__init__.py#L208-L214
Potential issue: The `_get_eligible_contracts` function, which identifies contracts a
user can join, omits a filter for `active=True`. This allows a user with a valid
enrollment code to be attached to an inactive contract. Subsequently, the
`_get_active_code_user_contract` function, which was added in the pull request to fetch
the contract for the response, correctly filters for active contracts. Since the user
was attached to an inactive one, this function returns `None`. As a result, the endpoint
returns a `null` body with a `201 Created` status, violating the OpenAPI schema that
expects a `ContractPage` object and potentially causing client-side errors.
Did we get this right? 👍 / 👎 to inform future reviews.
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Will leave rest of review to someone else, but i was just looking at the API schema concern and have a concern. Let me know if i am incorrect!
| - 404: Invalid or expired enrollment code | ||
| - 409: Code valid but no available seats in associated contract(s) | ||
| - list of ContractPageSerializer - the contracts for the user | ||
| - ContractPageSerializer (or null) - the active contract associated with the code |
There was a problem hiding this comment.
@gumaerc This is a breaking change in the API, right? Previously returned an array of objects, now returns a single object. I think that will break Learn here: https://github.com/mitodl/mit-learn/blob/6063a3d2dab070229ce3ff3dbb4f235eeb64ad5b/frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx#L45-L47
I agree that returning a single object is more natural.
But to avoid breaking the schema—which is consequential here—I would suggest we return [singleObject]
dsubak
left a comment
dsubak
left a comment
There was a problem hiding this comment.
Functionality looks good to me, was able to verify that I only get back the contract related to the code passed in in cases where I would've gotten everything back before.
Looks like @ChristopherChudzicki feedback has been addressed as well, so if ya'll are happy with it I think this'll play
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
I haven't tested (left to dan + you) but the spec change is fixed, thanks!
3 participants
What are the relevant tickets?
Pre-requisite for https://github.com/mitodl/hq/issues/11112
Description (What does it do?)
This PR changes the B2B enrollment code attach endpoint to return only the contract that you have enrolled in that is tied to the enrollment code you are redeeming. Previously it returned a list of all the users contracts every time.
How can this be tested?