Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amélioration de l'outil de diff #215

Merged
merged 5 commits into from
Aug 5, 2015
Merged

Amélioration de l'outil de diff #215

merged 5 commits into from
Aug 5, 2015

Conversation

yapper-git
Copy link

Q R
Correction de bugs ? oui
Nouvelle Fonctionnalité ? oui
Tickets (issues) concernés Sur ZdS

Cette PR est très semblable à celle ci pour zestedesavoir:

En résumé, voici les modifications:

  • comparaison dans le bon ordre (ancien=gauche, nouveau=droite) et affichage des fichiers modifiés, supprimés, renommés ou ajoutés corrects (actuellement buggé/foireux)
    génération d'une erreur 404 en cas de sha invalide (si l'utilisateur change l'URL) au lieu d'une 500…
  • utilisation de make_table au lieu de make_file ⇒ évite d'avoir une page HTML dans une autre (interdit sans iframe) + supprime les légendes multiples pour chaque diff + déplace le CSS (autrefois répété) dans un seul endroit
  • affichage de scroll pour les diffs seulement si besoin
  • utilisation de deux paramètres dans l'URL from et to (qui offre la possibilité de comparer deux versions quelconques)
  • utilisation d'un tag simple pour comparer deux chaînes {% htmldiff "string1" "string2" %} au lieu de deux filtres incompréhensibles avec du with…
  • ajout de la possibilité de comparer deux versions quelconques avec des radiobutton et un bouton de validation

NB: Cette PR ne touche pas au code pour les articles (d'ailleurs il est actuellement impossible d'avoir un diff d'un article mais cela sera réglé par la ZEP-12…). Bref cette PR ne concerne que les tutoriels.

QA

Tester les pages correspondant à l'historique d'un tutoriel (liste des commits + diffs) dans le cas où le tutoriel vient juste d'être créé (= 1 seul commit), avec plusieurs commits (possibilité de voir les modifications d'un commit donné).
Modifier les paramètres from et to dans l'URL et vérifier que cela génère une erreur 404.
Vérifier que les modifications d'un tutoriel/article (ajout section, correction typos) apparaissent bien dans le diff en question.
Vérifier que les bugs sont bien corrigés.

@yapper-git
Copy link
Author

Il reste quelques détails à voir/vérifier, notamment il semble qu'il y ait un décalage des dates pour les commits (pb de timezone?), je vais regarder et essayer de voir ce qu'il en est.

Le code actuel permet de mettre un article en béta depuis l'historique d'un article (ou un tuto).
Avec la ZEP-12, vous avez ajouter la possibilité de mettre un article en béta, c'est bien ça ?

@artragis
Copy link
Owner

artragis commented Aug 3, 2015

