Project

General

Profile

Development #22390

Ajouter un unique_together = (('user', 'external_id'),) sur le modèle Notification

Added by Benjamin Dauvergne about 4 years ago. Updated over 3 years ago.

Status:
Fermé
Priority:
Normal
Target version:
-
Start date:
08 Mar 2018
Due date:
% Done:

0%

Estimated time:
Patch proposed:
No
Planning:

Description

Il me semble que c'est ce qu'on souhaite en fait en lisant le code Notification.notify() et ça simplifierait la lecture du code à base de .first() qu'on pourrait remplacer par un .get_or_create() classique, actuellement il reste une race condition possible.

(À noter que ça n'aura pas d'impact si external_id = NULL, deux valeurs nulles n'étant jamais égales dans Postgres).


Files

0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch (599 Bytes) 0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch Benjamin Dauvergne, 16 Mar 2018 12:43 PM
0002-tox.ini-always-set-usedevelop-True-22390.patch (717 Bytes) 0002-tox.ini-always-set-usedevelop-True-22390.patch Benjamin Dauvergne, 16 Mar 2018 12:43 PM
0001-tox.ini-remove-pytest-capturelog-22390.patch (496 Bytes) 0001-tox.ini-remove-pytest-capturelog-22390.patch Benjamin Dauvergne, 16 Mar 2018 12:43 PM
0004-notifications-improve-internal-API-22390.patch (19.5 KB) 0004-notifications-improve-internal-API-22390.patch Benjamin Dauvergne, 16 Mar 2018 12:43 PM
0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch (599 Bytes) 0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0005-notifications-add-origin-to-unique-index-to-be-rebas.patch (1.99 KB) 0005-notifications-add-origin-to-unique-index-to-be-rebas.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0002-tox.ini-always-set-usedevelop-True-22390.patch (717 Bytes) 0002-tox.ini-always-set-usedevelop-True-22390.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0001-tox.ini-remove-pytest-capturelog-22390.patch (496 Bytes) 0001-tox.ini-remove-pytest-capturelog-22390.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0007-notifications-enforce-namespacing-on-external-ids-an.patch (1.71 KB) 0007-notifications-enforce-namespacing-on-external-ids-an.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0006-notifications-remove-unused-import-to-be-rebased.patch (962 Bytes) 0006-notifications-remove-unused-import-to-be-rebased.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0004-notifications-improve-internal-API-22390.patch (19.5 KB) 0004-notifications-improve-internal-API-22390.patch Benjamin Dauvergne, 16 Mar 2018 03:24 PM
0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch (599 Bytes) 0003-tests-set-a-TEST-database-name-so-that-pytest-reuse-.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0002-tox.ini-always-set-usedevelop-True-22390.patch (717 Bytes) 0002-tox.ini-always-set-usedevelop-True-22390.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0001-tox.ini-remove-pytest-capturelog-22390.patch (496 Bytes) 0001-tox.ini-remove-pytest-capturelog-22390.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0005-notifications-remove-unused-import-to-be-rebased.patch (962 Bytes) 0005-notifications-remove-unused-import-to-be-rebased.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0007-notifications-do-not-return-created-from-.notify-to-.patch (5.45 KB) 0007-notifications-do-not-return-created-from-.notify-to-.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0008-notifications-add-a-listing-API.patch (13 KB) 0008-notifications-add-a-listing-API.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0006-notifications-enforce-namespacing-on-external-ids-to.patch (6.39 KB) 0006-notifications-enforce-namespacing-on-external-ids-to.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM
0004-notifications-improve-internal-API-22390.patch (19.7 KB) 0004-notifications-improve-internal-API-22390.patch Benjamin Dauvergne, 18 Mar 2018 10:13 PM

Related issues

Related to Combo - Development #13122: lingo: systeme de notification en cas de facture à payerFermé08 Sep 201615 Mar 2018

Actions

Associated revisions

Revision 2a675d6c (diff)
Added by Benjamin Dauvergne about 4 years ago

tox.ini: remove pytest-capturelog (#22390)

Revision 12927558 (diff)
Added by Benjamin Dauvergne about 4 years ago

tox.ini: always set usedevelop=True (#22390)

It currently does not work without it.

Revision 64c767c5 (diff)
Added by Benjamin Dauvergne about 4 years ago

tests: set a TEST database name so that pytest --reuse-db works (#22390)

Revision f2719b3a (diff)
Added by Benjamin Dauvergne about 4 years ago

notifications: improve internal API (#22390)

  • (user, external_id) is now unique, no need to user .first() / .last()
  • new filter .find(user, id), .visible(user), new queryset actions .ack() and .forget()
  • new property notification.visible to know if a notification is shown
  • when notify() is called on an existing notification, it's unacked
  • notify's parameters url, body and origin now default to '', serializer
    has been updated

History

#1

Updated by Benjamin Dauvergne about 4 years ago

  • Description updated (diff)
#2

Updated by Benjamin Dauvergne about 4 years ago

  • Tracker changed from Bug to Development
#4

Updated by Benjamin Dauvergne about 4 years ago

Je n'ai pas touché du tout aux APIs externe (à part le serializer pour changer les valeurs par défaut, None -> ''). Mais il y aurait certainement des choses à faire, typiquement Ack devrait prendre une liste d'id parce que typiquement en cliquant sur le badge on devrait acker toutes les notifications visibles et ne pas faire n appels.

À voir si le acked=False sur un renotify est vraiment le comportement souhaité, il me semble que si.

#5

Updated by Frédéric Péters about 4 years ago

(ceci n'est pas une relecture)

cliquant sur le badge

En l'espèce on n'a pas de clic sur le badge, les clics se font sur les notifications, individuelles.

#6

Updated by Frédéric Péters about 4 years ago

  • (user, external_id) is now unique, no need to user .first() / .last()

J'ai l'impression qu'on doit aussi inclure l'origin dans le tuple d'unicité, ou on convient de donner comme consigne aux appelants de namespacer l'external_id.

#7

Updated by Benjamin Dauvergne about 4 years ago

Frédéric Péters a écrit :

(ceci n'est pas une relecture)

cliquant sur le badge

En l'espèce on n'a pas de clic sur le badge, les clics se font sur les notifications, individuelles.

En discutant avec Thomas il sous-entendait que c'était en cliquant sur le badge qu'un encart contenant les notifications s'ouvrait et que certainement à ce moment les notifications étaient ackés mais je ne pense pas qu'il ait déjà vu une vrai intégration lui non plus.

Est-ce que tu peux me pointer vers une intégration existante appelant les WS ack et forget dans combo je ne vois rien ?

#8

Updated by Benjamin Dauvergne about 4 years ago

  • Assignee set to Benjamin Dauvergne

Frédéric Péters a écrit :

  • (user, external_id) is now unique, no need to user .first() / .last()

J'ai l'impression qu'on doit aussi inclure l'origin dans le tuple d'unicité, ou on convient de donner comme consigne aux appelants de namespacer l'external_id.

1. origin n'étant jamais nul ça me va, mais est-ce qu'on impose au niveau du WS (voir au niveau de l'API interne aussi) d'avoir forcément un origin si on a un id fourni ? Ça me paraîtrait bien mais je ne voudrai pas casser une utilisation existante.

2. est-ce qu'on imposerait d'avoir au moins un caractère non alpha-numérique dans un id fourni via le web-service ? pour éviter les collisions si jamais un client voulait envoyer un entier et que par hasard ça matchait une autre notification existante.

#9

Updated by Frédéric Péters about 4 years ago

Est-ce que tu peux me pointer vers une intégration existante appelant les WS ack et forget dans combo je ne vois rien ?

gnm, https://git.entrouvert.org/publik-base-theme.git/tree/templates/variants/grandlyon-gnm/combo/notificationscell.html &

    var ack_url = $(this).data('combo-notification-ack-url');
    $.ajax({
         url: ack_url,
         success: function(html) {
           $(cell).removeClass('new').addClass('acked');
...

dans le js.

#10

Updated by Frédéric Péters about 4 years ago

1. origin n'étant jamais nul ça me va, mais est-ce qu'on impose au niveau du WS (voir au niveau de l'API interne aussi) d'avoir forcément un origin si on a un id fourni ? Ça me paraîtrait bien mais je ne voudrai pas casser une utilisation existante.

Je ne pense pas que ça casse (à gnm ça serait utilisé pour avoir la commune traitant dans le champ origine mais ça n'est pas encore présent).

2. est-ce qu'on imposerait d'avoir au moins un caractère non alpha-numérique dans un id fourni via le web-service ? pour éviter les collisions si jamais un client voulait envoyer un entier et que par hasard ça matchait une autre notification existante.

Je pense qu'on pourrait imposer la forme namespace:whatever, [\w-]+:[\w]+ (voire même imposer plusieurs caractères pour le namespace ?). (et si on fait ça je pense on peut laisser origin hors des critères d'unicité, juste une info utile pour l'affichage).

#11

Updated by Benjamin Dauvergne about 4 years ago

Frédéric Péters a écrit :

1. origin n'étant jamais nul ça me va, mais est-ce qu'on impose au niveau du WS (voir au niveau de l'API interne aussi) d'avoir forcément un origin si on a un id fourni ? Ça me paraîtrait bien mais je ne voudrai pas casser une utilisation existante.

Je ne pense pas que ça casse (à gnm ça serait utilisé pour avoir la commune traitant dans le champ origine mais ça n'est pas encore présent).

Après réflexion si on rend origin+external_id unique alors il faut demander l'origine sur les appels à ack et forget.

2. est-ce qu'on imposerait d'avoir au moins un caractère non alpha-numérique dans un id fourni via le web-service ? pour éviter les collisions si jamais un client voulait envoyer un entier et que par hasard ça matchait une autre notification existante.

Je pense qu'on pourrait imposer la forme namespace:whatever, [\w-]+:[\w]+ (voire même imposer plusieurs caractères pour le namespace ?). (et si on fait ça je pense on peut laisser origin hors des critères d'unicité, juste une info utile pour l'affichage).

Ok.

#12

Updated by Serghei Mihai about 4 years ago

  • Related to Development #13122: lingo: systeme de notification en cas de facture à payer added
#13

Updated by Benjamin Dauvergne about 4 years ago

Si l'id est un entier est qu'une notification existe, on laisse passer.

Si l'id n'est pas un entier alors on exige la présence d'une origine et l'id
doit match une regexp qui impose un namespace.

Tout ceci est indépendant du fait qu'on demande l'unicité ou pas par rapport à
origin (enfin dans ce cas il faudrait quand même modifier find et les
update_or_create/get_or_create pour considérer l'origine, je laisse le
changement d'index dans la liste des patchs en attendant reconfirmation qu'on
s'en fout).

#14

Updated by Benjamin Dauvergne about 4 years ago

Dernière suggestion: je me demande si le retour created par notify() a vraiment un intérêt, ici il ne sert qu'une fois, pour la réponse au WS add, je ne sais pas trop ce que ça apporte.

#15

Updated by Benjamin Dauvergne about 4 years ago

Dans les derniers patchs seulement 5, 6 et 7 ont bougé.

#16

Updated by Frédéric Péters about 4 years ago

Dernière suggestion: je me demande si le retour created par notify() a vraiment un intérêt, ici il ne sert qu'une fois, pour la réponse au WS add, je ne sais pas trop ce que ça apporte.

Il est arrivé dans https://dev.entrouvert.org/issues/21189

Vraiment l'origine on s'en fout pour l'unicité, pour tout le reste aussi, juste une information d'affichage; et donc je n'ai pas du être clair, dans 0007, la validation, ça devrait pour moi juste être en cas d'id une vérif sur le format "<namespace>:<whatever>" (la partie regex manque du patch ?). (et si cette validation peut se monter pour se faire aussi dans l'API interne, c'est super).

(ça fait trop de patchs tu pousserais ça dans une branche ?)

#17

Updated by Benjamin Dauvergne about 4 years ago

Frédéric Péters a écrit :

Dernière suggestion: je me demande si le retour created par notify() a vraiment un intérêt, ici il ne sert qu'une fois, pour la réponse au WS add, je ne sais pas trop ce que ça apporte.

Il est arrivé dans https://dev.entrouvert.org/issues/21189

Clairement ça ne sert à rien je vais le virer, les notifications sont nouvelles si acked == False rien à voir avec la présence ou pas en base.

Vraiment l'origine on s'en fout pour l'unicité, pour tout le reste aussi, juste une information d'affichage; et donc je n'ai pas du être clair, dans 0007, la validation, ça devrait pour moi juste être en cas d'id une vérif sur le format "<namespace>:<whatever>" (la partie regex manque du patch ?). (et si cette validation peut se monter pour se faire aussi dans l'API interne, c'est super).

D'ac je virer l'index et la contrainte pour avoir origin, je vais voir pour l'API interne.

(ça fait trop de patchs tu pousserais ça dans une branche ?)

Oui.

#21

Updated by Frédéric Péters about 4 years ago

(vraiment utiliser la branche plutôt que poser un tas de patchs).

0008 est à discuter ailleurs il me semble.

Et pousse déjà 0001→0003, qui n'ont pas de rapport.

#22

Updated by Serghei Mihai about 4 years ago

Ack pour ma part pour les patchs 0001-0007.
Le 0008, comme dit Frédéric, peut-être traité dans un autre ticket.

#23

Updated by Benjamin Dauvergne about 4 years ago

  • Status changed from Nouveau to Résolu (à déployer)
commit f2719b3a68b4e7e4b5ba54870305ee8c60c058ab (HEAD -> master, origin/master, origin/HEAD)
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Mar 16 12:35:28 2018 +0100

    notifications: improve internal API (#22390)

    * (user, external_id) is now unique, no need to user .first() / .last()
    * new filter .find(user, id), .visible(user), new queryset actions .ack() and .forget()
    * new property notification.visible to know if a notification is shown
    * when notify() is called on an existing notification, it's unacked
    * notify's parameters url, body and origin now default to '', serializer
    has been updated

Je déplace le nouveau dans un nouveau ticket.

#24

Updated by Frédéric Péters over 3 years ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF