-
Notifications
You must be signed in to change notification settings - Fork 24
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
Notifications: Correction de l'envoi de certaines notifications #5343
Conversation
a162235
to
2b804fc
Compare
2b804fc
to
4ab98cb
Compare
if not self.user.is_active: | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il ne faudrait pas le mettre après le gros if
vérifiant les memberships pour forward aux admins ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh !
Indeed
4ab98cb
to
6b65553
Compare
6b65553
to
daa58e3
Compare
@@ -42,5 +42,7 @@ def send(self): | |||
logger.info("Send email copy to admin, admin_count=%d", len(admins)) | |||
for admin in admins: | |||
self.__class__(admin, self.structure, self.user, **self.context).send() | |||
if not self.user.is_active: | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Est-ce que cela n'aurait pas plus de sens d'être intégré au should_send
?
The sender may be an employer
daa58e3
to
d53b340
Compare
@@ -253,7 +253,7 @@ | |||
# --- | |||
# name: test_employer_create_update_notification_settings[view queries - disable all notifications] | |||
dict({ | |||
'num_queries': 19, | |||
'num_queries': 22, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ça me chagrine un peu toutes ces requêtes mais ça sera pour une autre fois 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai eu la même reaction. Mais vu que c'est pas une page très utilisée, je me suis dit que c'était pas super grave
🤔 Pourquoi ?
Certaines notifications ne prennent pas en compte la configuration par l'utilisateur, ou alors peuvent être envoyée à un utilisateur désactivé
🍰 Comment ?
🚨 À vérifier
🏝️ Comment tester
💻 Captures d'écran