C'est tout à fait ça @yapper-git.
Je laisse ta PR en attente que vous ayiez pu corriger les petits détails. Surtout que je ne peux pas mettre ta PR sur mon serveur pour l'instant car j'ai déjà la PR de la recherche en test (et qu'elle est un poil plus prioritaire).

@yapper-git
Copy link
Author

Voilà je viens de modifier la PR pour prendre en compte le fuseau horaire du commit et éviter les décalages de 2 heures…

@artragis
Copy link
Owner

artragis commented Aug 3, 2015

passe pas les tests car la ZEP-12 corrigeait certains comportement mauvais de l'ancienne version. Du coup va falloir rechanger le comportement ou vous adapter aux tests, notamment, on doit pouvoir regarder le "diff" du premier commit.

@pierre-24
Copy link
Collaborator

on doit pouvoir regarder le "diff" du premier commit.

... Est ce vraiment nécessaire ?

@yapper-git
Copy link
Author

passe pas les tests car la ZEP-12 corrigeait certains comportement mauvais de l'ancienne version. Du coup va falloir rechanger le comportement ou vous adapter aux tests, notamment, on doit pouvoir regarder le "diff" du premier commit.

Je vais regarder, j'avais pas pensé à lancer les tests…

on doit pouvoir regarder le "diff" du premier commit.
... Est ce vraiment nécessaire ?

Non c'est pas nécessaire à mon goût. C'est un cas particulier à gérer qui ne s'y prête pas très bien dans le code ou alors si faut ajouter des conditions…

Le test semble avoir été ajouté pour éviter une erreur 500 (code actuel) en cliquant sur le diff du premier commit, dans ma PR c'est une erreur 404 volontairement. Je vais modifier le test je pense.

@artragis
Copy link
Owner

artragis commented Aug 3, 2015

... Est ce vraiment nécessaire ?

on s'en est rendu compte car un membre a signalé le bug, donc je pense que
oui.

Le 3 août 2015 16:03, Yapper notifications@github.com a écrit :

passe pas les tests car la ZEP-12 corrigeait certains comportement mauvais
de l'ancienne version. Du coup va falloir rechanger le comportement ou vous
adapter aux tests, notamment, on doit pouvoir regarder le "diff" du premier
commit.

Je vais regarder, j'avais pas pensé à lancer les tests…

on doit pouvoir regarder le "diff" du premier commit.
... Est ce vraiment nécessaire ?

Non c'est pas nécessaire à mon goût. C'est un cas particulier à gérer qui
ne s'y prête pas très bien dans le code ou alors si faut ajouter des tests…

Le test semble avoir été ajouté pour éviter une erreur 500 (code actuel)
en cliquant sur le diff du premier commit, dans ma PR c'est une erreur 404
volontairement. Je vais modifier le test je pense.


Reply to this email directly or view it on GitHub
#215 (comment).

@yapper-git
Copy link
Author

Voir zestedesavoir#2308

@yapper-git
Copy link
Author

J'ai regardé le test test_diff_for_new_content sauf que c'est pas terrible…

Le test consiste à se connecter, créer un tuto vide, aller voir l'historique, vérifier qu'on obtient le code 200.
Sauf que :

  1. l'URL ne contient pas de paramètre GET nommé version (pas normal)
  2. le diff en question est totalement vide, le visiteur n'a aucune info donc autant ne pas lui proposer un diff vide (et lancer une erreur 404 s'il essaye d'y accéder)

Je vais tenter d'ajouter des tests.

@artragis
Copy link
Owner

artragis commented Aug 3, 2015

Sauf que :

  1. l'URL ne contient pas de paramètre GET nommé version (pas normal)
  2. le diff en question est totalement vide, le visiteur n'a aucune info donc autant ne pas lui proposer un diff vide (et lancer une erreur 404 s'il essaye d'y accéder)

en effet. on a été un peu vite sur ce test là. En fait on a juste voulu vérifier qu'on n'avait pas de 500 :p
C'est vrai que le 404 peut etre une bonne idée.

@yapper-git
Copy link
Author

J'ai voulu ajouter un test pour DisplayHistory mais j'y arrive pas et je comprend pas pk ça bugge.

J'ai testé ceci

    def test_display_history(self):
        """Test DisplayHistory view"""

        tuto = PublishableContent.objects.get(pk=self.tuto.pk)

        # no public access
        self.client.logout()
        result = self.client.get(
            reverse('content:history', args=[tuto.pk, tuto.slug]),
            follow=False)
        self.assertEqual(result.status_code, 302)

        # login as guest and test the non-access
        self.client.logout()
        self.assertEqual(
            self.client.login(
                username=self.user_guest.username,
                password='hostel77'),
            True)
        result = self.client.get(
            reverse('content:history', args=[tuto.pk, tuto.slug]),
            follow=False)
        self.assertEqual(result.status_code, 403)

        # staff access
        self.client.logout()
        self.assertEqual(
            self.client.login(
                username=self.user_staff.username,
                password='hostel77'),
            True)
        result = self.client.get(
            reverse('content:history', args=[tuto.pk, tuto.slug]),
            follow=False)
        self.assertEqual(result.status_code, 200)

        # login as author and test the access
        self.client.logout()
        self.assertEqual(
            self.client.login(
                username=self.user_author.username,
                password='hostel77'),
            True)
        result = self.client.get(
            reverse('content:history', args=[tuto.pk, tuto.slug]),
            follow=False)
        self.assertEqual(result.status_code, 200)

Mais j'ai une erreur bizzarre :

$ python manage.py test zds.tutorialv2.tests.tests_views.ContentTests.test_display_history
Creating test database for alias 'default'...
E
======================================================================
ERROR: test_display_history (zds.tutorialv2.tests.tests_views.ContentTests)
Test DisplayHistory view
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/yvand/src/git/zds-site-yapper/zds/tutorialv2/tests/tests_views.py", line 1723, in test_display_history
    follow=False)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/client.py", line 470, in get
    **extra)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/client.py", line 286, in get
    return self.generic('GET', path, secure=secure, **r)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/client.py", line 358, in generic
    return self.request(**r)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/client.py", line 440, in request
    six.reraise(*exc_info)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/core/handlers/base.py", line 137, in get_response
    response = response.render()
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/response.py", line 103, in render
    self.content = self.rendered_content
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/response.py", line 80, in rendered_content
    content = template.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 148, in render
    return self._render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/utils.py", line 88, in instrumented_test_render
    return self.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 126, in render
    return compiled_parent._render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/utils.py", line 88, in instrumented_test_render
    return self.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 126, in render
    return compiled_parent._render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/utils.py", line 88, in instrumented_test_render
    return self.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 126, in render
    return compiled_parent._render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/test/utils.py", line 88, in instrumented_test_render
    return self.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 65, in render
    result = block.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 65, in render
    result = block.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 65, in render
    result = block.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/loader_tags.py", line 65, in render
    result = block.nodelist.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 844, in render
    bit = self.render_node(node, context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/debug.py", line 80, in render_node
    return node.render(context)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/defaulttags.py", line 201, in render
    nodelist.append(node.render(context))
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/defaulttags.py", line 525, in render
    six.iteritems(self.extra_context))
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/defaulttags.py", line 524, in <genexpr>
    values = dict((key, val.resolve(context)) for key, val in
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/template/base.py", line 624, in resolve
    new_obj = func(obj, *arg_vals)
  File "/home/yvand/src/git/zds-site-yapper/zds/utils/templatetags/profile.py", line 28, in user
    current_user = User.objects.get(pk=user_pk)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/manager.py", line 92, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/query.py", line 345, in get
    clone = self.filter(*args, **kwargs)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/query.py", line 691, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/query.py", line 709, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/sql/query.py", line 1331, in add_q
    clause, require_inner = self._add_q(where_part, self.used_aliases)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/sql/query.py", line 1358, in _add_q
    current_negated=current_negated, connector=connector)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/sql/query.py", line 1230, in build_filter
    condition = self.build_lookup(lookups, col, value)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/sql/query.py", line 1138, in build_lookup
    return final_lookup(lhs, rhs)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/lookups.py", line 82, in __init__
    self.rhs = self.get_prep_lookup()
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/lookups.py", line 85, in get_prep_lookup
    return self.lhs.output_field.get_prep_lookup(self.lookup_name, self.rhs)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/fields/__init__.py", line 646, in get_prep_lookup
    return self.get_prep_value(value)
  File "/home/yvand/src/git/zds-site-yapper/zdsenv/lib/python2.7/site-packages/django/db/models/fields/__init__.py", line 915, in get_prep_value
    return int(value)
