facturation: pouvoir annuler un règlement (#88592) #176

Merged
lguerin merged 2 commits from wip/88592-invoicing-payment-cancel into main 2024-03-29 08:28:56 +01:00
Owner
No description provided.
lguerin added 2 commits 2024-03-26 11:28:45 +01:00
lguerin force-pushed wip/88592-invoicing-payment-cancel from ed86f60770 to 87b5933109 2024-03-26 11:29:37 +01:00 Compare
lguerin reviewed 2024-03-26 11:30:54 +01:00
@ -0,0 +17,4 @@
<div class="section">
<div class="pk-tabs">
<div class="pk-tabs--tab-list" role="tablist">
<button aria-controls="panel-payment" aria-selected="true" id="tab-payment" role="tab" tabindex="0">{% trans "Payments" %}</button>
Author
Owner

dans l'idée d'avoir aussi des motifs d'annulation pour les factures, j'ai fait un onglet "payments" pour avoir un onglet "invoices" dans un futur ticket

dans l'idée d'avoir aussi des motifs d'annulation pour les factures, j'ai fait un onglet "payments" pour avoir un onglet "invoices" dans un futur ticket
lguerin changed title from WIP: facturation: pouvoir annuler un règlement (#88592) to facturation: pouvoir annuler un règlement (#88592) 2024-03-26 11:36:37 +01:00
pmarillonnet requested review from pmarillonnet 2024-03-26 12:20:16 +01:00
Owner

(Je commence à relire.)

(Je commence à relire.)
pmarillonnet requested changes 2024-03-26 16:03:37 +01:00
Dismissed
pmarillonnet left a comment
Owner

Quelques petit trucs ramassés au passage, du détail vraiment.
J’ai fait une remarque sur les libellés 'Cancelled at' qui devraient être plutôt 'Cancelled on', je te laisse voir si par souci de cohérence on en profite pour renommer le champ de modèle cancelled_on lui aussi.

Quelques petit trucs ramassés au passage, du détail vraiment. J’ai fait une remarque sur les libellés `'Cancelled at'` qui devraient être plutôt `'Cancelled on'`, je te laisse voir si par souci de cohérence on en profite pour renommer le champ de modèle `cancelled_on` lui aussi.
@ -1317,0 +1321,4 @@
class Meta:
ordering = ['label']
unique_together = ['slug']
Owner

J’imagine qu’il y a une bonne raison pour ne pas avoir posé cette contrainte d’unicité dans le champ directement (genre avec un unique=True à la définition du champ) mais je ne saisis pas laquelle, je viens bien la savoir ne serait-ce que pour ma propre édification personnelle stp :)

J’imagine qu’il y a une bonne raison pour ne pas avoir posé cette contrainte d’unicité dans le champ directement (genre avec un `unique=True` à la définition du champ) mais je ne saisis pas laquelle, je viens bien la savoir ne serait-ce que pour ma propre édification personnelle stp :)
Author
Owner

non c'est une erreur, merci :) (fixup)

non c'est une erreur, merci :) (fixup)
pmarillonnet marked this conversation as resolved
@ -1433,0 +1463,4 @@
result = []
if not self.cancelled_at:
return result
result.append((_('Cancelled at'), self.cancelled_at.strftime('%d/%m/%Y %H:%M')))
Owner

Pareil ici, plutôt 'Cancelled on'.

Pareil ici, plutôt `'Cancelled on'`.
Author
Owner

fixup pour ça

fixup pour ça
pmarillonnet marked this conversation as resolved
@ -70,0 +86,4 @@
<tr class="line {% if forloop.last %}last-line{% endif %}" data-related-invoicing-element-id="{{ payment.pk }}" style="display: none;">
<td colspan="2"></td>
<td class="payment-details" colspan="3">
<i>{% blocktrans %}{{ label }}:{% endblocktrans %} {{ value }}</i>
Owner

Du détail i18n, mais je mettrais tout le {{ label }}: {{ value }} dans le bloc internationalisé (genre il y a des langues comme le grec ou bien certaines langues slaves où les deux points n’ont pas cette signification descriptive, et où il faudrait traduire le bloc entier d’une façon différente).

Du détail i18n, mais je mettrais tout le `{{ label }}: {{ value }}` dans le bloc internationalisé (genre il y a des langues comme le grec ou bien certaines langues slaves où les deux points n’ont pas cette signification descriptive, et où il faudrait traduire le bloc entier d’une façon différente).
Author
Owner

Alors il faudrait reprendre les trads de toutes les briques, on a souvent des chaînes qui terminent par ":".

Alors il faudrait reprendre les trads de toutes les briques, on a souvent des chaînes qui terminent par ":".
Owner

Oui je sais bien, je lutte contre ça mais tu as raison, ce bateau a déjà pris les voiles :)
(pour tout te dire il y a même des endroits dans les briques où c’est “internationalisé” en {{ label }}{% trans ": " %}{{ value }} 🙃).

