Release 1.145.0#3462
Closed
odlbot wants to merge 6 commits into
Closed
Conversation
Co-authored-by: Asad Ali <asad.ali@arbisoft.com>
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}
Comment on lines
+112
to
+120
| .annotate( | ||
| discount_count=Count( | ||
| Subquery( | ||
| Discount.objects.filter( | ||
| products__product__is_active=True, | ||
| products__product__content_type=courserun_content_type, | ||
| products__product__object_id__in=CourseRun.objects.filter( | ||
| b2b_contract=OuterRef("pk") | ||
| ).all(), |
There was a problem hiding this comment.
Bug: The get_queryset method uses an invalid Count(Subquery(...)) pattern for annotations, which will cause a runtime error when the queryset is evaluated.
Severity: CRITICAL
Suggested Fix
Refactor the annotations to use the correct pattern. The Count aggregation should be placed inside the Subquery itself, for example: Subquery(Model.objects.filter(...).annotate(c=Count('pk')).values('c')).
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/manager.py#L112-L120
Potential issue: In `ManagerContractViewSet.get_queryset`, the annotations for
`discount_count` and `enrollment_count` use an incorrect Django ORM pattern:
`Count(Subquery(...))`. The `Count` aggregation function expects a field name, not a
`Subquery` object. This will cause the application to raise a `FieldError` or a database
error at runtime whenever the API endpoint for listing or retrieving contracts is
accessed. This failure will prevent the B2B manager dashboard from displaying any
contract data, rendering the feature non-functional.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+238
to
+243
| ManagerEnrollmentCodeSerializer( | ||
| [discounts.order_by("id").first()], | ||
| context={"contract": contract}, | ||
| many=True, | ||
| ).data | ||
| ) |
There was a problem hiding this comment.
Bug: The codes action crashes with an AttributeError if a contract has unlimited seats but no discount codes, as it attempts to serialize a None object.
Severity: HIGH
Suggested Fix
Add a check to handle the case where discounts.order_by("id").first() returns None. If no discount code is found, return an empty list in the response instead of attempting to serialize None.
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/manager.py#L238-L243
Potential issue: In the `codes` action of `ManagerContractViewSet`, if a contract has
unlimited seats (`max_learners=0`) and no associated discount codes, the query
`discounts.order_by("id").first()` returns `None`. This `None` value is then wrapped in
a list and passed to `ManagerEnrollmentCodeSerializer(many=True)`. The serializer will
then raise an `AttributeError` when it attempts to access an attribute on the `None`
object, causing the API request to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+107
to
+110
| def get_queryset(self): | ||
| """Get the queryset; add some annotations/etc for computed fields""" | ||
| return ( | ||
| ContractPage.objects.select_related("organization") |
There was a problem hiding this comment.
Bug: The get_queryset method in ManagerContractViewSet incorrectly filters out contracts for superusers, returning an empty list instead of all accessible contracts.
Severity: HIGH
Suggested Fix
In get_queryset, add a condition to check if self.request.user.is_superuser. If true, return the base queryset without applying the filter that restricts results to the user's managed organizations, mirroring the logic in ManagerOrganizationViewSet.
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/manager.py#L107-L110
Potential issue: The `ManagerContractViewSet.get_queryset` method filters contracts by
checking if the request's user is an organization manager. However, it lacks a special
case for superusers. While the `IsOrganizationManager` permission class grants access to
superusers, the queryset filter will still be applied. Since superusers are often not
explicitly assigned as managers of an organization, this filter will cause the API to
return an empty list of contracts for them, effectively denying them access to data they
are authorized to see. This is a silent failure that impacts admin and troubleshooting
workflows.
Did we get this right? 👍 / 👎 to inform future reviews.
| from courses.models import CourseRun, CourseRunEnrollment | ||
| from ecommerce.models import Discount | ||
|
|
||
| courserun_content_type = ContentType.objects.get_for_model(CourseRun) |
There was a problem hiding this comment.
Bug: A database query is executed at the module level, which can cause the application to fail during startup if the database is not yet ready.
Severity: MEDIUM
Suggested Fix
Move the database query ContentType.objects.get_for_model(CourseRun) from the module level into the method or function where the courserun_content_type variable is actually used. This defers the query until it is needed at request time.
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/manager.py#L36
Potential issue: A database query, `ContentType.objects.get_for_model(CourseRun)`, is
executed at the module level. This query runs when the application starts and imports
the module, not when a request is handled. If the database is not yet available or
migrations have not been run during startup (a common scenario in containerized
deployments), this will cause a fatal `OperationalError` or similar exception,
preventing the application from starting successfully. This is an established
anti-pattern in Django development.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
annagav
James Kachel
Asad Ali