ValueError: invalid literal for int() with base 10: 'firm2'

----------------------------------------------------------------------
Ran 1 test in 1.387s

FAILED (errors=1)
Destroying test database for alias 'default'...

Si qqn a une idée je suis preneur ! :D

@pierre-24
Copy link
Collaborator

Quelle ligne correspond à la 1723 ?

@yapper-git
Copy link
Author

^^ de plus en plus louche !

Ligne 1723 :

        self.assertEqual(result.status_code, 200)

Et les lignes précédentes :

        self.client.logout()
        self.assertEqual(
            self.client.login(
                username=self.user_staff.username,
                password='hostel77'),
            True)
        result = self.client.get(
            reverse('content:history', args=[tuto.pk, tuto.slug]),
            follow=False)
        self.assertEqual(result.status_code, 200)

@pierre-24
Copy link
Collaborator

Il a l'air perdu quelque part en transformant le template de la page, vu que

  File "/home/yvand/src/git/zds-site-yapper/zds/utils/templatetags/profile.py", line 28, in user
    current_user = User.objects.get(pk=user_pk)

et que le user_pk, pour une raison qui m'échappe, est firm2 (soit le login et non le pk)

@yapper-git
Copy link
Author

Je regarderai plus en détail plus tard, il semble buggé sur la ligne juste au-dessus, j'ai du ajouter un saut de ligne, il bug sur la ligne follow=False), je retesterai + tard.

@pierre-24
Copy link
Collaborator

Bref, ça confirme ce que je dit, il bugge dans l’interprétation de la template. Ceci dit, je suis pas sur et certain du comportement de GIT via les environnement de tests (par exemple, je ne suis pas certain que le commit initial qui crée le tutoriel dans les environnements de tests soient relié à un utilisateur)

@yapper-git
Copy link
Author

Merci @pierre-24 pour ton aide !!

Je crois avoir compris d'où venait le problème :

  • l'auteur du commit doit être firm2, or cela ne correspond pas à l'ID d'un membre de ZdS (ce n'est même pas un nombre)
  • j'avais fait un git checkout pour tester un truc et oublié de revenir à mes modifications et avoir un code qui fonctionne même si l'auteur du commit n'est pas un membre de ZdS…

Voilà maintenant ça marche chez moi et j'espère sur Travis.

@pierre-24
Copy link
Collaborator

Rapport de QA:

  • J'adore. Vraiment :)
  • On ne sais pas quel est le fichier qui a été modifié: est-ce que c'est voulu ?

Du reste, grâce à toi (et ton diff enfin efficace), je me suis rendu compte que notre procédure de migration n'était pas correcte, je vais donc continuer ma QA quand j'aurai corrigé çà :)

@yapper-git
Copy link
Author

On ne sais pas quel est le fichier qui a été modifié: est-ce que c'est voulu ?

C'est précisé dans des <h2> sauf erreur, ça ne suffit pas ?
Un titre sur les colonnes c'est simple à rajouter, mais je pensais que cela serait redondant.

Vous préférez quoi ?

  1. Le nom de fichier que dans un h2
  2. Le nom de fichier que dans un th en haut de tableau (nom a priori identique dans les 2 colonnes)
  3. Le nom de fichier dans un h2 et dans un th

Du reste, grâce à toi (et ton diff enfin efficace), je me suis rendu compte que notre procédure de migration n'était pas correcte, je vais donc continuer ma QA quand j'aurai corrigé çà :)

De quelle procédure de migration parles-tu ?

@pierre-24
Copy link
Collaborator

C'est précisé dans des <h2> sauf erreur, ça ne suffit pas ?

Ben justement, je vois pas les fameux <h2> en question, donc je m'étonne :

screenshot from 2015-08-04 20 03 54

De quelle procédure de migration parles-tu ?

De la procédure qui migre les articles et tutos de "avant la ZEP-12" à la ZEP-12 (voir ici). Sauf que grâce à ton magnifique outil, je me suis rendu compte qu'on oubliais d'indexer le manifest.json. Rien à voir avec toi, mais par contre, il faudra que je vérifie que ton outil fonctionne aussi dans ce cas là :)

@yapper-git
Copy link
Author

Je comprend pas bien pourquoi les titres contenant les noms de fichiers n'apparaissent pas…oO

Quand tu regardes le code source, tu as des <h3> ? (ce sont des <h3> et non des <h2> je m'étais trompé).

@pierre-24
Copy link
Collaborator

Alors, deux choses:

  1. Il y a bien des <h3>, mais ils sont vides
  2. Pour une raison que j'ai pas encore comprise, la migration des articles fait planter ton code: on peut comparer des commits "pré-migration" entre eux, des commits "post-migration" entre eux, mais si on compare des commits pré- et post-migration, on se mange une erreur d'UTF-8. (DjangoUnicodeDecodeError In template /home/pbeaujea/Devels/zds-site/templates/tutorialv2/view/diff.html, error at line 77). Ceci dit, comme l'erreur n'arrive pas avec les migration de tutoriels, je sais pas si c'est ton code qui plante ou si c'est la procédure de migration qui fait des trucs qu'elle ne devrait pas faire. J'investigue.

@pierre-24
Copy link
Collaborator

Bon, pour le point 2, ça a clairement pas l'air d'être de ta faute: pour une raison que je comprend pas, on ce mange des fin de ligne "à la windows" quand le script de migration "découpe" les articles. Oublie ça le temps que je trouve une solution.

@pierre-24
Copy link
Collaborator

Bon, tords partagés: on avait des erreurs UTF-8 des deux côtés. Du coup, j'ai fait une proposition de PR pour intégrer à celle-ci :)

pierre-24 and others added 2 commits August 5, 2015 08:45
- Apparition des chemins de fichier
- Erreurs UTF-8
- "Pas de changements"
@pierre-24
Copy link
Collaborator

À merger quand Travis est content. Puis remonte les une ou deux corrections (sur htmldiff et pour les chemins) sur upstream et j'irai merger là bas aussi.

@pierre-24
Copy link
Collaborator

On est bon :)

pierre-24 added a commit that referenced this pull request Aug 5, 2015
@pierre-24 pierre-24 merged commit 7c1639e into artragis:zep_12_double_stack Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants