refactor(badges): prevent orphaned BadgeProgress creation#2989
refactor(badges): prevent orphaned BadgeProgress creation#2989luisfelipec95 wants to merge 4 commits into
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @luisfelipec95! 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. |
There was a problem hiding this comment.
Thank you, @luisfelipec95, the solution is clear and straightforward.
I left a couple of suggestion for the method.
Before it can be merged this method also should be covered with test cases for new params/return combinations.
Also, please check CI lint failures. I think there are several whitespace issues.
|
|
||
| @classmethod | ||
| def for_user(cls, *, username, template_id): | ||
| def for_user(cls, *, username, template_id, create=True): |
There was a problem hiding this comment.
In general, I agree with this approach. But I suggest set create=False by default and pass True explicitly in signals handlers https://github.com/openedx/credentials/blob/master/credentials/apps/badges/signals/handlers.py#L53.
I also optionally propose renaming this parameter to create_if_absent, so it's a little more specific.
Also, please update a docstring and type annotations for this method with new implementation and contract, as the current docstring not really useful at all.
There was a problem hiding this comment.
Thanks for the feedback, I agree
| if create: | ||
| progress, __ = cls.objects.get_or_create(username=username, template_id=template_id) | ||
| return progress | ||
| else: |
There was a problem hiding this comment.
This else block can be removed as per linter recommendation
4 participants
Description
The badge processing system is creating an excessive number of orphaned
BadgeProgressrecords (records without associatedFulfillmentdata) due to a critical logic flaw. With 140+ active badge templates, this issue is significantly amplified, creating thousands of unnecessary database records daily.Changes
An optional
createparameter (default True) was added to thefor_user()method. This parameter controls whether the method should create a new BadgeProgress record when it doesn't exist, or simply retrieve it if it already exists.Previously, the method always created a record using
get_or_create(), with no option to just query. This meant any code that called this method, even just to check if a record existed, would end up automatically creating one.How to test
Configure Badge Template:
org.openedx.learning.course.passing.status.updated.v1org.openedx.learning.ccx.course.passing.status.updated.v1Create Badge Requirements:
is_passing = true)Create a test course in Studio:
Enroll test student in course
Before the changes
At the time of testing, there are 8 active badge templates, of which only 4 meet the rules for a completed course, yet 8 badgeProgress records are generated.
After the changes
It only creates in badgeProgress the necessary records—those that meet the badge template rules.