Produits Entr'ouvert » Combo

(patch 0001 plutôt impossible à relire avec du code qui se déplace dans tous les sens)

    def handle_payment(self, request, regie, items, remote_items=[], next_url='/', email=''):

Le remote_items=[] va gentiment nous être signalé par pylint comme une construction dangereuse. Comme en plus il n'est pas fait profit d'avoir des valeurs par défaut aux paramètres, je dirais de simplement retirer les valeurs par défaut. (au moins pour remote_items).

    url(r'^lingo/regie/(?P<regie_id>[\w,-]+)/item/(?P<item_id>[\w,-]+)/pay$',
        BasketItemPayView.as_view(), name='basket-item-pay-view'),

Je ne pense pas utile d'inclure la régie ici (on peut l'obtenir via l'item). L'item_id pourrait être limité à des chiffres.

        user = request.user if request.user.is_authenticated() else None
        if user is None:

Plutôt simplement if request.user and requet.user.is_authenticated():.

On autorise ici un utilisateur loggué à payer un élément de panier d'une autre personne, est-ce volontaire ?

À défaut d'une solution plus intelligente, il faudrait refuser l'appel à cette vue si la régie définit extra_fees_ws_url. (parce que ça permettrait de payer individuellement les différentes pièces et de laisser genre les frais postaux de côté).

Une raison pour ne pas placer le @atomic sur le handle_payment ?

Message pas clair de ma part plus haut, dans class BasketItemPayView, je voulais dire que je ne voyais pas le sens à déporter la condition dans une affectation user = ... if .. or ..., la condition, en elle-même, c'était mieux de la garder pour faire la gestion de l'erreur au plus tôt, que le niveau d'indentation de base de la méthode soit le cas passant (surtout qu'ici il y a un mix). Tant qu'à gérer ces situations, soyons francs via un HttpResponseForbidden, plutôt que redirection + message; bref :

if not (request.user and requet.user.is_authenticated()):
    return HttpResponseForbidden('No item payment for anonymous users')
...
if regie.extra_fees_ws_url:
    return HttpResponseForbidden('No targeted payment as extra fees are set')
...

Il me semble que #25552 serait traité ici, pour être sûr tu pourrais allonger ton test pour vérifier le bon suivi des redirections ? (uniquement si c'est supposé fonctionner, sinon ça sera patch dans l'autre ticket).

Yep, c'est à traiter ici.
En plus c'est le test que je fais en local pour m'assurer que tout fonctionne.

Je rajoute des tests.

J'ai du mal à suivre le test, j'ai l'impression que comme avant ça crée la redirection vers dummy-payment.

    assert resp.location.startswith('http://dummy-payment.demo.entrouvert.com/')
    qs = urlparse.parse_qs(urlparse.urlparse(resp.location).query)
    assert qs['amount'] == ['12.00']
...
    resp = app.get(qs['return_url'][0], params=data)

Ça teste qu'on envoie "return_url" à dummy-payment, avec la bonne valeur, mais il me semble qu'on ne vérifie rien du retour. (si je regarde le code ça doit passer par un lingo_next_url dans la session ?).

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

J'ai du mal à suivre le test, j'ai l'impression que comme avant ça crée la redirection vers dummy-payment.

resp = app.get(qs['return_url'][0], params=data)

Simule le retour de l'usager de la plateforme de paiement à travers le bouton "Retour au commerce". Ça renvoie vers Combo, qui redirige vers la next_url, stockée en session lors du lancement du paiement

Simule le retour de l'usager de la plateforme de paiement à travers le bouton "Retour au commerce".

Je reste pas clair là-dessus, pour moi ça ne teste pas ce qu'on veut, qui est qu'après le paiement on arrive sur /lingo/return/... et qu'il doit y avoir traitement et redirection. Peut-être plus clairement : je me serais attendu à avoir /lingo/return/... dans ton test.

qs['return_url'][0] contient l'url de retour vers combo: /lingo/return/<regie>/.
Et à l'appel vers return_url (qui est fait par le backend de paiement), une rédirection vers la next_url se fait.

Ou ton idée est d'appeler l'url de retour dans le style ?

app.get(reverse('lingo-return', kwargs={'regie_pk': ...}), params=data)

Le parcours, il est combo → payment → combo → next_url. Et ton test, il fait juste combo → payment, est-ce que j'ai donné à payment le nécessaire pour que derrière il y ait au final redirection à next_url, mais pas est-ce que le parcours → combo → next_url qui devrait suivre se joue vraiment correctement.

