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.

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

Add notifications when a badge is awarded #3442

Merged
merged 32 commits into from
Dec 11, 2024

Conversation

harmitgoswami
Copy link
Collaborator

@harmitgoswami harmitgoswami commented Nov 15, 2024

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.

TODO:

  • With the newly added messaging center, all templates and logic for handling notifications should be moved into pontoon.messaging (see this comment below). This includes moving _send_badge_notification as well. We should remember to file an issue for this before this PR is merged.
image image

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

codecov-commenter commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 44.64286% with 62 lines in your changes missing coverage. Please review.

Project coverage is 79.98%. Comparing base (6273ecb) to head (076e825).
Report is 7 commits behind head on main.

Additional details and impacted files

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?

# 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

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

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
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 Outdated Show resolved Hide resolved
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
pontoon/base/forms.py Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/translations/views.py Show resolved Hide resolved
pontoon/translations/views.py Outdated Show resolved Hide resolved
pontoon/translations/views.py Show resolved Hide resolved
translate/src/modules/user/components/UserNotification.tsx Outdated Show resolved Hide resolved
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.

Almost there!

pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/translations/views.py Outdated Show resolved Hide resolved
@harmitgoswami
Copy link
Collaborator Author

@mathjazz I've also changed the badge display order for the changes requested in documentation. Just curious, are you still planning to take a look at the CSS for the popup?

@mathjazz mathjazz marked this pull request as ready for review December 2, 2024 20:45
@mathjazz
Copy link
Collaborator

mathjazz commented Dec 2, 2024

@mathjazz I've also changed the badge display order for the changes requested in documentation. Just curious, are you still planning to take a look at the CSS for the popup?

Thanks! The code looks good now. I'll work on the CSS tomorrow.

pontoon/base/models/user.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
@mathjazz
Copy link
Collaborator

mathjazz commented Dec 5, 2024

Known issues:

1. Translation Champion badge and Review Master badge can be awarded at the same time

  • If I submit a translation to a string that already has a translation, the old translation gets rejected. That means I get 1 action logged against the Translation Champion badge (translation submitted) and 1 against the Review Master badge (translation rejected, i.e. reviewed). If I do that 5 times in a row, I can get both badges awarded at the same time (one needs 5 translation submissions or reviews to be awarded the Level 1 Translation Champion or Review Master badge).
  • Our UI currently only shows one badge popup and sends one notification (both for the Translation Champion badge). But on the Profile page I see both badges. We need to account for that scenario in the UI.
  • Or figure out a way to count reviews differently (e.g. as explicit actions only), but that would require more work.

pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/base/models/user.py Outdated Show resolved Hide resolved
pontoon/base/models/user.py Outdated Show resolved Hide resolved
@mathjazz
Copy link
Collaborator

mathjazz commented Dec 9, 2024

I noticed that confetti on the translate page appear behind the editor, editor menu and concordance search field.

pontoon/base/models/user.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
docs/admin/deployment.rst Outdated Show resolved Hide resolved
pontoon/translations/views.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
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.

Hmm, I feel like is_implicit_action might be a better field choice? Most of the ActionLog items will now have is_explicit_action set to True, and I think it's more natural if the exceptional (minority) cases are True. No strong opinions, leaving the decision to you.

pontoon/actionlog/models.py Outdated Show resolved Hide resolved
pontoon/actionlog/models.py Outdated Show resolved Hide resolved
pontoon/batch/actions.py Outdated Show resolved Hide resolved
pontoon/settings/base.py Outdated Show resolved Hide resolved
pontoon/sync/changeset.py Outdated Show resolved Hide resolved
pontoon/sync/changeset.py Outdated Show resolved Hide resolved
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! Please fix the two outstanding issues and then we can deploy to stage for another round of testing.

Relatedly, I think #3481 and #3482 need to be rephrased: their actions are already included in the badge counts, we just need to award the badge when they trigger it.

pontoon/actionlog/models.py Outdated Show resolved Hide resolved
pontoon/base/models/user.py Outdated Show resolved Hide resolved
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.

The code looks good and I also didn't spot any issues on stage. \o/

Thanks for keeping up with this roller coaster, Harmit! And nice job with the solution you've proposed for tracking explicit review actions only!

I believe we should now focus on #3481 and #3482, but I wouldn't block a release on these two issues.

@mathjazz mathjazz merged commit 1ea8f48 into mozilla:main Dec 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send notification upon user receiving new badge
3 participants