Projet

Général

Profil

Development #13122

lingo: systeme de notification en cas de facture à payer

Ajouté par Thomas Noël il y a plus de 7 ans. Mis à jour il y a environ 5 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
08 septembre 2016
Echéance:
15 mars 2018
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:

Description

avoir une commande "notify_invoices" dans lingo, qui, pour chaque utilisateur :
  • récupère l'ensemble des factures qu'il doit payer (sur toutes les régies)
  • élimine de la liste les factures déjà notifiées il y a moins de x jours (x dépendant de la régie de la facture)
  • envoie un mail avec l'ensemble des factures à notifier, avec l'URL à code de facture chiffré, et en attachant les PDF si disponible
  • le texte du mail étant un template

note : dans un premier temps, uniquement sur les régies distantes, mais voir si c'est généralisable.


Fichiers


Demandes liées

Lié à Combo - Bug #13124: lingo: moins dépendre d'une requestFermé08 septembre 2016

Actions
Lié à Combo - Development #19841: avoir un champ pour définir le délai, en jours, de relance de paiement d'une factureRejeté31 octobre 2017

Actions
Lié à Passerelle - Development #19842: family: fournir un endpoint de récuperation des NameIDs liés aux familles et qui ont des factures à payerFermé31 octobre 2017

Actions
Lié à Intégrations graphiques Publik - Development #20094: ajouter templates pour mails les notification des nouvelles facturesFermé15 novembre 2017

Actions
Lié à Combo - Development #21189: notifications: modifier l'API pour préciser si une nouvelle notification est crééeFermé15 janvier 2018

Actions
Lié à Passerelle - Development #21390: agoraplus et teamnet axel: rajouter le endpoint "users/with-pending-invoices"Rejeté24 janvier 2018

Actions
Lié à Combo - Development #22420: notifications: envoi des notifications par mailRejeté09 mars 201816 mai 2018

Actions
Lié à Combo - Development #22390: Ajouter un unique_together = (('user', 'external_id'),) sur le modèle NotificationFermé08 mars 2018

Actions

Révisions associées

Révision 8d9ac05b (diff)
Ajouté par Benjamin Dauvergne il y a presque 6 ans

