Produits Entr'ouvert » Chrono

idée : les exceptions pourraient venir d'un calendrier type iCalendar/ics qui, typiquement, présenterait les moments où la personne est déjà occupée. Idéalement un fichier ics accessible via URL et mis à jour automatiquement ; mais dans un premier temps plutôt un dépot local.

Pour le moment ça va plutôt être un modèle réfléchi pour chrono et alimenté via un formulaire web dans chrono.

Et si jamais une exception est posée alors qu'il y a des rendez-vous pris sur la période, il faut les notifier (pour qu'un trigger wcs puisse faire le nécessaire).

En terme d'UI, il s'agirait d'une action "Nouvelle exclusion" au niveau de l'agenda, une boite de dialogue où choisir une date, une heure de début et une heure de fin (alternativement une date/heure de début et une date/heure de fin, pour gérer en un coup une semaine de congés).

Ensuite, lorsque la liste des créneaux possibles est générée (api/views/MeetingDatetimes), retirer ce qui doit l'être.

Et comme noté juste au-dessus, si jamais une exception est posée alors qu'il y a des rendez-vous pris sur la période, il faut les notifier (pour qu'un trigger wcs puisse faire le nécessaire).

Je dirais de commencer alors par un truc totalement indépendant de w.c.s., qui simplement refuserait les exceptions sur les espaces déjà occupés. (mais ça veut dire que dans le patch, il ne doit pas y avoir de référence à w.c.s.).

Déjà pointé, il y a plein d'avantages à faire une chose et puis l'autre; ici j'ai souvenir d'un message de Serghei disant que la gestion des exceptions devrait se faire par guichet. (si ça se confirme, on peut dire que ce ne serait pas ici mais faut alors créer un autre ticket pour que ça soit fait).

            if BookingException.objects.filter(
                    start_datetime__lte=slot.start_datetime,
                    end_datetime__gt=slot.start_datetime):

Au moins les exceptions doivent être liées à agenda, elles le sont du côté du modèle mais ce n'est pas exploité, et plouf.

Autre plouf, au moment de la prise de rendez-vous (fillslot), il faut prendre les exceptions en compte.

@@ -28,6 +29,7 @@ from django.utils.translation import ugettext_lazy as _

 from jsonfield import JSONField

+
 AGENDA_KINDS = (
     ('events', _('Events')),
     ('meetings', _('Meetings')),

Please, je ne veux pas relire des patchs qui ajoutent des espaces sans rapport avec l'histoire.

+KNOWN_SERVICES = {
+    'wcs': {
+        'default': {'title': 'test', 'url': 'http://wcs.example.net/',
+                    'secret': 'combo', 'orig': 'combo', 'secondary': False,
+                    'backoffice-menu-url': 'http://wcs.example.net/backoffice/'}
+    }
+}

Plus de rapport avec ce ticket non plus.

Plutôt TimePeriodException.

Couverture du code par les tests unitaires. (ok, j'avais zappé le test_manager.py, qui ne s'était pas appliqué correctement sur origin/master).

@@ -162,6 +163,15 @@ class MeetingDatetimes(GenericAPIView):
                 start_datetime__lte=max_datetime + datetime.timedelta(meeting_type.duration))
         busy_time_slots = list(busy_time_slots)

+        # Search for excluded time slots
+        time_slots_list = []
+        for slot in all_time_slots:
+            if agenda.bookingexception_set.filter(start_datetime__lte=slot.start_datetime,
+                                                  end_datetime__gt=slot.start_datetime):

J'ai récemment travaillé à optimiser les perfs de cet appel, on peut très facilement se trouver avec des centaines de créneaux, et ce code donnera autant d''appels à la db en plus.

Si le plan est désormais d'attaquer directement sur #18111, le noter ici.

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

Si le plan est désormais d'attaquer directement sur #18111, le noter ici.

Je confirme que je vais pour les exceptions par guichet.
(J'ai poussé la branche parce que Serghei avait besoin du model d'exception)

Ok j'ai plutôt fermé l'autre vu que l'historique est ici.

Pourquoi le retour d'un manuel has_overlap après avoir introduit une dépendance sur intervaltree ? Proche, pourquoi ne pas gérer les exceptions lors du get_open_slots ?

Patch avec test_manager.py qui doit échouer.

Ça va avec le reste, genre ça échoue parce que tu as laissé un test d'une UI qui n'est plus présente parce que tu as des question, où c'est autre chose ?

Je n'ai pas ajouté l'affichage des exceptions sur manager_agenda_view à cause du changement fait dans #18414.

Dans #18414 il y a différentes choses, je ne vois pas précisément la modif là-dedans qui te perturbe ici.

Est ce que l'on reste sur l'idée d'ajouter les exceptions juste en dessous des time periods ? Si oui comment on les distingue l'un de l'autre ?

Cela étant, plus généralement, les commentaires précédents dans ce ticket, quand il est question d'interface, c'était avant la gestion multi-guichet, dans son intégralité, et donc oui, il faut réfléchir à ce point.

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

Et donc ma proposition côté interface, cf captures attachées :

• à partir du moment où il y a une période horaire, inclure un lien "Ajouter une exception" sous la liste.
• quand des exceptions sont présentes,
  • afficher celles des deux semaines à venir (ou la suivante peu importe sa date si aucune dans les semaines à venir)
  • inclure un lien permettant d'obtenir l'ensemble des exceptions (futures) (là j'imagine l'ouverture d'une popup qui présentera cette liste, avec comme seule action la possibilité de supprimer une exception)
• à noter l'ajout de la possibilité d'un libellé aux exceptions

Je fais le patch qui correspond

Je laisse par contre de côté pour le moment quelque chose qui me semblera important pour la suite, une gestion plus globale des exceptions, pour ne pas devoir déclarer les jours fériés une fois par guichet.

Et ouverture pour le ticket #16798, le choix d'un fichier ics se ferait dans la même boite de dialogue "Ajouter une exception".

def has_overlap(x, y):

J'avais fait un commentaire à ce sujet.

    label = models.CharField(_('Label'), max_length=150)

« possibilité d'un libellé aux exceptions » → optionnel.

    import pdb; pdb.set_trace()

passera pas.

Sur l'UI; cf les captures :

• les exceptions sont dans la même liste.
• je notais « ou la suivante peu importe sa date si aucune dans les semaines à venir » et ce n'est pas le cas.
• c'est le lien "(voir toutes les exceptions)" qui permet de voir toutes les exceptions (pas un clic sur "Exceptions"),
  • et ce lien est en italique.
  • dans la situation où toutes les exceptions sont déjà affichées, il n'est pas utile, ne doit pas être affiché.
• quand il y a un libellé, bien sûr qu'il faut un espace entre le libellé et la parenthèse avec les dates.
  • quand le début ou la fin n'est pas minuit, il faut également afficher les heures.

Comme noté dans le commentaire au-dessus, le libellé doit être optionnel, doit du coup arriver après start_datetime et end_datetime, et l'ui pourrait afficher "Optional Label", que ça soit bien clair.

Patch avec les modifications correspondantes au mockup.

Ce serait sans doute plus facile si tu précisais d'emblée ce en quoi tu as décidé de dévier, plutôt qu'annoncer une conformité.

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

Sur l'UI; cf les captures :

• les exceptions sont dans la même liste.
• je notais « ou la suivante peu importe sa date si aucune dans les semaines à venir » et ce n'est pas le cas.
• c'est le lien "(voir toutes les exceptions)" qui permet de voir toutes les exceptions (pas un clic sur "Exceptions"),
  • et ce lien est en italique.
  • dans la situation où toutes les exceptions sont déjà affichées, il n'est pas utile, ne doit pas être affiché.
• quand il y a un libellé, bien sûr qu'il faut un espace entre le libellé et la parenthèse avec les dates.
  • quand le début ou la fin n'est pas minuit, il faut également afficher les heures.

Comme noté dans le commentaire au-dessus, le libellé doit être optionnel, doit du coup arriver après start_datetime et end_datetime, et l'ui pourrait afficher "Optional Label", que ça soit bien clair.

Patch avec les modifications correspondantes au mockup.

Ce serait sans doute plus facile si tu précisais d'emblée ce en quoi tu as décidé de dévier, plutôt qu'annoncer une conformité.

Ok. Je refais le patch.

Précise ce qui change dans le fichier, stp.

En attendant, le commentaire qui était en cours d'écriture :

Essayer de garder les import triés; regarder la couverture du code par les tests unitaires et constater qu'il manque un bout.

Ce serait sans doute plus facile si tu précisais d'emblée ce en quoi tu as décidé de dévier, plutôt qu'annoncer une conformité.

Toujours le cas, toujours à moi d'être attentif sur les commentaires passés et de noter que "et ce lien est en italique" a été zappé.

+            'label': forms.TextInput(attrs={'placeholder': _('Optional Label')})

Pas de placeholder, jamais.

+        exc_repr = u'(%s → %s)' % (DateFormat(self.start_datetime).format(date_format),
+                                   DateFormat(self.end_datetime).format(date_format))
+        if self.label:
+            exc_repr = '%s %s' % (self.label, exc_repr)

Quand il n'y a pas de libellé on se trouve avec du texte entre parenthèses uniquement, ce n'est pas souhaitable.

+        if not exceptions:
+            return []
+        return list(exceptions)

Le if not exception: return [] apporte quelque chose que je ne capte pas ?

+    all_time_slots = reduce(operator.__or__, deepcopy(open_slots_by_desk).values())

En dix mots, ça fait quoi ? Plutôt que me répondre le mettre en commentaire au-dessus de cette ligne. (ou écrire ça d'une manière plus lisible).

+{% endblock %}
+

Sur une fin de fichier, commentaire obligatoire sur une ligne blanche malheureuse.

+        url(r'^time-period-exceptions/(?P<pk>\w+)/exception-list$', views.time_period_exception_list,
+            name='chrono-manager-time-period-exception-list'),
+
+
         url(r'^agendas/events.csv$', views.agenda_import_events_sample_csv,
             name='chrono-manager-sample-events-csv'),

Sur un milieu de fichier, commentaire obligatoire sur une ligne blanche malheureuse.

J'ai glissé une erreur lors de l'ajout des exceptions aussi. Je corrige avec en prenant en compte les remarques dessus.

La gestion des timezones a un problème j'ai ajouté une exception de 8h à 18h et ça a été enregistré comme étant de 8 à 18 UTC, et le résultat c'est que l'horaire normal, qui affiche "8h → 18h", et bien les deux premières heures sont restées disponibles.

Essayer de garder les import triés; regarder la couverture du code par les tests unitaires et constater qu'il manque un bout.

Tu as décidé que non ?

Le if not exception: return [] apporte quelque chose que je ne capte pas ?

Gestion du cas ou aucune exception n'existe à partir de la date d'aujourd'hui.

Mais le return list(query set qui retourne rien), qui est juste derrière, il fera pareil, retournera une liste vide.

Mais je note que le code est désormais différent,

        exceptions = self.timeperiodexception_set.filter(
            start_datetime__gte=now()).order_by('start_datetime').first()
        if exceptions:
            return [exceptions]

Pareil qu'en d'autres occasions, soudainement "exceptions" qui était avant utilisé pour tenir un queryset avec plusieurs éléments se met avec le même nom à contenir un élément unique, ça n'aide vraiment pas la lecture d'avoir comme ça la nature d'une variable changer.

La gestion des timezones a un problème j'ai ajouté une exception de 8h à 18h et ça a été enregistré comme étant de 8 à 18 UTC, et le résultat c'est que l'horaire normal, qui affiche "8h → 18h", et bien les deux premières heures sont restées disponibles.

Comme il n'y a aucune indication et que je ne vois pas différence dans les fichiers sous /manager/ qui seraient en lien, je me dis que ce commentaire a été zappé ?

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

Essayer de garder les import triés; regarder la couverture du code par les tests unitaires et constater qu'il manque un bout.

Tu as décidé que non ?

J'ai amelioré la couverture de tests et couvert tout ce qu'il y'avait dans agendas.models. C'est bien dans ce fichier que tout le code n'était pas couvert.
Pour les imports, encore dans ce fichier, je les ai remis par ordre alphabetique. Si je ne me trompe pas, c'était le django.utils.dateformat qui n'était pas à la bonne position au niveau des imports.

Le if not exception: return [] apporte quelque chose que je ne capte pas ?

Gestion du cas ou aucune exception n'existe à partir de la date d'aujourd'hui.

Mais le return list(query set qui retourne rien), qui est juste derrière, il fera pareil, retournera une liste vide.

Dans une de tes notes, tu demandais à ce que les exceptions dans les 2 semaines avenir soient affichées, dans le cas ou celle ci n'existent, afficher la prochaine exception dans le futur. La liste pour gère le cas ou il n'y a aucune exceptions dans le futur à partir de la date d'aujourd'hui, dans la mesure ou l'on n'affiche pas celles dans le passé.

Mais je note que le code est désormais différent,

[...]

Pareil qu'en d'autres occasions, soudainement "exceptions" qui était avant utilisé pour tenir un queryset avec plusieurs éléments se met avec le même nom à contenir un élément unique, ça n'aide vraiment pas la lecture d'avoir comme ça la nature d'une variable changer.

La gestion des timezones a un problème j'ai ajouté une exception de 8h à 18h et ça a été enregistré comme étant de 8 à 18 UTC, et le résultat c'est que l'horaire normal, qui affiche "8h → 18h", et bien les deux premières heures sont restées disponibles.

Comme il n'y a aucune indication et que je ne vois pas différence dans les fichiers sous /manager/ qui seraient en lien, je me dis que ce commentaire a été zappé ?

Je ne comprends pas. Est ce au niveau de l'affichage ? En local quand j'ai les bon affichage et au niveau de l'api les bons timeslots filtrés. Je rate surement quelque chose parce que je n'arrive pas à reproduire.

Toujours le cas, toujours à moi d'être attentif sur les commentaires passés et de noter que "et ce lien est en italique" a été zappé.

Corrigé

C'est le titre "Exceptions" qui a été passé en italique, pas le lien "(see all exceptions)".

cf https://dev.entrouvert.org/attachments/download/18878/Screenshot-2017-9-5%20Chrono(2).png

(la référence au ticket dans le commit n'est pas correcte)

À lire le début j'ai l'impression que open_slots ne change pas de nature.

     open_slots = reduce(operator.__or__, open_slots_by_desk.values())
-    return open_slots
+    return open_slots, all_time_slots

pourtant plus loin il y a ce changement :

-            open_slots = get_open_slots(agenda, MeetingType.objects.get(id=meeting_type_id))
+            open_slots, _ = get_all_slots(agenda, MeetingType.objects.get(id=meeting_type_id))
             slot = open_slots[event.start_datetime:event.end_datetime]
             if slot:
-                available_desk = slot.pop().data
+                available_desk = slot.pop().data.desk

Je ne vois pas ce qui est à l'origine de ce changement.

+                        <a href="#"><strong>{% trans 'Exceptions' %}</strong></a>

Pas un lien pas de <a>. (et oui ça veut sans doute dire gérer le padding d'un <strong> tapé là direct.) (bon, je peux oublier)

+                    {% if not desk.are_all_exceptions_displayed %}
+                    <li><a rel="popup" href="{% url 'chrono-manager-time-period-exception-list' pk=desk.id %}"><i>({% trans 'see all

L'italique ici aurait plutôt dû être amené par une classe sur le lien; j'oublie.

+{% block content %}
+<div class="timeperiod">
+    <form>

J'imagine le <form> présent ici pour le bénéfice de la popup, il faut lire gadjo.js et utiliser data-selector="..." pour plutôt préciser un sélecteur différent.

+    # exception within hours
+    TimePeriodException.objects.create(
+        desk=desk, start_datetime=datetime.datetime(2017, 5, 22, 9, 00),
+        end_datetime=datetime.datetime(2017, 5, 22, 12, 00))
+    resp2 = app.get('/api/agenda/meetings/%s/datetimes/' % meeting_type.id)
+    assert len(resp.json['data']) == len(resp2.json['data']) + 4

Déjà noté en d'autres occasions, il est pas mal utile de tester les bornes, basiquement ici, tester une exception qui commence au moment de la tranche horaire (qui est ici 10h/12h), assurer qu'elle ne laisse pas passer le créneau de 10h, tester une autre exception qui termine à 11h, assurer qu'elle ne "mange" pas aussi la tranche 11h/11h30.

+from django.core.management import call_command

Ajout de cet import mais il n'est pas utilisé.

Dans test_data_migrations.test_time_period_data_migration j'ai enlevé la fixture transactional_db parce qu'avec cette fixture ce test supprimait la migration 0019 relative au model TimePeriodException.

Quel était son rôle avant ? Tant qu'à toucher cette ligne, y corriger le nom du test ? (priod → period)

-    resp = resp.click('Add')
+    resp = resp.click(href="/manage/agendas/%d/desk/%d/add-time-period" % (agenda.pk, desk.pk), index=0)

Si vraiment il faut passer par préciser une URL plutôt qu'un texte de lien, utiliser reverse(). (on ne garantit pas d'URL stable ici).

+def test_meetings_agenda_delete_time_period_exception(app, admin_user):

Particulièrement celui-ci, faut le faire respirer un peu, mettre des commentaires pour décrire le déroulé. Alternative raisonnable ici, créer de manière rapide le contexte d'exécution du test en créant les objets nécessaires, et faire les manipulations d'UI uniquement pour la partie concernée par le test ("delete time period exception").

+    label = models.CharField(_('Label'), max_length=150, blank=True, null=True, help_text=_('Optional Label'))

Très sérieusement, ça fait un formulaire avec un champ "Libellé" suivi d'une description "Libellé optionnel"; gagnons du temps et de l'espace en mettant directement "Libellé optionnel" comme nom du champ.

Quel était son rôle avant ?

Mystère, passons.

fields = ['desk', 'start_datetime', 'end_datetime', 'label']
...
exclude = []

Si fields est renseigné, exclude n'a pas besoin de l'être.

Question subsidiaire, trouve toi-même deux tickets à créer sur des détails à améliorer.

Pas un lien pas de <a>. (et oui ça veut sans doute dire gérer le padding d'un <strong> tapé là direct.) (bon, je peux oublier)

J'ai ajouté de la css pour

Mais ça a décalé les noms des guichet. (vallait mieux oublier comme je le faisais)

J'ai supprimé le bloque définissant le padding de la css

Soupir. Sans remettre le <a> et donc "Exceptions" se trouve collé au bord. Quand je notais "je peux oublier" c'était une suggestion de ne rien faire, parce que j'imaginais trop bien comment ça allait repartir pour trois tours.

J'ai voulu tester une dernière fois et d'un mauvais clic j'ai oublié d'entrer une date de fin et ça a explosé avec TypeError at /manage/agendas/82/desk/1/add-time-period-exception / can't compare datetime.datetime to NoneType.

J'imagine le <form> présent ici pour le bénéfice de la popup, il faut lire gadjo.js et utiliser data-selector="..." pour plutôt préciser un sélecteur différent.

ok. data-selector="div.timeperiod" est maintenant utilisé.

Mais le <form> est resté :/ (je fatigue à devoir être continuellement attentif aux mêmes points)

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

Mais le <form> est resté :/ (je fatigue à devoir être continuellement attentif aux mêmes points)

gadjo cherche un form en dessous du selector défini (http://git.entrouvert.org/gadjo.git/tree/gadjo/static/js/gadjo.js#n141-n148), enfin c'est ce que j'ai compris de ce bout de code.

         * The dialog content is extracted from "form" (this selector can be
         * changed with a @data-selector attribute).

Si ça ne fonctionne pas, ça se corrige en commençant par un ticket dans gadjo.

J'ai voulu tester une dernière fois et d'un mauvais clic j'ai oublié d'entrer une date de fin et ça a explosé avec TypeError at /manage/agendas/82/desk/1/add-time-period-exception / can't compare datetime.datetime to NoneType.

J'ai corrigé en ajoutant un control clean_end_datetime au niveau du formulaire

J'ai supprimé la balise form de la page listant les exceptions (en attendant le patch gadjo).

J'avais posé une exception lors d'un test passé, qui couvrait encore aujourd'hui et demain et plus tard, et alors qu'elle est d'application elle n'apparait pas (get_exceptions_within_two_weeks devrait plutôt se baser sur end_time pour la borne basse).

Pire, j'ai commencé à jouer à partir de cette situation et j'ai découvert un bug majeur :

 id | label |     start_datetime     |      end_datetime      | desk_id 
----+-------+------------------------+------------------------+---------
 12 |       | 2017-10-21 00:00:00+02 | 2017-10-30 00:00:00+02 |       1
 13 |       | 2017-10-23 00:00:00+02 | 2017-10-24 00:00:00+02 |       1

Ces deux-là étant présentes, on s'attendrait à l'équivalent d'une exception allant du 21 au 30 (la première), mais non, ça fait comme si l'exclusion était du 21 au 24.

Question subsidiaire, trouve toi-même deux tickets à créer sur des détails à améliorer.

Comme on est parti pour le weekend, je préviens déjà que je ne vais pas oublier ce point.

J'avais posé une exception lors d'un test passé, qui couvrait encore aujourd'hui et demain et plus tard, et alors qu'elle est d'application elle n'apparait pas (get_exceptions_within_two_weeks devrait plutôt se baser sur end_time pour la borne basse).

Corrigé

Mais une exception qui est en cours mais se termine le mois prochain n'apparait plus. (genre fermeture d'un guichet pendant l'été, je n'invente pas de truc fantaisiste).

Capture qui montre un gadjo pas à jour, tournée à l'eocamp. Je ne comprends pas comment vous continuez à vivre ainsi.

-<a href="{% url 'chrono-manager-agenda-view' pk=desk.agenda.id %}">{{desk.agenda.label}}</a>
+<a href="{% url 'chrono-manager-agenda-view' pk=object.id %}">{{desk.label}}</a>

Ça me semble aussi peu correct avant (lien vers l'agenda alors que ce lien existe déjà dans le fil d'ariane via chrono/manager_agenda_view.html) qu'après, lien vers l'agenda mais avec le nom du guichet comme libellé. Pour un lien qui n'apparait pas. Ce qu'il faut, c'est que le dernier élément du fil d'ariane pointe vers la page en cours (cf agenda_view, dont le dernier lien est l'agenda).

Aussi, quand je demandais deux trucs à améliorer, c'était pour avoir des idées d'évolutions, de détails à affiner, plus tard, pas des bugs à corriger aujourd'hui. (mais oublions pour l'instant)

+{% if object.id %}
+<a href="{% url 'chrono-manager-agenda-view' pk=object.agenda.pk %}">{{object.label}}</a>
+{% endif %}

Affichage d'un lien vers l'agenda qui répète celui posé dans manager_agenda_view.html.

+{% endblock %}
+
+

Ligne blanche gratuite.

+{% if timeperiodexception.id %}
+<a href="{% url 'chrono-manager-time-period-exception-edit' pk=timeperiodexception.pk %}">{{timeperiodexception.label|default:timeperiodexception.pk}}</a>

Libellé / identifiant numérique alors qu'il y a eu unicode pour avoir une représentation.

Ahlala, je suggérerais d'oublier, d'arrêter d'essayer, de reprendre sur l'état en commentaire 61.

Tu peux crier "Pour Brice" en poussant ça. Reste des améliorations que j'aimerais bien que tu imagines et notes dans de nouveaux tickets.