Produits Entr'ouvert » Combo

Après discussion avec Thomas, on préfere que ça soit lingo qui gère cette partie.
Un draft dans http://git.entrouvert.org/lingo.git/?h=teamnet

Il faut alors changer le projet dans ce ticket et, comme il s'agit d'un unique patch, il peut être attaché au ticket.

D'une première lecture.

Les deux migrations devraient sans doute être réunies.

+    ws_url = models.URLField(_('Webservice URL'), blank=True)

ws_url ? faudrait sans doute être plus spécifique, webservice pour faire quoi ?

+    def get_invoices(self, nameid):
+        url = self.ws_url + '/regie/%s/invoices/?orig=combo&NameID=%s&months=12' % (self.slug,
+                                                    nameid)

Notre objet "régie", il se trouve là maintenant dépendant d'un "ws_url" pour connaitre la liste des factures; j'ai l'impression qu'il manque une abstraction quelque part. Ça rejoint sans doute la question précédente; de la même manière qu'on a une abstraction pour le service de paiement, on en aurait une ici pour le "service de facturation" (?). (et donc → invoice_webservice_url ?).

+        url = self.ws_url + '/regie/%s/invoices/?orig=combo&NameID=%s&months=12' % (self.slug,
+                                                    nameid)
+        url = sign_url(url, key='xxx')

Si on met des valeurs bidons pour accélerer le développement initial (orig=combo, key=xxx), il faut au moins penser à mettre un gros TODO pour corriger ça plus tard.

+    def get_invoices(self, nameid):
+        url = self.ws_url + '/regie/%s/invoices/?orig=combo&NameID=%s&months=12' % (self.slug,
+                                                    nameid)

Le "months=12", j'en ferais un paramètre.

+        url = self.ws_url + ??? + '/?orig=combo&NameID=%s' % (nameid)
+        url = sign_url(url, key='xxx')
+        return requests.get(url).json()
...
+        url = self.ws_url + ??? + '/?orig=combo&NameID=%s' % (nameid)
+        url = sign_url(url, key='xxx')
+        return requests.get(url).json()
...
+        url = self.ws_url + ??? + '/?orig=combo&NameID=%s' % (nameid)
+        url = sign_url(url, key='xxx')
+        return requests.get(url).json()

Il me saute aux yeux une factorisation possible.

-        return Regie.objects.count() > 0
+        return Regie.objects.filter(ws_url='').count() > 0

Ça me semble très très bizarre, ça lie la gestion du panier à l'absence de webservice de facturation. S'il s'agit de faire un truc tout à fait différent, sans rien en commun, autant le faire ailleurs plutôt que mélanger du code sans rapport.

+class LingoRemoteRegie(CellBase):

Ça me semble plutôt être une cellule "Factures" qu'une cellule "régie distante" (ce qui parle à personne).

+        if request.GET.get('number'):
+            context.update({'invoice': self.regie.get_invoice(request.GET['number'],
+                                        nameid)})
+        else:
+            context.update({'invoices': self.regie.get_invoices(nameid)})

Ça sonne comme si la même cellule est à la fois la cellule affichant une liste de factures et la cellule affichant une seule facture, j'éviterais ce mélange, qu'il y ait une cellule affichant la liste des factures, point.

+      <a href="{{ page.get_online_url }}?number={{ invoice.nofacture }}" class="view">{% trans "View" %}</a>

En lien avec la note précédente, ce que je ferais ici, c'est ouvrir dans une popup le détail de la facture, plutôt qu'altérer le contenu de la cellule.

+<h2>{% trans "Your invoices for" %} {{ regie }}</h2>
...
+<h2>{% trans "Invoice nr. " %} {{ invoice.nofacture }}</h2>

On ne peut pas traduire les choses ainsi, dans certaines langues le "regie" ou le "numéro de facture" ne se trouveraient pas à la fin.

+    <td>{{ invoice.nofacture }}</td>
+    <td>{{ invoice.femission }}</td>
+    <td>{{ invoice.fmontant }} €</td>

Ce serait mieux d'avoir des attributs correspondant à l'anglais des libellés de colonne (no, date, amount) (et label et balance).

+    <td>{{ invoice.fmontant }} €</td>
...
+<p><strong>{% trans "Amount:" %}</strong> {{ invoice.fmontant }}€</p>

D'un côté il y a un espace entre le montant et l'€, pas dans l'autre.

-from .views import RegiesApiView, AddBasketItemApiView, PayView, CallbackView
+from .views import RegiesApiView, AddBasketItemApiView, PayView, CallbackView, \
+    InvoiceDownloadView

Utiliser des parenthèses plutôt que le \.

+class InvoiceDownloadView(View):
+    http_method_names = [u'get']
+
+    def get(self, request, *args, **kwargs):
+        mellon = self.request.session.get('mellon_session')
+        if mellon:

Ici, je ne suis fan de l'arrivée du mellon_session, et là aussi je vivrais très bien la présence d'une couche intermédiaire.

On pourrait avoir "regie.download_invoice(context, kwargs['invoice_id'])", et que ça soit au download_invoice de faire sa popote pour retrouver le nécessaire selon l'utilisateur connecté.

+ invoice = regie.download_invoice(kwargs['invoice_id'], mellon['name_id_content'])

Et c'est le download_invoice qui, selon moi, doit râler et lever une exception si la facture demandée ne correspond pas à l'usager.

Et faire le décodage base64.