C'est ce que je fais:

• combo: lancement du paiement de l'element du panier avec une next_url:
  

      # set next_url to check if redirection is done after payment
      resp = app.get(payment_url, params={'next_url': 'http://example.net/form/id/'})
  

• payment: vérification que la rédirection est faite vers le backend
  

  assert resp.location.startswith('http://dummy-payment.demo.entrouvert.com/')
  

• backend de payment -> combo:
  

  resp = app.get(qs['return_url'][0], params=data)
  

Il manque ici la vérification que l'item est bien payé. Je vais le rajouter.

• vérification que combo fait la redirection vers next_url:
  

  assert resp.location == 'http://example.net/form/id/'
  

    resp = app.get(payment_url, params={'next_url': 'http://example.net/form/id/'})    
    assert resp.location.startswith('http://dummy-payment.demo.entrouvert.com/')

On est au moment où la redirection vers le système de paiement a lieu.

    qs = urlparse.parse_qs(urlparse.urlparse(resp.location).query)

On tape dans qs la query string de cette redirection.

resp = app.get(qs['return_url'][0], params=data)

Et là en fait le return_url tapé ici, c'est l'adresse /lingo/return/XXX/, ce que je ne captais pas.

Je pense critique de taper des commentaires pour noter les différentes étapes, parce que même en essayant d'y être attentif c'est super chaud à suivre, et quand il s'agira de comprendre à nouveau ça dans trois mois, ça sera la même misère.

Patch à jour avec des commentaires et check que l'item est bien marqué comme payé.

Manque le patch.

Et là en fait le return_url tapé ici, c'est l'adresse /lingo/return/XXX/, ce que je ne captais pas.

Et les deux lignes de commentaires ajoutés n'aident en rien. Je suis peut-être super con à lire ce test mais pour comprendre c'est plus facile pour moi d'avoir des commentaires qui décrivent les actions (sans empêcher les commentaires qui décrivent ce qui est testé), ex:

# simulate payment service redirecting the user to /lingo/return/... (eopayment
# dummy module put that URL in return_url query string parameter).
resp = app.get(...)
# check payment is marked as done
assert BasketItem.objects.filter(regie=regie, amount=amount, payment_date__isnull=False).exists()
# check user is redirected to the URL that was specified at the beginning
assert resp.location == 'http://example.net/form/id/'

1. simulate successfull payment response from dummy backend

successful.

1. simulate payment backend redirecting to /ling/return/, which is passed by

lingo.

Après ça devient intéressant que ça soit désormais quelqu'un d'autre que moi qui relise pour voir si le déroulé des tests lui est compréhensible. (parce que moi je trouve toujours peu claire l'affaire, qui suppose que le lecteur sait que ce return_url se trouve dans la redirection parce que le backend dummy d'eopayment l'y met, ce qui n'est pas noté, et le lecteur de se perdre parce que tu écris "passed by lingo" et qu'on ne trouve pas de return_url dans le code, etc. c'est contre ça que je mettais ma parenthèse "(eopayment dummy module put that URL in return_url query string parameter)".)

(bref je passe la main)

•         response.write(json.dumps({'result': 'success', 'id': str(item.id)}))
          payment_url = reverse('basket-item-pay-view', kwargs={'item_id': item.id})
          response.write(json.dumps({'result': 'success', 'id': str(item.id),
                                     'payment_url': request.build_absolute_uri(payment_url)}))

  
  À ce stade c'est juste pour faire chier mais il existe django.http.response.JsonResponse déjà en Django 1.8

•         next_url = request.GET.get('next_url') or '/'

  C'est de la prospective à ce stade mais c'est possible à un moment que '/' ne soit pas la racine du projet (si un jour on déploie sur des sous-chemins), il vaudrait mieux utiliser reverse(combo.public.views.page).

•     page = Page(title='xxx', slug='index', template_name='standard')
      page.save()
      cell = LingoBasketCell(page=page, placeholder='content', order=0)
      cell.save()
  

  .objects.create te ferait gagner une ligne (c'est discutable avec beaucoup d'arguments mais c'est plus clair je trouve)

•     data = {'amount': amount, 'display_name': 'test amount',
              'url': 'http://example.com'}

  c'est PEP8 mais moi ça me fait mal aux yeux, je préfère que tout soit aligné ou tout sur une ligne mais pas le mélange

•     payment_url = json.loads(resp.content)['payment_url']

  Toutes les réponses de webtest on un attribut .json, pas besoin de faire json.loads