notifications: add a namespace() query filter (#13122)

Révision ad3a44d6 (diff)
Ajouté par Benjamin Dauvergne il y a presque 6 ans

notifications: do not unack automatically on .notify() (#13122)

It's also supported by the notification web-service.

Révision 37c1985c (diff)
Ajouté par Benjamin Dauvergne il y a presque 6 ans

notifications: allow dash in notification_id right part (#13122)

Révision 83d2de70 (diff)
Ajouté par Benjamin Dauvergne il y a presque 6 ans

notifications: make user optional in .visible() filter (#13122)

Révision 26beade1 (diff)
Ajouté par Serghei Mihai il y a presque 6 ans

lingo: notify new remote invoices (#13122)

Révision 1933eb6a (diff)
Ajouté par Serghei Mihai il y a presque 6 ans

lingo: send emails when notifying new invoices (#13122)

Historique

#1

Mis à jour par Thomas Noël il y a plus de 7 ans

  • Lié à Bug #13124: lingo: moins dépendre d'une request ajouté
#2

Mis à jour par Frédéric Péters il y a environ 7 ans

  • Projet changé de Combo à Lingo
#4

Mis à jour par Serghei Mihai il y a plus de 6 ans

Je vois d'abord ici un webservice dans passerelle exposant les NameID liés aux familles, pour ne pas intérroger passerelle autant de fois que d'utilisateurs provisionnés dans Combo.
Ensuite récuperation des factures à payer pour ces NameID.

#5

Mis à jour par Serghei Mihai il y a plus de 6 ans

  • Lié à Development #19841: avoir un champ pour définir le délai, en jours, de relance de paiement d'une facture ajouté
#6

Mis à jour par Serghei Mihai il y a plus de 6 ans

  • Lié à Development #19842: family: fournir un endpoint de récuperation des NameIDs liés aux familles et qui ont des factures à payer ajouté
#7

Mis à jour par Thomas Noël il y a plus de 6 ans

Autre plan pour éviter trop de requêtes :
  • avoir un webservice dans lingo qui reçoit, pour un utilisateur, la liste des ses factures à payer. Lingo notifie alors, ou pas, en fonction de la configuration de la régie (délais de relance, texte de la notif) et des notifications déjà déclenchées sur l'usager concerné. Un mail est envoyé pour chaque facture notifiée, avec un petit texte, configuré dans la régie, et le doc attaché (PDF ou autre) s'il était présent.
  • c'est donc le connecteur passerelle qui aura la charge de parcourir la liste des utilisateurs qui ont des factures à payer, et appelle le webservice lingo ci-dessus pour chaque utilisateur.
#8

Mis à jour par Thomas Noël il y a plus de 6 ans

Retour sur le plan d'origine, avec l'optimisation proposée par Serghei, qui serait d'avoir d'abord un webservice qui renvoie la liste des NameID à interroger. Donc dans le connecteur de la regie dans passerelle, avoir un endpoint qui réponde à la question « la liste des comptes qui valent la peine d'être interrogés » (et qui fait au mieux, sachant qu'il doit répondre en 10 ou 20 secondes max).

Ensuite, on reprend pour chaque utilisateur de cette liste, le plan indiqué dans la description de ce ticket.

#9

Mis à jour par Serghei Mihai il y a plus de 6 ans

Un premier brouillon avec une méthode de la régie en charge des notifications, à appeler dans une commande.

#11

Mis à jour par Frédéric Péters il y a plus de 6 ans

  • Statut changé de Nouveau à En cours
  • Assigné à changé de Thomas Noël à Serghei Mihai

invoice_email_notifications_delay = models.IntegerField(help_text=_('In days'))

Manque très clairement un verbose_name; c'est tellement basique que je ne vais pas plus loin.

#12

Mis à jour par Frédéric Péters il y a plus de 6 ans

      A new invoice nr. {{ id }} is available.

      To view it please go to <a href="{{ portal_url }}">portal</a> or <a href="{{ payment_url }}">pay it directly</a>.

Je suggérerais aussi de voir dès maintenant pour un message qui soit ok pour Orléans; par exemple à regarder l'existant, je note qu'on mentionne le montant à payer dans le message. (et de manière générale, le message brut de fonderie ainsi, il crie juste "ça sera corrigé dans publik-base-theme").

~~

                     base_url = getattr(settings, 'SITE_BASE_URL', '')
                     payment_url = reverse('view-item', kwargs={'regie_id': self.id,
                                        'item_crypto_id': remote_item.crypto_id})
                    ctx = {'payment_url': urlparse.urljoin(base_url, payment_url),

Nope; build_absolute_uri.

            if items['invoices']:
                for item in items['invoices']:

Useless use of if.

#13

Mis à jour par Serghei Mihai il y a plus de 6 ans

  • Lié à Development #20094: ajouter templates pour mails les notification des nouvelles factures ajouté
#14

Mis à jour par Frédéric Péters il y a plus de 6 ans

il crie juste "ça sera corrigé dans publik-base-theme".

Peut-être être explicite ici ? → Le message de base dans Combo doit déjà être correct; publik-base-theme peut améliorer, ajouter des spécifités, mais ce n'est pas un module pour corriger les choses mal foutues ailleurs.

#15

Mis à jour par Serghei Mihai il y a plus de 6 ans

C'était tout à fait mon idée. publik-base-theme fera plutôt de la mise en page et non correction des textes.

#16

Mis à jour par Serghei Mihai il y a plus de 6 ans

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

Nope; build_absolute_uri.

Il me manque request pour ça et étant donné que cela sera executé dans par commande django dans un cron je ne peux pas l'avoir.

#17

Mis à jour par Serghei Mihai il y a plus de 6 ans

J'imagine plutôt utiliser portal_url disponible dans les TEMPLATE_VARS de chaque tenant.
Au passage l'objet RemoteItem prend en compte la possibilité de non paiement en ligne si le montant de la facture est inférieur à celui autorisé par la régie.

#18

Mis à jour par Serghei Mihai il y a environ 6 ans

Après discussion avec Thomas il me semble intéressant d'utiliser les notifications pour enregistrer l'arrivée des nouvelles factures.
Elles serviront pour les relancer par mail à l'usager au lieu d'un modèle dédié.

Patch testé en local.

#19

Mis à jour par Frédéric Péters il y a environ 6 ans

Patch testé en local.

Mais environnement local totalement à côté de la plaque.

Exactement la même affaire ici qu'avec corbo :

response = requests.get(url, remote_service=None, cache_duration=0)

Ça dit "non non non ne signe pas l'appel".

#20

Mis à jour par Frédéric Péters il y a environ 6 ans

                notification = Notification.objects.filter(models.Q(end_timestamp__lt=now) | models.Q(acked=False),
                                            external_id=invoice, user=user, summary=invoice['label']).last()

Ça m'a l'air bien moche d'aller si profond dans un modèle extérieur. Ça veut sans doute dire qu'il manque une API dans combo.apps.notifications; c'est ça qu'il faut arranger.

Et la ligne en question, de toute façon trop compliquée à comprendre, m'a l'air incorrecte, me donne l'impression qu'une personne qui a acké la notif recevrait quand même un email. (je me trompe peut-être sur le end_timestamp__lt, justement le genre de question que je ne veux pas avoir).

#21

Mis à jour par Serghei Mihai il y a environ 6 ans

  • Lié à Development #21189: notifications: modifier l'API pour préciser si une nouvelle notification est créée ajouté
#22

Mis à jour par Serghei Mihai il y a environ 6 ans

Avant de proposer d'autres patchs, voici mon idée.

Lors de la récupération des factures à partir de l'endpoint /users/with-pending-invoices/ dans le connecteur famille:

  • chercher le user correspondant au nameid ** si NameID ne correspondant à aucun User, passer au NameID suivant ** si User trouvé, pour chaque facture: *** créer une notification associé à User et à un id de la facture (régie et numéro combinés) avec une date d'expiration paramètrable dans la régie *** envoyer un mail avec la facture jointe ** lors d'une autre itération *** vérifier si la notification précedemment créée n'est pas ackée (puisque la facture a été payée entre temps) et si oui, ne plus envoyer de mail *** vérifier si la notification est expirée, envoyer de nouveau un mail avec la facture en pièce jointe, et créer une nouvelle notification. Si la notification n'est pas éxpirée, passer.

Cela permet plus tard (dans un autre ticket) de ack-er une notification quand la facture sera payée (callback lingo).

#23

Mis à jour par Frédéric Péters il y a environ 6 ans

vérifier si la notification précedemment créée n'est pas ackée (puisque la facture a été payée entre temps) et si oui, ne plus envoyer de mail
vérifier si la notification est expirée, envoyer de nouveau un mail avec la facture en pièce jointe, et créer une nouvelle notification. Si la notification n'est pas expirée, passer.

Le ack d'une notification c'est une action de l'usager, ta parenthèse "puisque la facture a été payée entre temps" m'évoque un temps différent, où il serait constaté qu'une facture a été payée et où l'action "forget" serait appelée sur la notification.

Je pense qu'il y a embrouille et tentative de détourner le sens des champs existants.

À mon sens, le déroulé pourrait être :

  • pour une facture
    • si elle est payée, forget(id='facture-numero-' +xxx)
    • si elle n'est pas payée, notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=facture.date limite de paiement)

Une fois qu'on a ça, quel déroulé pour avoir en plus un rappel ?

  • pour une facture,
    • si elle est payée, forget(id='facture-numero-' + xxx); forget(id='rappel-facture-numero-' + xxx)
    • si elle n'est pas payée,
      • obtenir la notification id='facture-numero-' + xxx → c'est une API qu'on n'a pas, la récupération des infos sur une notification existante.
      • si la notif n'existe pas, notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=now + deux semaines, genre)
      • si la notif existe et qu'elle est dépassée, notify(summary='rappel: faut payer', id='rappel-numero-' + xxx, end_timestamp=facture.date limite de paiement)

Alternativement, le rappel pourrait plutôt se faire sur base la date limite de paiement.

  • pour une facture,
    • si elle est payée, forget(id='facture-numero-' + xxx); forget(id='rappel-facture-numero-' + xxx)
    • si elle n'est pas payée,
      • notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=facture.date limite de paiement)
    • si la date limite est dans moins d'une semaine, notify(summary='rappel: faut payer', id='rappel-numero-' + xxx, end_timestamp=facture.date limite de paiement)

Et il n'y a pas de nouvelle API ici, mais besoin de modifier le retour de l'appel "notify" pour préciser si la notification a été créée ou si elle existait déjà.

#24

Mis à jour par Serghei Mihai il y a environ 6 ans

  • Lié à Development #21390: agoraplus et teamnet axel: rajouter le endpoint "users/with-pending-invoices" ajouté
#25

Mis à jour par Brice Mallet il y a environ 6 ans

  • Echéance mis à 15 mars 2018
#26

Mis à jour par Serghei Mihai il y a environ 6 ans

Je suis d'accord avec le déroulé que tu proposes, avec quelques suggestions.

  • pour une facture
    • si elle est payée, forget(id='facture-numero-' +xxx)

On ne sait pas si elle a été payée. Le endpoint '/users/with-pending-invoices/ de passerelle retourne seulement les factures à payer. Donc on laissera la notification expirer suite à:

  • notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=now + un certain nombre de jours)

Ensuite pour le rappel de paiement:

  • obtenir la notification id='facture-numero-' + xxx
  • si la notif n'existe pas, notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=now + un certain nombre de jours)
  • si la notif existe et qu'elle est dépassée
  • si facture.date limite de paiement <= now + un certain nombre de jours , alors notify(summary='rappel: faut payer', id='rappel-numero-' + xxx, end_timestamp=facture.date limite de paiement), sinon end_timestamp=now + un certain nombre de jours

Le certain nombre de jours serait un setting.

#27

Mis à jour par Serghei Mihai il y a environ 6 ans

Je me rend compte en lisant le code de Notification.notify qu'il renouvelle une notification lors des appels repetés et donc une notification initialement créée, n'expire pas.
Je suis pour ajouter un paramètre dans l'appel à notify pour lui signaler de ne pas renouveller une notification: pas mettre à jour la date de début et de fin.

#28

Mis à jour par Frédéric Péters il y a environ 6 ans

Je me rend compte en lisant le code de Notification.notify qu'il renouvelle une notification lors des appels repetés et donc une notification initialement créée, n'expire pas.
Je suis pour ajouter un paramètre dans l'appel à notify pour lui signaler de ne pas renouveller une notification: pas mettre à jour la date de début et de fin.

Je relis et tu écris dans le commentaire précédent ton plan avec notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=now + un certain nombre de jours).

C'est toi qui décide dans cet appel de l'expiration, j'ai l'impression que tu te crées ainsi un problème que tu essaies ensuite de contourner en complexifiant encore la chose. Encore du mal à capter pourquoi tout ça.

#29

Mis à jour par Serghei Mihai il y a environ 6 ans

Je m'explique.

J'imagine la détéction des nouvelles facture sous forme d'une tâche cron, lancée une fois par jour.

Lors de l'arrivée d'une nouvelle facture il y a donc une notification. Cette notification aura une durée de vie de n jours.
Lors d'un nouvel appel à notify, la notification existe déjà. Elle n'est pas encore expirée et je ne souhaite pas la renouveller.

Quand elle aurait expiré, je veux créer une nouvelle notification du genre "reminder: invoice X to pay".

#30

Mis à jour par Frédéric Péters il y a environ 6 ans

Lors de l'arrivée d'une nouvelle facture il y a donc une notification. Cette notification aura une durée de vie de n jours.
Lors d'un nouvel appel à notify, la notification existe déjà. Elle n'est pas encore expirée et je ne souhaite pas la renouveller.

Mais tu crées toi-même ce problème en faisant "moment de l'appel + n jours", quel problème à définir de manière fixe le moment de l'expiration ? (que ça soit sur base de la date de limite de paiement comme je le notais dans un commentaire précédent, ou sur base de la date d'émission, ou autre).

#31

Mis à jour par Serghei Mihai il y a environ 6 ans

Mais tu as raison, suis je bête.
La date d'expiration serait la date d'émission + n jours.

#32

Mis à jour par Serghei Mihai il y a environ 6 ans

Avant de me lancer dans l'envoi des mails, voici un patch créant les notifications pour les nouvelles factures.

#33

Mis à jour par Frédéric Péters il y a environ 6 ans

Il y manque une commande de management, non ?

#34

Mis à jour par Serghei Mihai il y a environ 6 ans

Si. Un simple appel à la méthode pour toutes les régies distantes.

#36

Mis à jour par Frédéric Péters il y a environ 6 ans

De manière amusante tu wrappes les lignes dans le message HTML où une longue ligne n'aurait pas d'incidence mais laisse de longues lignes dans la version text/plain, où il serait par contre plus conforme aux usages de wrapper.

#37

Mis à jour par Frédéric Péters il y a environ 6 ans

Sur le fond, j'imaginais vu l'utilisation de combo.apps.notifications qu'on gagne la possibilité de noter que certaines notifs doivent également être accompagnées d'un email, et ainsi on aurait pu avancer dans un cadre général, à un moment proposer à l'usager de contrôler comment il recevait ces notifications, lesquelles il voulait recevoir, etc.

Plutôt que gagner un truc spécifique pour les factures, qui me semble déjà une impasse.

#38

Mis à jour par Serghei Mihai il y a environ 6 ans

J'avais pensé à un paramètre dans notify qui déclencherai l'envoi d'un mail.
Et dans ce cas passer le sujet, le corps du message (il y a summary et body pour ça) mais aussi une pièce jointe.

Mais je ne connais pas trop l'usage actuel des notifications et l'impacte que ça peut avoir.
Je préfère donc partir sur ce cas spécifique des factures, et réflechir plus tard à comment intégrer l'envoi des mails dans les notifications.

#39

Mis à jour par Frédéric Péters il y a environ 6 ans

réflechir plus tard

Tu t'engages avec un ticket et une échéance ?

#40

Mis à jour par Serghei Mihai il y a environ 6 ans

Oui, mais pas avant début mai, genre.

#41

Mis à jour par Frédéric Péters il y a environ 6 ans

Genre tu créeras le ticket début mai ?

#42

Mis à jour par Serghei Mihai il y a environ 6 ans

Maintenant: #22420

#43

Mis à jour par Frédéric Péters il y a environ 6 ans

#44

Mis à jour par Frédéric Péters il y a environ 6 ans

Mais je ne connais pas trop l'usage actuel des notifications et l'impacte que ça peut avoir.

(mais je peux quand même dire dès maintenant, qu'un paramètre genre "mail", dont la valeur par défaut serait False, n'aurait pas d'impact).

#45

Mis à jour par Serghei Mihai il y a environ 6 ans

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

De manière amusante tu wrappes les lignes dans le message HTML où une longue ligne n'aurait pas d'incidence mais laisse de longues lignes dans la version text/plain, où il serait par contre plus conforme aux usages de wrapper.

Si je comprend bien, il s'agit de lignes trop longues dans la version text ? Ok, je rajoute des sauts de ligne à environ 80 caractères.

#46

Mis à jour par Frédéric Péters il y a environ 6 ans

Ces variables : site_title & portal_url n'existent pas sans hobo.

@pytest.fixture
def user():

plutôt l'appeler admin, pour éviter les confusions futures.

(avec une succession de deux patchs ça commence à devenir plus facile de suivre via une branche)

remind_id = 'remind-%s' % invoice_id

s/remind/reminder/

Mais revenons autour de ça :

                if invoice_pay_limit_date < notification_end_timestamp:
                    notification_end_timestamp = invoice_pay_limit_date

J'ai un peu du mal à capter les logiques sur les temps de notification / relance.

  • la facture est créée → notification
  • dix jours après, alors qu'elle arriverait à expiration, si la facture est toujours retournée par l'endpoint (= pas encore payée), une notification de rappel est créée
    • cette seconde notification, elle est notée pour expirer au même moment que la première
    • elle n'apparaitra donc jamais (?!)

Et si jamais elle apparait avec moins de dix jours par rapport à sa date limite de paiement, c'est la même chose mais la notif disparait à la date limite de paiement.

Ou je lis mal ?

#47

Mis à jour par Frédéric Péters il y a environ 6 ans

Et je reviens plus haut encore, mon commentaire :

pour une facture,
  • si elle est payée, forget(id='facture-numero-' + xxx); forget(id='rappel-facture-numero-' + xxx)

Et ta réponse :

On ne sait pas si elle a été payée. Le endpoint '/users/with-pending-invoices/ de passerelle retourne seulement les factures à payer. Donc on laissera la notification expirer suite à:

Mais l'endpoint retourne les factures qui n'ont pas expiré, on peut donc considérer comme à oublier (faire le .forget(...)) les notifications sur d'autres factures.

#48

Mis à jour par Serghei Mihai il y a environ 6 ans

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

Mais revenons autour de ça :

  • la facture est créée → notification
  • dix jours après, alors qu'elle arriverait à expiration, si la facture est toujours retournée par l'endpoint (= pas encore payée), une notification de rappel est créée
    • cette seconde notification, elle est notée pour expirer au même moment que la première
    • elle n'apparaitra donc jamais (?!)

Ici tu as raison. Si la facture est déjà expirée mais toujours retournée par le webservice, il ne faut pas créer de notification.

A mon sens, une facture qui est expirée ne doit pas apparaître dans les factures à payer. Généralement elle passe en perception, marquée comme payée, et le mode de paiement change (le trésor public s'en charge, ou autre).

Et si jamais elle apparait avec moins de dix jours par rapport à sa date limite de paiement, c'est la même chose mais la notif disparait à la date limite de paiement.

Oui, une seule notification serait envoyée pour la facture, car après elle passe en perception.

Je pose mes patchs dans la branche http://git.entrouvert.org/combo.git/?h=wip/notifications-13122

#49

Mis à jour par Frédéric Péters il y a environ 6 ans

Ici tu as raison. Si la facture est déjà expirée mais toujours retournée par le webservice, il ne faut pas créer de notification.

Sans doute pas clair, "alors qu'elle arriverait à expiration" je parlais de la notification, pas de la facture.

Je relis à nouveau, et déjà c'était pas clair et j'avais eu du mal à comprendre l'intention, c'est toujours pareil.

Je reprends en lisant le code,

  • la facture est créée → notification
  • le lendemain,
  • if not created → on entre dans ce bloc
  • if notification.end_timestamp < timezone.now(): la notification n'est pas expirée, à demain. (en fait c'est cette ligne qui gagnerait vraiment à avoir un commentaire "# notification has expired", je pense)
  • ...
  • n jours plus tard
  • if not created → on entre dans ce bloc
  • if notification.end_timestamp < timezone.now():
    • → la notification est expirée
    • Notification.notify(... end_timestamp=notification_end_timestamp) → notification de rappel. Mais cette notification de rappel, elle est créée avec un end_timestamp dans le passé, ne sera jamais affichée.
#50

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

On peut s'amuser.

start
if (facture en cours?) then (oui)
  :créer première notification;
  if (notification existe déjà et expiré?) then
     :créer rappel;
  endif
else (non)
  fork
  if (notification existe) then (oui)
     :expirer/supprimer la notification;
  endif
  fork again
  if (rappel existe) then (oui)
     :expirer/supprimer la notification;
  endif
  endfork
endif
stop

#51

Mis à jour par Frédéric Péters il y a environ 6 ans

s/créer rappel/créer rappel mais ne pas dire qu'il est déjà lui-même expiré parce que sinon ça va pas servir à grand chose/

#52

Mis à jour par Serghei Mihai il y a environ 6 ans

Ok. Pigé, par "facture en cours" tu sous-entends une facture qui est retournée par le webservice et qui n'est pas expirée.
Je revois ma logique.

#53

Mis à jour par Frédéric Péters il y a environ 6 ans

par "facture en cours" tu sous-entends (...)

Comme je n'ai jamais écrit "facture en cours", il y a peut-être encore des trucs pas clair.

#54

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

Serghei Mihai a écrit :

Ok. Pigé, par "facture en cours" tu sous-entends une facture qui est retournée par le webservice et qui n'est pas expirée.
Je revois ma logique.

Facture en cours pour moi ça veut dire qu'on voit toutes les factures (expirés ou pas, payés ou pas) et que pour chacune on peut dire non-expiré et non-payé = "en cours". Si on sort de ce cadre (genre les factures disparaissent du listing avant qu'on ait pu savoir qu'elles étaient expirées ou payées) alors il faut revoir la logique.

#55

Mis à jour par Frédéric Péters il y a environ 6 ans

À nouveau je ne pense pas avoir écrit "facture en cours", pour moi ce ticket ne parle pas des factures mais des notifications (invitant l'usager à payer). Et il faut faire gaffe à ne pas vouloir confondre les cycles de vie.

#56

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

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

À nouveau je ne pense pas avoir écrit "facture en cours", pour moi ce ticket ne parle pas des factures mais des notifications (invitant l'usager à payer). Et il faut faire gaffe à ne pas vouloir confondre les cycles de vie.

Je te renvoie au titre du ticket...

#57

Mis à jour par Frédéric Péters il y a environ 6 ans

I stand my ground. On parle des notifications. La page qui affiche les factures (en cours, ou pas, don't care) n'est en rien concernée.

#58

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

Ok bon je fais juste remarquer que si on a que la liste des factures payables maintenant on va avoir des notifications affichées pour des factures déjà payées et ou expirées, comme je ne sais pas ce que retourne exactement /users/with-pending-invoices/ et du code que je lis même si on a toutes les factures connues pour la personne de toute façon le code ne gère pas le cas que je décris.

#59

Mis à jour par Serghei Mihai il y a environ 6 ans

Ok.
J'ai poussé dans la branche les 2 patchs avec les modifs pour suivre le schéma.

#60

Mis à jour par Frédéric Péters il y a environ 6 ans

+ # invoice is not expired

Désolé mais moi je continue à vouloir parler des notifications.

Et le patch continue à créer une notif de rappel expirée.

#61

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

Serghei, tu crées tes deux notifications avec la même date de fin, si la première a déjà expirée forcément que la seconde aussi, en fait je me demande si c'est vraiment une histoire d'expiration qui nous intéresse et pas une histoire de lu/pas-lu. On devrait plutôt jouer avec le champs "acked" si la personne n'a rien fait depuis son dernier ack (mais comme on a pas la date du ack difficile de raisonner là dessus, ça aurait pu vraisemblement être un timestamp nullable plutôt qu'un booléen).

#62

Mis à jour par Serghei Mihai il y a environ 6 ans

Ok, branche à jour avec la notification de rappel avec une durée de vie à partir de la date d'expiration de la précedente + n jours.

#63

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

1. Il faudrait simplifier le code qui est bien trop profond, en 3 méthodes disons:
  • une méthode récupération des factures avec parsing des dates, make_aware, etc.. consommation du web-service et conversion en données Python.
  • une méthode qui boucle sur les utilisateurs et les factures récupérés au point précédent
  • une méthode qui envoie les notifications

2. Le warning Invoices available for unknown user: %s je ne sais pas si c'est très intéressant vu qu'on a pas de deprovisionning il me semble dans les connecteurs.

3. Les .last() et .first() je trouve ça assez moche je trouverai bien que le modèle de donnée soit affiné (voir #22390) qu'on puisse faire du .get_or_create() proprement.

4. Les commentaires devraient être tournés en terme d'expérience utilisateur comme par exemple

# remind user to pay this invoice

# last reminder

plutôt que de parler de notifications qui devraient être créées, et décrire un peu plus le cycle de vie:

<--created-------------------------Invoice---------------------------|Paid-------------pay_limit_date->
<--Notification------|Ack----created+delta-><-Reminder Notification-----|Ack---created+2*delta-->

en regardant ça je me dit qu'on se fout du delta en fait autant mettre pay_limite_date en end_timestamp, et créer le reminder plutôt delta jours avant pay_limite_date si la première notification n'est plus visible.

5. Notification.notify() fait un .first() ici on fait un .last(), notify devrait just renvoyer la notification qu'il a trouvé ou créé (ça rejoint le point 3)

6. notification.forget(..), reminder_notification.forget(...), forget est une méthode classe et on est certain que public_id() == invoice_id par définition, donc autant faire directement:

Notification.forget(user, invoice_id)
Notification.forget(user, reminder_id)

sans faire tous ces if (c'est aussi coûteux de faire un select suivi éventuellement d'un update que de faire un update vide). Maintenant je préfèrerai même un .delete() je ne sais pas trop pourquoi on garde de vieilles notifications comme cela, un truc comme ça.
# après la boucle sur les factures d'un utilisateur
# faire en sorte que reminder_id commence aussi par invoice-
external_ids = [ list of reminder_id or invoice_id alive ]
Notification.objects.filter(user=user, external_id__startswith='invoice-%s-' % self.slug).exclude(external_id__in=external_ids).delete()

et ça prendra en compte les factures qui on été payées.
7. Mettre pay_limit_date et le montant dans le texte de la notification "Payer facture %s de %s € avant le %s", puis "Rappel: payer la facture %s de %s € avant le %s" ou alors un intitulé si on a, ou rendre ça configurable un jour.

#64

Mis à jour par Frédéric Péters il y a environ 6 ans

Benjamin, aujourd'hui :

en regardant ça je me dit qu'on se fout du delta en fait autant mettre pay_limite_date en end_timestamp, et créer le reminder plutôt delta jours avant pay_limite_date si la première notification n'est plus visible.

Moi, il y a deux mois :

si elle n'est pas payée, notify(summary='faut payer', id='facture-numero-' + xxx, end_timestamp=facture.date limite de paiement)
(...)
si la date limite est dans moins d'une semaine, notify(summary='rappel: faut payer', id='rappel-numero-' + xxx, end_timestamp=facture.date limite de paiement)

Bref, je rends la main, on se revoit cet été.

#65

Mis à jour par Serghei Mihai il y a environ 6 ans

Remarques de Benjamin prises en compte sauf:

  • suppression des notifications pour les factures payées. Je préfère les garder pour l'instant comme cela doit se faire par ailleurs et qu'on décide dans un autre ticket de la durée de vie des notifications. En plus vu qu'on va créer 2 notifications: une au moment de la "découverte" d'une nouvelle facture et l'autre au moment de la relance, on ne risque pas d'être innondé par des nombreuses lignes dans la base.
  • je ne trouve pas très utiles d'inclure le montant et la date limite de paiement dans l'intitulé de la notification. En voyant la notification l'usager se rend sur la page des factures et voit son montant et la date limite de paiement.

J'ai mis à jour la branche.

#66

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

Serghei Mihai a écrit :

Remarques de Benjamin prises en compte sauf:

  • suppression des notifications pour les factures payées. Je préfère les garder pour l'instant comme cela doit se faire par ailleurs et qu'on décide dans un autre ticket de la durée de vie des notifications. En plus vu qu'on va créer 2 notifications: une au moment de la "découverte" d'une nouvelle facture et l'autre au moment de la relance, on ne risque pas d'être innondé par des nombreuses lignes dans la base.

Ok, on verra à l'usage.

  • je ne trouve pas très utiles d'inclure le montant et la date limite de paiement dans l'intitulé de la notification. En voyant la notification l'usager se rend sur la page des factures et voit son montant et la date limite de paiement.

Ok à défaut ce serait quand même pratique d'avoir un lien vers la page des factures ou pour payer la facture indiquée sur la notification (en utilisant le paramètre url de notify).

#67

Mis à jour par Serghei Mihai il y a environ 6 ans

Benjamin Dauvergne a écrit :

Ok à défaut ce serait quand même pratique d'avoir un lien vers la page des factures ou pour payer la facture indiquée sur la notification (en utilisant le paramètre url de notify).

Yep, je rajoute dans url l'url de la page avec les factures à payer, si elle existe.
Branche à jour.

#68

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

Et de mon coté je veux absolument que le #22390 soit commité avant de merger ça et que le code soit adapté pour utiliser les nouvelles API.

#69

Mis à jour par Serghei Mihai il y a environ 6 ans

Ok, je m'y colle.

#70

Mis à jour par Benjamin Dauvergne il y a environ 6 ans

En attendant,

Je changerai ça:

+            if not created and pay_limit_date <= now + remind_delta:

en ça:
+            if not created and (pay_limit_date <= now + remind_delta or not notification.acked):

ou plutôt
+            if not notification.visible():

et là je retombe sur sur ce que je dis de #22390, Notification.notify() devrait retourner un objet par un id.

Il y a un import d'override_settings et settings qui ne sont pas utilisés dans les tests.

#71

Mis à jour par Serghei Mihai il y a environ 6 ans

  • Lié à Development #22390: Ajouter un unique_together = (('user', 'external_id'),) sur le modèle Notification ajouté
#72

Mis à jour par Serghei Mihai il y a presque 6 ans

J'ai rebasé sur le master et poussé la branche à jour.

#74

Mis à jour par Benjamin Dauvergne il y a presque 6 ans

Un question qu'on se pose jamais: pay_limit_date c'est le dernier jour où on peut payer ou c'est la date à partir de laquelle on ne peut plus payer ? Dans le doute je mettrai pay_limit_date >= now puisque de te toute façon ça ne part pas à la trésorerie pile ce jour là.

Je me de dis que ça ne sert à rien d'avoir deux notifications visibles et en fait le système permet de mettre à jour le texte d'une notification et de la déacké (et puis finalement non voir ma branche, avoir deux identifiants ça permet de ne pas avoir à traiter le cas où la notification serait nouvelle ou pas, acké ou pas, on en crée toujours une nouvelle).

Voir ma branche avec mes modifications: http://git.entrouvert.org/combo.git/log/?h=wip/13222-invoice-notififications-bd

J'en ai profité pour faire quelques ajustements dans les notifications (plus facile de voir ce qui est utile quand on s'en sert):
  • je ne met plus automatiquement acked=False sur une re-notification (il faut le demander), dans le cas d'une synchro régulière state-less comme ici c'est plus chiant qu'autre chose
  • j'ai mis l'argument "user" optionnel sur le filtre .visible() (ça sert surtout dans les tests mais bon, ça reste logique)
  • je permet les tirets dans le coté droit d'un id de notification (on peut vouloir namespacé les id aussi en fait, ici reminder et première notification)
#75

Mis à jour par Frédéric Péters il y a presque 6 ans

La branche référence et contient des commits qui références #13222.

Je notais plus haut "Le ack d'une notification c'est une action de l'usager"; ça m'ennuie vraiment que l'API permette d'influencer ça.

#76

Mis à jour par Serghei Mihai il y a presque 6 ans

Ok pour moi pour les modifs que tu as apportées, Benj. Rajoute le bon numéro de ticket dans les messages de commit.

Comme le ack n'est pas utilisé dans le code de création des notifications par les factures, je rejoins Frédéric pour ne pas de-acker une notification lors du notify.

#77

Mis à jour par Benjamin Dauvergne il y a presque 6 ans

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

La branche référence et contient des commits qui références #13222.

Corrigé.

Je notais plus haut "Le ack d'une notification c'est une action de l'usager"; ça m'ennuie vraiment que l'API permette d'influencer ça.

Dans ce cas permettre de mettre à jour une notification existante je trouve ça étrange, notification acké, nouvelles informations poussée pourtant la notification reste considérée comme déjà acké :/

En fait le fait de mettre acked=True ne m'intéresse pas c'est acked=True => acked=False qui m'intéresse (mais de fait je permet les deux). Plusieurs possibilité:
  • je reviens à l'état initial: on peut mettre à jour une notification existante mais on a pas vraiment d'idée si l'utilisateur le saura ou pas (on peut inspecter notification.acked après coup mais pas moyen de s'en assurer via notify()), fonctionnement avant #22390.
  • je laisse acked comme champ du web-service mais je n'accepte que la valeur "true"
  • je vire la possibilité via le web-service mais je la conserve pour .notify()

Serghei Mihai a écrit :

Comme le ack n'est pas utilisé dans le code de création des notifications par les factures, je rejoins Frédéric pour ne pas de-acker une notification lors du notify.

C'est justement ce qui est actuellement fait depuis le commit du #22390, là je défais les choses partiellement.

#78

Mis à jour par Serghei Mihai il y a presque 6 ans

Benjamin Dauvergne a écrit :

En fait le fait de mettre acked=True ne m'intéresse pas c'est acked=True => acked=False qui m'intéresse (mais de fait je permet les deux). Plusieurs possibilité:
  • je reviens à l'état initial: on peut mettre à jour une notification existante mais on a pas vraiment d'idée si l'utilisateur le saura ou pas (on peut inspecter notification.acked après coup mais pas moyen de s'en assurer via notify()), fonctionnement avant #22390.

Virer 'acked': False de defaults me semble la bonne option.

Et pour traiter le cas de savoir si l'usager à vu et acké la notification dans un autre ticket.

#79

Mis à jour par Serghei Mihai il y a presque 6 ans

Dans ce ticket: #22650

#80

Mis à jour par Serghei Mihai il y a presque 6 ans

En regardant plus attentivement le commit http://git.entrouvert.org/combo.git/commit/?h=wip/13222-invoice-notififications-bd&id=ad3a44d64c0b29ce806b050d93cedeee0c72b160 retablit le fonctionnement actuel: on notify répété sur une notification ne la de-acke pas.
Mais laisse la possibilité de la de-acker explicitement si besoin.

Ack pour moi.

#81

Mis à jour par Frédéric Péters il y a presque 6 ans

Mais laisse la possibilité de la de-acker explicitement si besoin.

JE NE VEUX PAS PERMETTRE CELA.

#82

Mis à jour par Frédéric Péters il y a presque 6 ans

  • Statut changé de En cours à Résolu (à déployer)

Et ça a été poussé, sans que ça ne soit mentionné ici. Bravo.

#83

Mis à jour par Frédéric Péters il y a environ 5 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF