Produits Entr'ouvert » Chrono

top, nickel

Pas vraiment lu le code, je me suis arrêtée au résultat :)
Ce qui me gêne: sur d'autres écrans chrono (ou même combo ou wcs), on a plutôt un formulaire "dupliquer" qui ne présente que les champs éditables (ici ça serait jour et heure si on suit la demande, peut-être les champs qui parlent de récurrence si la source est un event récurrent);
Je ne crois pas qu'on ait un formulaire de création init avec des données existantes (mais je peux me tromper).

Autres retours bienvenus.

a minima, il faudrait libellé, jour, heure.
Le reste peut se paramétrer par la suite.

Lauréline Guerin a écrit :

Pas vraiment lu le code, je me suis arrêtée au résultat :)
Ce qui me gêne: sur d'autres écrans chrono (ou même combo ou wcs), on a plutôt un formulaire "dupliquer" qui ne présente que les champs éditables (ici ça serait jour et heure si on suit la demande, peut-être les champs qui parlent de récurrence si la source est un event récurrent);
Je ne crois pas qu'on ait un formulaire de création init avec des données existantes (mais je peux me tromper).

Autres retours bienvenus.

Est-ce un problème que la fonction dupliquer permette d'éditer ces champs ? Ça me parait plus confortable pour l'utilisateur qui souhaite dupliquer un évènement et changer le nombre de places dispo de pouvoir le faire directement, avec le même formulaire que pour créer un évènement, plutôt que de devoir faire une première duplication, et ensuite devoir remodifier l'entrée après plusieurs clics.

(et au niveau code c'est aussi plus simple, puisqu'on peut réutiliser exactement les mêmes formulaires, ne pas rajouter de vues, templates, etc.)

Agate Berriot a écrit :

Est-ce un problème que la fonction dupliquer permette d'éditer ces champs ? Ça me parait plus confortable pour l'utilisateur qui souhaite dupliquer un évènement et changer le nombre de places dispo de pouvoir le faire directement, avec le même formulaire que pour créer un évènement, plutôt que de devoir faire une première duplication, et ensuite devoir remodifier l'entrée après plusieurs clics.

(et au niveau code c'est aussi plus simple, puisqu'on peut réutiliser exactement les mêmes formulaires, ne pas rajouter de vues, templates, etc.)

je vous laisse techniquement décider de ce qui est mieux ou non, mais d'un point de vue fonctionnel, le même form qu'à la création est plus ergo je trouve. ça créé des habitudes.

Sur l'existant d'un tour assez rapide, on est plutôt à avoir un mini-formulaire pour la duplication,

• dans combo sur la duplication d'une page on demande juste le titre, champ vide,
• dans chrono sur la duplication d'un agenda pareil,
• dans w.c.s. sur la duplication d'un formulaire on demande juste le titre mais on préremplit le champ avec "titre existant (copie)",

mais ce qu'on a combo/wcs c'est qu'une fois dupliqué le nouvel élément n'est pas "actif" par défaut (soit désactivé pour le formulaire, soit hors de la navigation pour une page), et pour chrono on peut se dire que c'est pareil, sans formulaire qui en présente les créneaux, c'est comme s'il était désactivé.

Mais ça ne s'applique pas aux événements, dès qu'on va le dupliquer il peut apparaître dans une démarche branchée sur l'agenda, donc ça me semble plutôt nécessaire de pouvoir le paramétrer dès le début.

Oui on pourrait forcer une date de publication sur l'événement dupliqué pour permettre la configuration mais je ne suis pas tout à fait à l'aise avec ça, si c'était juste un flag "publié" ça m'irait mieux, je crois. (mais ça demanderait davantage de réflexions).

Je pense aussi que ça aurait été bien de suivre le même mode de duplication que dans les autres briques mais qu'on est coincé par le fait que l'évènement va direct apparaître dans les formulaires, donc l'approche ici me va.

