Skip to content

fix(notifications): deliver @mention notifications and match full usernames#15196

Merged
Maffooch merged 4 commits into
bugfixfrom
notification-user-mentions-fix
Jul 10, 2026
Merged

fix(notifications): deliver @mention notifications and match full usernames#15196
Maffooch merged 4 commits into
bugfixfrom
notification-user-mentions-fix

Conversation

@blakeaowens

@blakeaowens blakeaowens commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

Notifying a user with @username in a note has two bugs in
process_tag_notifications (dojo/notifications/helper.py):

  1. Notifications were never delivered. The function collects Dojo_User
    objects and passes them as recipients to create_notification. But
    NotificationManager._process_recipients resolves recipients with
    Notifications.objects.filter(user__username__in=recipients). username
    is a CharField, so each Dojo_User is coerced to str(user) =
    "First Last (username)", which never equals a stored username — zero
    recipients match, and no alert/email/Slack message is sent. This fix passes
    user.username strings, which is what the parameter expects (see the
    existing recipients=["admin"] cases in unittests/test_notifications.py).

  2. Usernames with . - @ + couldn't be fully mentioned. Django's
    username validators allow those characters, but the mention regex only
    captured \w+, so @jane.doe resolved to jane. The capture is broadened
    to [\w.@+-]+; the leading (?:\A|\s)@ anchor still prevents email
    addresses written in prose (e.g. contact a@b.com) from triggering a
    mention, and a trailing sentence period is stripped so thanks @jane.
    still resolves to jane.

Behavior change

  • @mention notifications now actually reach the mentioned user (previously a
    silent no-op).
  • @jane.doe-style (and SSO/email-style) usernames now resolve to the full
    username instead of a truncated prefix — which also avoids accidentally
    notifying a different user who happens to own that prefix.

Testing

  • unittests/test_notifications.py passes (its user_mentioned cases already
    use recipients=["admin"], matching the corrected caller).
  • Added a regression test that a note containing @<active-user> creates an
    alert for that user (previously none was created).

@blakeaowens blakeaowens added this to the 3.1.100 milestone Jul 9, 2026

@Maffooch Maffooch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add some regression tests for this to lock in behavior

@valentijnscholten valentijnscholten left a comment

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.

Does this superseed #14927?

@blakeaowens

blakeaowens commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Does this superseed #14927?

@valentijnscholten Yes, this supersedes #14927. Both fix the same root cause (#14923): process_tag_notifications() handed Dojo_User objects to create_notification(recipients=…), but _process_recipients() filters on user__username__in, so the objects matched nothing and no notification went out.

On top of that delivery fix, this PR also broadens the mention regex from @(\w+) to @([\w.@+-]+) (plus a trailing-dot strip) so usernames containing ., -, @, or + resolve to the full username instead of a truncated prefix.

@blakeaowens
blakeaowens requested a review from Maffooch July 9, 2026 07:52

@Maffooch Maffooch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the tests!

@Maffooch
Maffooch requested a review from Jino-T July 9, 2026 23:51
@Maffooch
Maffooch merged commit 4954365 into bugfix Jul 10, 2026
149 of 150 checks passed
@Maffooch
Maffooch deleted the notification-user-mentions-fix branch July 10, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants