Project

General

Profile

Bug #57305

API de modification d'un événement

Added by Nicolas Roche 20 days ago. Updated 9 days ago.

Status:
Solution déployée
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
27 Sep 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

L'idée c'est de pouvoir modifier le nombre de places, changer le libellé ou les dates de début ou de fin par exemple.


Files

Associated revisions

Revision 9a227239 (diff)
Added by Nicolas Roche 11 days ago

agendas: factorize recurring event update code (#57305)

Revision 030a355e (diff)
Added by Nicolas Roche 11 days ago

api: add an endpoint to patch an event (#57305)

History

#2

Updated by Nicolas Roche 19 days ago

  • Assignee set to Nicolas Roche
#3

Updated by Nicolas Roche 19 days ago

Je réalise que j'ai ajouté un nouveau endpoint 'add-event' dans #47337
alors que j'aurais plutôt intérêt à regrouper les nouveaux verbes POST et PATCH sur le endpoint 'status' (qui ne fait qu'un GET actuellement).
Sauf que 'status' c'est pas trés explicite (j'aurais préféré 'event').
Est-ce que je dupplique le GET de 'status' dans un nouveau endpoint 'event' ?

(patch pour illustrer si ce n'est pas clair.)

#4

Updated by Frédéric Péters 19 days ago

Je ne sais pas ce que le patch est supposé illustrer. (je ne veux pas savoir)

Ça n'aurait aucun sens de faire l'ajout d'un événement sur un endpoint dont l'URL contient un identifiant d'événement.

Je comprends qu'il y a envie d'une URL "neutre" pour un événement, pour pouvoir y taper un PATCH pour le modifier.

Qu'en fait aujourd'hui on a :

  • '^agenda/(?P<agenda_identifier>[\w-]+)/status/(?P<event_identifier>[\w:-]+)/$'
  • '^agenda/(?P<agenda_identifier>[\w-]+)/bookings/(?P<event_identifier>[\w:-]+)/$'
  • '^agenda/(?P<agenda_identifier>[\w-]+)/check/(?P<event_identifier>[\w:-]+)/$'

et que ça serait tellement plus net si on avait ^agenda/(?P<agenda_identifier>[\w-]+)/event/(?P<event_identifier>[\w:-]+)/(status ou bookings ou check ou autre).

Qu'il ne soit rien fait de cela ici.

Qu'il soit créé un url(r'^agenda/(?P<agenda_identifier>[\w-]+)/event/(?P<event_identifier>[\w:-]+)/$') et que ça prenne un PATCH pour modifier l'événement en question.

Qu'il ne soit rien fait rien discuté d'autre.

Pas de GET pas de POST pas de modification dans les URL existants.

#5

Updated by Nicolas Roche 17 days ago

Merci Fred, remarque prise en compte.

Je propose 0001.

Et dans 0002 je propose également de factoriser le code du manager,
mais c'est pas très lisible donc peut-être de trop.

#6

Updated by Valentin Deniaud 12 days ago

Les parties # legacy access by sont à supprimer, on a pas à supporter l'ancien dans du nouveau (cf le get_object de EventBookings).

Je trouve que la validation est trop bien faite, il faudrait faire moins bien et plus simple : lever ValidationError comme c'est fait partout et ne pas chercher à collecter toutes les erreurs dans un dictionnaire pour toutes les rapporter en une fois.

Ensuite limiter la duplication de code de validation entre le formulaire et l'API me paraît effectivement nécessaire. Pour que 0002 soit plus propre tu pourrais essayer d'en faire un contextmanager qui serait une méthode de Event comme tes pre/post_update, et ça donnerait un truc genre

with self.instance.update_recurrences(self.changed_data, self.cleaned_data, self.protected_fields):
    super().save(*args, **kwargs)

(et niveau découpage de patches ça serait 0001 ajout de ce contextmanager et utilisation dans le formulaire et 0002 le reste)

Malgré ça il reste plein de code dupliqué mais je ne sais pas trop comment faire autrement :/ d'ailleurs tu as oublié de dupliquer la partie qui empêche de modifier certains champs d'une récurrence d'un évènement (le if self.instance.primary_event:).

#7

Updated by Nicolas Roche 12 days ago

Les parties # legacy access by sont à supprimer, on a pas à supporter l'ancien dans du nouveau (cf le get_object de EventBookings).

Fait, merci.

Je trouve que la validation est trop bien faite, il faudrait faire moins bien et plus simple : lever ValidationError comme c'est fait partout et ne pas chercher à collecter toutes les erreurs dans un dictionnaire pour toutes les rapporter en une fois.

ValidationError est levée par les serializers puis retranscrite en APIError dans les vues, pour avoir par exemple 'err=1'.
cf le test test_datetimes_api_user_external_id :

    assert resp.json['err'] == 1
    assert resp.json['err_desc'] == 'invalid payload'
    assert resp.json['errors']['user_external_id'] == [
        'user_external_id and exclude_user_external_id have different values'
    ]

Ici on ne peut pas détecter les erreurs champ par champ au niveau du serializer.
Ensuite, comme de toute façon il faut un dictionnaire, je me dis que ça ne coûte rien de retourner toutes les erreurs d'un coup (je trouve ça plus sympa quand il faut utiliser un WS).
Mais bon, dis-moi si tu veux que je sorte direct en erreur.

Ensuite limiter la duplication de code de validation entre le formulaire et l'API me paraît effectivement nécessaire. Pour que 0002 soit plus propre tu pourrais essayer d'en faire un contextmanager qui serait une méthode de Event comme tes pre/post_update, et ça donnerait un truc genre

with self.instance.update_recurrences(self.changed_data, self.cleaned_data, self.protected_fields):
super().save(*args, **kwargs)

Fait.

Malgré ça il reste plein de code dupliqué mais je ne sais pas trop comment faire autrement :/ d'ailleurs tu as oublié de dupliquer la partie qui empêche de modifier certains champs d'une récurrence d'un évènement (le if self.instance.primary_event:).

Fait, et j'ai ajouté ces 2 tests, merci :
- update unprotected fields of one of the event recurrencies
- try to update protected fields of one of the event recurrencies

#8

Updated by Valentin Deniaud 11 days ago

Mais bon, dis-moi si tu veux que je sorte direct en erreur.

Oui je trouve que ça fait quand même un code plus lisible et cohérent avec le reste, le gain d'avoir tout rapporté en un coup me paraît trop marginal.

0001, le nom more_protected_field n'est pas très explicite, ça pourrait s'appeler exclude_fields et contenir la liste des champs à ne pas mettre à jour, genre exclude_fields=protected_fields + ['recurrence_end_date', 'frequency'] au moment de l'appel.

0002,

if payload.get(field) and payload.get(field) != getattr(event, field):
    changed_data.append(field)

Si je veux retirer la date de publication d'un évènement, je vais envoyer publication_date: null, et alors publication_date ne se retrouvera pas dans changed_data alors que ça devrait. J'imagine que le premier test devrait être un « in ».

#10

Updated by Valentin Deniaud 11 days ago

Dernier round de pinaillage :

0001, ça ne sert à rien d'avoir les arguments optionnels dans la méthode, vu qu'on passe tout à chaque fois.

0002, maintenant qu'il n'y a plus qu'une erreur dans chaque APIError, autant avoir l'erreur dans err_desc directement (ie remplacer 'invalid payload' par l'erreur en question et ne plus utiliser le paramètre errors).

#12

Updated by Valentin Deniaud 11 days ago

  • Status changed from Solution proposée to Solution validée

Top, dernière chose les titres de commit
« factorize recurrent event » -> recurring
« add an enpoint » -> enDpoint

#13

Updated by Nicolas Roche 11 days ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 030a355ebf443a8d8fb3b6840343d0af4b4fa854
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Thu Sep 30 00:23:31 2021 +0200

    api: add an endpoint to patch an event (#57305)

commit 9a2272396b62b057c62caa1fe1ebc1fc51b96f1f
Author: Nicolas ROCHE <nroche@entrouvert.com>
Date:   Tue Oct 5 14:36:29 2021 +0200

    agendas: factorize recurring event update code (#57305)
#14

Updated by Frédéric Péters 9 days ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF