Merged
Conversation
OpenAPI ChangesShow/hide 175 changes: 0 error, 0 warning, 175 infoUnexpected 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
+1128
to
+1130
| direct_learning_resource_id = serializers.IntegerField( | ||
| source="direct_learning_resource.id", required=False, allow_null=True | ||
| ) |
There was a problem hiding this comment.
Bug: The direct_learning_resource_id field uses a dotted source direct_learning_resource.id but lacks a default value, which can cause an AttributeError when direct_learning_resource is None.
Severity: HIGH
Suggested Fix
Add default=None to the direct_learning_resource_id field definition in ContentFileSerializer. This ensures that DRF will correctly handle cases where the intermediate direct_learning_resource object is None without raising an exception.
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: learning_resources/serializers.py#L1128-L1130
Potential issue: The `ContentFileSerializer` defines `direct_learning_resource_id` with
`source="direct_learning_resource.id"`. The related `ContentFile` model allows
`direct_learning_resource` to be `None`. When serializing a `ContentFile` instance where
this foreign key is `None`, Django REST Framework will attempt to access `.id` on a
`None` object, raising an `AttributeError`. The use of `allow_null=True` without an
explicit `default=None` is insufficient to prevent this error with a dotted source path.
This will cause API endpoints that serialize these objects to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
459
to
+462
|
|
||
| all_filter_clauses[filter_name] = {"bool": {"should": clauses_for_filter}} | ||
|
|
||
| if search_params.get("endpoint") == LEARNING_RESOURCE and not search_params.get( |
There was a problem hiding this comment.
Bug: The implicit show_ocw_files filter is incorrectly applied to aggregation queries, causing aggregation counts to be silently filtered and therefore inaccurate.
Severity: MEDIUM
Suggested Fix
Modify generate_aggregation_clauses to explicitly exclude the filter generated by show_ocw_files from the other_filters list that is applied to aggregation buckets. This will ensure aggregation counts reflect the total dataset, independent of this display-oriented filter.
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: learning_resources_search/api.py#L459-L462
Potential issue: In the search API, the `show_ocw_files` parameter, which defaults to
`False`, generates a filter clause. This clause is incorrectly included when calculating
aggregation counts for other facets (like `resource_type`). As a result, when a user
performs a search without explicitly setting `show_ocw_files=True`, the aggregation
counts will be silently and incorrectly reduced, as they will exclude certain OCW
resources. This provides misleading information to the user about the total number of
available resources for each aggregation bucket.
Did we get this right? 👍 / 👎 to inform future reviews.
odlbot
added
waiting for checkboxes
all checkboxes checked
and removed
deploying to rc
waiting for checkboxes
labels
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
3 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.
Matt Bertrand
Anastasia Beglova