• Ça me paraît important de réussir à passer par Event.duplicate (ça nécessitera d'ajouter un paramètre save vrai par défaut).
• Je préférerais ne pas complexifier la vue d'ajout, plutôt avoir une nouvelle vue de duplication dédiée.

J'imagine que ça se fait bien avec une UpdateView et le get_object qui fait event = super().get_object(); return event.duplicate(save=False), plus besoin d'aller jouer du initial/setattr. Si ça ne se fait pas bien alors il ne faut peut-être pas m'écouter :)

Valentin Deniaud a écrit :

Je pense aussi que ça aurait été bien de suivre le même mode de duplication que dans les autres briques mais qu'on est coincé par le fait que l'évènement va direct apparaître dans les formulaires, donc l'approche ici me va.

Remarques sur le code :

• Ça me paraît important de réussir à passer par Event.duplicate (ça nécessitera d'ajouter un paramètre save vrai par défaut).
• Je préférerais ne pas complexifier la vue d'ajout, plutôt avoir une nouvelle vue de duplication dédiée.

J'imagine que ça se fait bien avec une UpdateView et le get_object qui fait event = super().get_object(); return event.duplicate(save=False), plus besoin d'aller jouer du initial/setattr. Si ça ne se fait pas bien alors il ne faut peut-être pas m'écouter :)

Sauf erreur de ma part, la fonction Event.duplicate copie un certain nombre de choses qu'on ne va pas vouloir garder sur un nouvel évènement, par exemple la date de publication, le nombre de places déjà réservées, si l'évènement est complet, divers timestamps…

Pour le fait de ne pas complexifier la vue d'ajout, je veux bien créer une vue dédiée duplicate, mais je ne vois pas forcément l'intérêt : un duplicate, c'est dans les fait une création en changeant certaines choses.

Un dernier point concernant l'UX : quand on fait une duplication comme sur les autres formulaires, au hasard duplication d'un agenda ou d'un formulaire, on se retrouve avec un formulaire sans contexte : on ne sait pas ce qu'on duplique, quels sont les attributs de l'objet source. Utiliser le même formulaire que pour la création avec un pré-remplissage permet de résoudre ce problème, tout en offrant davantage de flexibilité à l'utilisateur.

Maintenant si le consensus est de garder la cohérence avec l'existant, je fais ça :)

Agate Berriot a écrit :

Sauf erreur de ma part, la fonction Event.duplicate copie un certain nombre de choses qu'on ne va pas vouloir garder sur un nouvel évènement, par exemple la date de publication, le nombre de places déjà réservées, si l'évènement est complet, divers timestamps…

Mon idée c'est qu'il ne faut pas que la vue ait à se soucier de savoir comment dupliquer un évènement. Au début j'étais parti pour dire qu'il manquait dans le patch la duplication des custom fields et je me suis aperçu qu'en fait ça manquait dans Event.duplicate. Mais l'argument reste le même, le jour où ça sera géré on ne voudra avoir qu'un seul endroit à modifier. De même, si on ajoute un champ au modèle évènement modifier le code dans la vue de duplication sera systématiquement oublié (encore plus si elle s'appelle vue d'ajout :)).

Event.duplicate sert vraiment à dupliquer un évènement, si ça dupliquait aussi le statut complet/nombre de places réservées etc ce serait un bug. Tu as raison cette partie est tout à fait obscure, c'est il me semble mis à jour par des triggers en db (#54747).

J'ai effectivement l'impression que ça duplique les timestamps, ça ressemble à un bug : mais c'est à investiguer/gérer dans un autre ticket.

Pour la date de publication, je pense qu'il faut la dupliquer, ça me paraît peu clair que la duplication modifie la valeur de certains champs.

Pour le fait de ne pas complexifier la vue d'ajout, je veux bien créer une vue dédiée duplicate, mais je ne vois pas forcément l'intérêt : un duplicate, c'est dans les fait une création en changeant certaines choses.

C'est sûrement subjectif alors, moi je trouve qu'il faudrait mieux distinguer les deux. Par exemple avoir un titre contenant le mot « Duplication » dans la popup, peut-être ne garder dans le formulaire que les champs qui configurent la date. Modification mineures qui seraient compliquées à faire dans la vue d'ajout, faciles si on sépare les choses.

Un dernier point concernant l'UX : quand on fait une duplication comme sur les autres formulaires, au hasard duplication d'un agenda ou d'un formulaire, on se retrouve avec un formulaire sans contexte : on ne sait pas ce qu'on duplique, quels sont les attributs de l'objet source. Utiliser le même formulaire que pour la création avec un pré-remplissage permet de résoudre ce problème, tout en offrant davantage de flexibilité à l'utilisateur.

Mais le formulaire d'ajout étant partiel, ça va quand même prendre des attributs de l'objet source sans les afficher explicitement (en vérité ça m'irait bien que la duplication amène direct sur le formulaire d'édition avec tous les champs, mais peut-être ne changeons pas tout à la fois).

Valentin Deniaud a écrit :

la duplication des custom fields et je me suis aperçu qu'en fait ça manquait dans Event.duplicate

En fait si c'est juste un JSONField donc c'est géré par la duplication telle qu'elle est codée, j'ai rien dit.

Mon dernier patch passe sure une vue et un formulaire de duplication dédiée. Par contre j'ai un problème avec mon env de dev, les changements dans le code ne sont pas pris en compte quand je démarre le service. J'essaye de comprendre pourquoi.

Agate Berriot a écrit :

Mon dernier patch passe sure une vue et un formulaire de duplication dédiée. Par contre j'ai un problème avec mon env de dev, les changements dans le code ne sont pas pris en compte quand je démarre le service. J'essaye de comprendre pourquoi.

C'est bon j'ai trouvé le souci, tout fonctionne comme discuté (modale qui affiche juste le label et la date/heure de l'évènement à dupliquer, appel à la fonction Event.duplicate)

• Je pense que la duplication sera toujours utilisée depuis la vue paramétrage, l'accès direct à la duplication dans manager_event_detail.html peut être retirée
• Il est tout à fait légitime d'avoir plusieurs évènements avec le même label, à mon avis ce sera même le cas d'usage majoritaire, donc ça serait bien d'avoir le champ label prérempli
• messages.info puis dans le message « successfully », plutôt messages.success alors ?
  • Je ne comprends pas du tout le len(form.instance.label) dans le message (et c'est souvent bien de vérifier dans les tests qu'il apparaît)
• Le bouton cancel dans le template redirige vers le formulaire (oui c'est marginal et visible seulement en ouvrant le lien dupliquer dans un nouvel onglet), il faudrait qu'il redirige vers la page précédente genre la vue settings
• Dans le breadcrumb on peut ajouter une ligne <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a>
  • Tu peux aussi essayer d'utiliser le template assez minimal et générique manager_agenda_form.html pour ne pas t'embêter

De manière générale je ne suis toujours pas 100% convaincu par le parcours popup -> formulaire d'édition, ça pourrait être direct formulaire d'édition ou au contraire juste popup et aller éditer l'évènement si besoin, mais passons ça comme ça et voyons les éventuels retours.

Agate Berriot a écrit :

Et comme dans ce popup on a toutes les actions possibles pour un évènement (supprimer, annuler, éditer, etc.)

Oui mais si tu regardes comment est fabriquée l'action éditer il y a un paramètre ?next en plus, pour qu'un parcours visualisation reste sur les vues de visualisation et un parcours paramétrage reste sur les vues paramétrage. Et même ça ce n'est pas très bien fait (le lien breadcrumb ne correspond pas à la page précédente).

Donc on simplifie pas mal de chose à ne pas avoir le bouton dupliquer côté visualisation. Certes on l'a puisqu'on peut éditer -> dupliquer, mais ça me va de négliger ça ou de regarder si request.GET.next == 'settings' pour le masquer ou non.

Side note, il me semble que le bouton dupliquer vivrait mieux à côté de la croix pour supprimer un évènement dans la vue de paramétrage, mais c'est sûrement pas si simple à faire gardons ça pour un autre ticket.

• Je ne comprends pas du tout le len(form.instance.label) dans le message (et c'est souvent bien de vérifier dans les tests qu'il apparaît)

C'était un reliquat d'un copier coller, c'est corrigé ;)

Il manque le bout de test :)

• Dans le breadcrumb on peut ajouter une ligne <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a>
  • Tu peux aussi essayer d'utiliser le template assez minimal et générique manager_agenda_form.html pour ne pas t'embêter

J'ai changé de template de base comme suggéré

Mmmh je ne vois pas la modif dans le patch ?

• Ne pas permettre la duplication de ces évènements, ne permettre que la duplication de l'évènement de base (donc juste checker que primary_event est nul pour permettre la duplication).
  • La raison c'est qu'un cron tourne pour mettre à jour tout ça à l'ajout d'exceptions et qu'il ne s'attend pas à ce que les évènements liés à un évènement récurrent diffèrent de ceux qu'on a créé automatiquement.
• Copier/coller le save() de NewEventForm pour que les récurrences soient créées à la duplication (actuellement elles le sont au save() de la vue d'édition qui suit, mais on est pas obligé de cliquer sur enregistrer sur cette page).

Valentin Deniaud a écrit :

Agate Berriot a écrit :

Et comme dans ce popup on a toutes les actions possibles pour un évènement (supprimer, annuler, éditer, etc.)

Oui mais si tu regardes comment est fabriquée l'action éditer il y a un paramètre ?next en plus, pour qu'un parcours visualisation reste sur les vues de visualisation et un parcours paramétrage reste sur les vues paramétrage. Et même ça ce n'est pas très bien fait (le lien breadcrumb ne correspond pas à la page précédente).

Donc on simplifie pas mal de chose à ne pas avoir le bouton dupliquer côté visualisation. Certes on l'a puisqu'on peut éditer -> dupliquer, mais ça me va de négliger ça ou de regarder si request.GET.next == 'settings' pour le masquer ou non.

Okay, je retire :)

Il manque le bout de test :)

C'est la dernière ligne du test test_duplicate_event: assert 'Event successfully duplicated.' in str(resp.body)

• Dans le breadcrumb on peut ajouter une ligne <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a>
  • Tu peux aussi essayer d'utiliser le template assez minimal et générique manager_agenda_form.html pour ne pas t'embêter

J'ai changé de template de base comme suggéré

Mmmh je ne vois pas la modif dans le patch ?

Effectivement, je pensais l'avoir fait, ça devrait être bon

Point important, je me rends compte qu'il manque un (petit) truc pour gérer les évènements récurrents. La manière dont on a construit ça c'est que quand un évènement récurrent est créé, il n'est lui-même pas réservable par contre il entraîne la création de nouveaux évènements qui eux le sont, suivant la récurrence définie. Ces évènements sont liés à l'évènement récurrent par l'attribut primary_event. Pour gérer ça il faut :

• Ne pas permettre la duplication de ces évènements, ne permettre que la duplication de l'évènement de base (donc juste checker que primary_event est nul pour permettre la duplication).
  • La raison c'est qu'un cron tourne pour mettre à jour tout ça à l'ajout d'exceptions et qu'il ne s'attend pas à ce que les évènements liés à un évènement récurrent diffèrent de ceux qu'on a créé automatiquement.
• Copier/coller le save() de NewEventForm pour que les récurrences soient créées à la duplication (actuellement elles le sont au save() de la vue d'édition qui suit, mais on est pas obligé de cliquer sur enregistrer sur cette page).

J'ai rajouté une condition pour masquer le bouton dans ce cas, une exclusion pure et simple des évènements liés au niveau de la vue, je pense que ça devrait le faire. (Et le code dans le save du form pour créer les occurences aussi)

Agate Berriot a écrit :

Il manque le bout de test :)

C'est la dernière ligne du test test_duplicate_event: assert 'Event successfully duplicated.' in str(resp.body)

Vu, je cherchais ça juste après le resp = resp.form.submit().follow().

• Dans le breadcrumb on peut ajouter une ligne <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a>
  • Tu peux aussi essayer d'utiliser le template assez minimal et générique manager_agenda_form.html pour ne pas t'embêter

J'ai changé de template de base comme suggéré

Mmmh je ne vois pas la modif dans le patch ?

Effectivement, je pensais l'avoir fait, ça devrait être bon

Je me suis mal exprimé, je voulais dire soit ajouter la ligne soit génériciser un peu plus manager_agenda_form et l'utiliser directement. Ici hériter d'un template pour une ligne ne me paraît pas justifié, donc ma préférence irait à l'ajout explicite de <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a> en héritant de manager_agenda_view comme avant.

J'ai rajouté une condition pour masquer le bouton dans ce cas

Nickel.

une exclusion pure et simple des évènements liés au niveau de la vue

Une petite ligne pour tester ça genre assert app.get(f'/manage/agendas/{agenda.pk}/events/{event.pk}/duplicate', status=404) ? (pas sûr de la forme)

Et le code dans le save du form pour créer les occurences aussi

Pareil, couvrir ce if ce serait bien ! (on est un peu fiers de la couverture de code de chrono, pour ce fichier de 1500 lignes c'est moins de 10 lignes qui ne sont pas couvertes https://jenkins.entrouvert.org/job/chrono-wip/job/wip%252F67292-Pouvoir-dupliquer-un-evenement/6/Coverage_20Report_20_28native_29/)

Dernier truc, pour faire comme partout il faudrait remplacer primary_event=None par primary_event__isnull=True et str(resp.body) par resp.text.

Valentin Deniaud a écrit :

Je me suis mal exprimé, je voulais dire soit ajouter la ligne soit génériciser un peu plus manager_agenda_form et l'utiliser directement. Ici hériter d'un template pour une ligne ne me paraît pas justifié, donc ma préférence irait à l'ajout explicite de <a href="{{ agenda.get_settings_url }}">{% trans 'Settings' %}</a> en héritant de manager_agenda_view comme avant.

Fait.

Une petite ligne pour tester ça genre assert app.get(f'/manage/agendas/{agenda.pk}/events/{event.pk}/duplicate', status=404) ? (pas sûr de la forme)

Ah oui, justement, je n'étais pas sûre de comment tester ça !

Pareil, couvrir ce if ce serait bien ! (on est un peu fiers de la couverture de code de chrono, pour ce fichier de 1500 lignes c'est moins de 10 lignes qui ne sont pas couvertes https://jenkins.entrouvert.org/job/chrono-wip/job/wip%252F67292-Pouvoir-dupliquer-un-evenement/6/Coverage_20Report_20_28native_29/)

J'ai rajouté un test dédié pour ça

Dernier truc, pour faire comme partout il faudrait remplacer primary_event=None par primary_event__isnull=True et str(resp.body) par resp.text.

Fait :)

Automatic expiration