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

Notifications: Correction de l'envoi de certaines notifications #5343

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

tonial
Copy link
Contributor

@tonial tonial commented Jan 6, 2025

🤔 Pourquoi ?

Certaines notifications ne prennent pas en compte la configuration par l'utilisateur, ou alors peuvent être envoyée à un utilisateur désactivé

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial added the modifié label Jan 6, 2025
@tonial tonial self-assigned this Jan 6, 2025
@tonial tonial force-pushed the alaurent/notifications_various_fixes branch 2 times, most recently from a162235 to 2b804fc Compare January 11, 2025 20:18
@tonial tonial requested a review from xavfernandez January 11, 2025 20:19
@tonial tonial marked this pull request as ready for review January 11, 2025 20:19
@tonial tonial force-pushed the alaurent/notifications_various_fixes branch from 2b804fc to 4ab98cb Compare January 11, 2025 20:36
Comment on lines 25 to 26
if not self.user.is_active:
return
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh !
Indeed

@tonial tonial force-pushed the alaurent/notifications_various_fixes branch from 4ab98cb to 6b65553 Compare January 13, 2025 11:44
@tonial tonial force-pushed the alaurent/notifications_various_fixes branch from 6b65553 to daa58e3 Compare January 14, 2025 05:12
@@ -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
Copy link
Contributor

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 ?

@tonial tonial force-pushed the alaurent/notifications_various_fixes branch from daa58e3 to d53b340 Compare January 14, 2025 09:25
@tonial tonial requested a review from xavfernandez January 14, 2025 09:25
@@ -253,7 +253,7 @@
# ---
# name: test_employer_create_update_notification_settings[view queries - disable all notifications]
dict({
'num_queries': 19,
'num_queries': 22,
Copy link
Contributor

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 🙈

Copy link
Contributor Author

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

@tonial tonial added this pull request to the merge queue Jan 14, 2025
Merged via the queue into master with commit 8dbb1e9 Jan 14, 2025
9 checks passed
@tonial tonial deleted the alaurent/notifications_various_fixes branch January 14, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants