Skip to content

refactor(badges): prevent orphaned BadgeProgress creation #2989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luisfelipec95 wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −5
Draft

refactor(badges): prevent orphaned BadgeProgress creation #2989

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions credentials/apps/badges/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,15 @@ def user_progress(self, username: str) -> float:
"""
Determines a completion progress for user.
"""
progress = BadgeProgress.for_user(username=username, template_id=self.id)
progress = BadgeProgress.for_user(
username=username,
template_id=self.id,
create=False
)

if not progress:
return 0.00

return progress.ratio

def is_completed(self, username: str) -> bool:
Expand Down Expand Up @@ -274,7 +282,15 @@ def is_group_fulfilled(cls, *, group: str, template: BadgeTemplate, username: st
Checks if the group is fulfilled.
"""

progress = BadgeProgress.for_user(username=username, template_id=template.id)
progress = BadgeProgress.for_user(
username=username,
template_id=template.id,
create=False
)

if not progress:
return False

requirements = cls.objects.filter(template=template, blend=group)
fulfilled_requirements = requirements.filter(fulfillments__progress=progress).count()

Expand Down Expand Up @@ -509,13 +525,16 @@ def __str__(self):
return f"BadgeProgress:{self.username}"

@classmethod
def for_user(cls, *, username, template_id):
def for_user(cls, *, username, template_id, create=True):

@GlugovGrGlib GlugovGrGlib Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Service shortcut.
"""

progress, __ = cls.objects.get_or_create(username=username, template_id=template_id)
return progress
if create:
progress, __ = cls.objects.get_or_create(username=username, template_id=template_id)
return progress
else:

@GlugovGrGlib GlugovGrGlib Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else block can be removed as per linter recommendation

return cls.objects.filter(username=username, template_id=template_id).first()

@property
def ratio(self) -> float:
Expand Down
Loading