Skip to content
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

Jump to bottom

Add notifications when a badge is awarded #3442

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Add notifications when a badge is awarded #3442

wants to merge 10 commits into from

Conversation

harmitgoswami
Copy link
Collaborator

Fix #3412

This PR adds notifications to signal to a user that they received a new badge/badge level. Notifications will appear in both the "Notifications Menu" in the header (first image), and as a notification banner across the top of the page. Notification banners for new badge levels will trigger a confetti animation, imported from https://github.com/catdad/canvas-confetti.

The initial commit for this PR has a noticeable bug, in that demotions and promotions will both count towards the Community Builder Badge. This issue stems from how assign_users_to_groups will always update translators then managers, in that order. Thus, there is no way to distinguish if an action is promoting a user from Contributor -> Translator or if an action is demoting a user from Manager -> Translator, especially since multiple users can be demoted/promoted in the same action.

image image

Harmit Goswami added 2 commits November 15, 2024 11:13
Init commit for badge notifications
8462f34 
In this state, there is still an issue with differentiating between demotions and promotions for the Community Builder Badge, and awarding badges accordingly
Merge main into branch
0abc604
@harmitgoswami harmitgoswami marked this pull request as draft November 15, 2024 19:41
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 40.86022% with 55 lines in your changes missing coverage. Please review.

Project coverage is 80.11%. Comparing base (9bd457f) to head (4ff5f1c).
Report is 11 commits behind head on main.

Additional details and impacted files

mathjazz
mathjazz reviewed Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Nice work!

As mentioned inline, I'd use a popup when the badge is awarded.

Looking into the Permission promotions bug.

pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/teams/static/js/permissions.js Outdated Show resolved Hide resolved
@mathjazz
Copy link
Collaborator

The initial commit for this PR has a noticeable bug, in that demotions and promotions will both count towards the Community Builder Badge. This issue stems from how assign_users_to_groups will always update translators then managers, in that order. Thus, there is no way to distinguish if an action is promoting a user from Contributor -> Translator or if an action is demoting a user from Manager -> Translator, especially since multiple users can be demoted/promoted in the same action.

Is this not working as expected then?

pontoon/pontoon/base/forms.py

Lines 125 to 136 in 8a16a65

# Check if user was demoted from Manager to Translator
# In this case, it doesn't count as a promotion
if group_name == "managers":
removal = PermissionChangelog.objects.filter(
performed_by=self.user,
action_type=PermissionChangelog.ActionType.REMOVED,
created_at__gte=now,
)
if removal:
for item in removal:
if "managers" in item.group.name:
after_count -= 1

Fixed confetti file
24a3519 
Changed the confetti file to be the unminified version instead. This commit also has a broken implementation of the permissions count bug, where I attempted to self-correct permissions counts inside of user.py
@mathjazz
Copy link
Collaborator

Is this not working as expected then?

This patch fixes the badges_promotion_count to properly exclude false positives and uses it when permissions get saved:
https://gist.github.com/mathjazz/bc5a9fa6359179fa1677c43b4d8f2080

mathjazz
mathjazz reviewed Nov 28, 2024
View reviewed changes
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Nice work! I didn't actually test the code yet, but left some notes on things that stood out.

It seems like the badge popup HTML and CSS are not the same in preference page and the translate view?

pontoon/teams/static/js/permissions.js Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/includes/permissions.html Outdated Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
translate/src/modules/notification/components/BadgeTooltip.css Outdated Show resolved Hide resolved
mathjazz
mathjazz reviewed Nov 29, 2024
View reviewed changes
pontoon/teams/static/js/permissions.js Outdated Show resolved Hide resolved
pontoon/teams/static/js/permissions.js Outdated Show resolved Hide resolved
pontoon/base/forms.py Outdated Show resolved Hide resolved
pontoon/settings/base.py
STATICFILES_DIRS = [

STATICFILES_DIRS = (
("badges", "pontoon/contributors/static/img"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I understand it, Django currently only serves static files from base/static. I believe adding this change allows the 3 badge svg files to also be served in our app.

The tuple is just to for better readability.

Is there a better way of doing this?

pontoon/teams/static/js/permissions.js Outdated Show resolved Hide resolved
pontoon/teams/templates/teams/includes/permissions.html Outdated Show resolved Hide resolved
pontoon/teams/views.py Outdated Show resolved Hide resolved
pontoon/base/forms.py Outdated Show resolved Hide resolved
pontoon/translations/views.py Outdated Show resolved Hide resolved
translate/public/translate.html Outdated Show resolved Hide resolved
@harmitgoswami harmitgoswami mentioned this pull request Nov 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathjazz mathjazz mathjazz left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send notification upon user receiving new badge
3 participants
@harmitgoswami @codecov-commenter @mathjazz