Oui je sais bien, je lutte contre ça mais tu as raison, ce bateau a déjà pris les voiles :) (pour tout te dire il y a même des endroits dans les briques où c’est “internationalisé” en `{{ label }}{% trans ": " %}{{ value }}` 🙃).
pmarillonnet marked this conversation as resolved
@ -0,0 +27,4 @@
def get_context_data(self, **kwargs):
kwargs.update(
{
'payment_reason_list': PaymentCancellationReason.objects.all(),
Owner

Question d’interface utilisateur, peut-être un order_by('-disabled') ici, histoire, dans le rendu du gabarit, d’avoir les raisons actives apparaissant en premier, puis à la fin celles qui sont désactivées ?

Question d’interface utilisateur, peut-être un `order_by('-disabled')` ici, histoire, dans le rendu du gabarit, d’avoir les raisons actives apparaissant en premier, puis à la fin celles qui sont désactivées ?
Author
Owner

ok, je fais ça (fixup)

ok, je fais ça (fixup)
pmarillonnet marked this conversation as resolved
@ -0,0 +36,4 @@
reason_list = ReasonListView.as_view()
class PaymentReasonAddView(CreateView):
Owner

Pour quelle raison cette vue de création n’a-t-elle pas recours, comme la vue d’édition, à un formulaire, similaire ou identique au PaymentCancellationReasonForm, qui gérerait la prévention de doublons sur le slug ?

Pour quelle raison cette vue de création n’a-t-elle pas recours, comme la vue d’édition, à un formulaire, similaire ou identique au `PaymentCancellationReasonForm`, qui gérerait la prévention de doublons sur le slug ?
Author
Owner

le slug est automatiquement setté par la méthode save s'il n'est pas fourni

le slug est automatiquement setté par la méthode save s'il n'est pas fourni
Owner

Ah oui ok j’avais loupé ça, merci.

Ah oui ok j’avais loupé ça, merci.
pmarillonnet marked this conversation as resolved
@ -0,0 +65,4 @@
model = PaymentCancellationReason
def get_queryset(self):
return PaymentCancellationReason.objects.filter(payment__isnull=True)
Owner

J’ai peur qu’on perde des agents en route ici, qu’illes croient à un bug à ne pas voir apparaître ici la raison d’annulation qu’illes souhaitent supprimer, alors qu’en fait cette raison n’y apparaît pas car encore liée à un paiement.

Est-ce qu’on aurait pas intérêt, dans un autre ticket, à faire apparaître dans cet écran la liste des raisons encore actives et qui ne peuvent pas être supprimées ?

J’ai peur qu’on perde des agents en route ici, qu’illes croient à un bug à ne pas voir apparaître ici la raison d’annulation qu’illes souhaitent supprimer, alors qu’en fait cette raison n’y apparaît pas car encore liée à un paiement. Est-ce qu’on aurait pas intérêt, dans un autre ticket, à faire apparaître dans cet écran la liste des raisons encore actives et qui ne peuvent pas être supprimées ?
Author
Owner

en fait on n'affiche pas le lien de suppression s'il existe des paiements liés à ce motif, c'est juste une protection

en fait on n'affiche pas le lien de suppression s'il existe des paiements liés à ce motif, c'est juste une protection
Owner

Ok, encore mieux, nickel, merci.

Ok, encore mieux, nickel, merci.
pmarillonnet marked this conversation as resolved
@ -634,0 +635,4 @@
]
+ [v for k, v in PAYMENT_INFO]
+ [
_('Cancelled at'),
Owner

Sur les libellés exposés dans l’UI, je dirais plutôt Cancelled on vu que c’est un datetime et pas juste un heure (à vue de nez “Cancelled on March 26th, 3:43p.m.” m’a l’air plus correct que avec “Cancelled at …”).

Sur les libellés exposés dans l’UI, je dirais plutôt `Cancelled on` vu que c’est un datetime et pas juste un heure (à vue de nez “Cancelled on March 26th, 3:43p.m.” m’a l’air plus correct que avec “Cancelled at …”).
Author
Owner

fixup pour ça

fixup pour ça
pmarillonnet marked this conversation as resolved
gbaffoin reviewed 2024-03-26 16:14:51 +01:00
lguerin added 4 commits 2024-03-26 16:21:39 +01:00
Author
Owner

dernier fixup: j'avais oublié de ne pas mettre les motifs disabled dans le select du formulaire d'annulation :)

dernier fixup: j'avais oublié de ne pas mettre les motifs disabled dans le select du formulaire d'annulation :)
lguerin requested review from pmarillonnet 2024-03-26 16:23:36 +01:00
pmarillonnet approved these changes 2024-03-27 10:07:54 +01:00
pmarillonnet left a comment
Owner

Ok, j’ai vu tes modifs dans les !fixup à part, c’est bon pour moi. Ack.

Ok, j’ai vu tes modifs dans les !fixup à part, c’est bon pour moi. Ack.
lguerin force-pushed wip/88592-invoicing-payment-cancel from d2d1918f5a to defa13e032 2024-03-28 09:34:37 +01:00 Compare
lguerin changed target branch from wip/88544-basket-activity to main 2024-03-29 08:21:22 +01:00
lguerin force-pushed wip/88592-invoicing-payment-cancel from defa13e032 to 9417ab08e4 2024-03-29 08:25:36 +01:00 Compare
lguerin merged commit 9417ab08e4 into main 2024-03-29 08:28:56 +01:00
lguerin deleted branch wip/88592-invoicing-payment-cancel 2024-03-29 08:28:57 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/lingo#176
No description provided.