caldav: add caldav connector (#87227) #472
Loading…
Reference in New Issue
No description provided.
Delete Branch "wip/87227-create-caldav-connector"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
b5e97d1604
to95bdcfca7e
95bdcfca7e
to551e1e62b0
551e1e62b0
toe484faa87f
e484faa87f
to1ad8197c78
1ad8197c78
to4e5ad7ca30
4e5ad7ca30
to94374e444c
94374e444c
toa8148add6a
a8148add6a
to62891f8dcd
62891f8dcd
tobea0fac1f6
bea0fac1f6
todf4ac67d9a
df4ac67d9a
tob68b6f5a67
b68b6f5a67
to108dae6302
108dae6302
to89c9de400f
89c9de400f
to7bce47d85c
7bce47d85c
to79aa098a2e
WIP: caldav: add caldav connector (#87227)to caldav: add caldav connector (#87227)Beau boulot ! Quelques remarques tout de même sinon c'est pas drôle :)
@ -0,0 +102,4 @@
'description': _('The relative path of the calendar'),
'type': 'string',
'pattern': r'^[^/].*$',
'examples': 'USERNAME/calendar/EVENT-ID.ics',
Je vois l'ambition d'avoir un truc générique mais je pense qu'il faut en premier lieu nous simplifier la vie (après tout c'est la raison d'être de ce connecteur) : permettre d'appeler avec un paramètre calendar_name (dans lequel on passera le username) et un paramètre event_id, et construire le chemin dans le connecteur.
Le truc, c'est que si j'ai bien compris la RFC (c'est pas certain), il n'y a pas de restriction sur les URL des événements :
https://www.rfc-editor.org/rfc/rfc4791#section-5.3.2
Si je comprend bien, malgré la présence d'un UID, un évènement est identifié par son URL (d’ailleurs EGW n'arrive pas a renvoyer un évènement en fonction de son UID).
Ensuite, au final, l'utilisateur ne devrait pas avoir à construire lui même l'URL vers un évènement : l'info est renvoyé par le endpoint de création.
Mais au final oui, on peut très bien forger les URL dans le connecteur en fonction du username et de l'UID, mais ça en ferait un connecteur EGW et pas CalDAV.
Yep c'est bien ça l'idée, la seule conséquence sera de rendre les choses plus simples pour le seul usage imaginé de ce connecteur.
Ce qui ne l'empêche pas de continuer à l'appeler CalDAV : en vérité aucun connecteur n'est 100% générique, toute utilisation avec une nouvelle instance du logiciel cible entraîne des devs supplémentaires, qu'on gérera ici si le besoin se présente :)
Ca me va :)
@ -0,0 +138,4 @@
@property
def dav_client(self):
'''Instanciate a caldav.DAVCLient and return the instance'''
if self.__client is None:
Il me semble que le décorateur @cached_property fait tout ça à moindre frais
Bien vu ! Merci :)
Tu as rajouté le décorateur mais tu n'as pas enlevé la mise en cache manuelle de l'objet via
self.__client
, pour moi cached_property permet de le faire sauterNavré -_- ! Merci !
@ -0,0 +201,4 @@
self._process_event_properties(post_data)
ical = self.get_event(cal_path, event_path)
vevt = ical.icalendar_instance.walk('VEVENT')
Je trouverais mieux sans abréviation ici, juste nommer la variable
vevent
Fait
@ -0,0 +209,4 @@
# datetime.date are badly converted to text that fails to be converted
# to datetime.datetime... Using the add() method seems to solves the
# bug
# vevt.update(post_data)
Je pense qu'on peut faire sans ces commentaires
Je trouve que la tentation de faire un
vevent.update()
et de passer leno_create=True
un peu forte ;)Ca te vas si je les réduis à une seule ligne de commentaire ?
Il y a une tentation pour toi, mais pas pour les autres lecteurs de ce code à qui ces commentaires vont plutôt alourdir la lecture : ignorance is bliss :)
Mais une ligne c'est déjà mieux en effet, tu peux laisser comme ça
@ -0,0 +245,4 @@
Returns a string representing the path to the object relative to
self.dav_url
Notes: uses the internal caldav's class caldav.lib.url.URL to
Ça pourrait plutôt être un commentaire à côté de la ligne en question
url = str(obj.url.canonical()) # normalize URL
J'avais mis ce commentaire vu que j'étais pas sur de moi. La classe URL de caldav indique en commentaire :
En réalité, en testant et en y pensant, c'est un truc "au cas ou", plutôt inutile à priori...
@ -0,0 +249,4 @@
normalize URL using the canonical() method on them
'''
url = str(obj.url.canonical())
dav_url = str(caldav.lib.url.URL(self.dav_url).canonical())
Peut-être à faire une seule fois dans le
__init__
?@ -0,0 +291,4 @@
def _match_params(self, name, params):
'''Match query parameters againts regex pattern
NOTE: should be implemented by endpoint
Ici tu as constaté que la validation induite par la clé « pattern » dans le schéma json n'était pas appliquée ? Ça paraît improbable vu le nombre de connecteurs qui utilisent cette fonctionnalité non ?
Non pas les pattern dans le schéma JSON, dans la spécification des paramètres de la query string.
Ah oui, c'est le nom SCHEMA qui m'avait induit en erreur, ça fait penser à du JSON alors que non. Mais cette partie devrait disparaître si on passe direct nom du calendrier + id de l'événement
Exactement
@ -0,0 +321,4 @@
else:
cls = datetime.datetime
try:
data[dt_field] = cls.fromisoformat(value)
Les autres connecteurs utilisent la fonction dateparse de Django pour gérer ça, ce serait sûrement plus simple
J'avoue que je n'ai pas compris en quoi c'était plus simple. Mais si les autres connecteurs utilisent ces fonctions et que ça unifie le code c'est chouette.
C'est plus simple parce que tu peux te passer de la partie qui mesure la longueur de la chaîne reçue et appelle l'une ou l'autre méthode : le pattern plus classique serait de faire deux try/except, si parser la chaîne en tant que date ne marche pas, la parser en tant que datetime, sinon retourner une erreur
@ -0,0 +90,4 @@
@pytest.fixture()
def mocks():
Pour moi cette partie induit trop d'indirections, l'objectif pour un test est d'être lisible au premier coup d'œil, là mon œil a besoin de beaucoup de coups :)
Pour simplifier je propose de remplacer la fixture par une simple méthode, get_mocked_calendar(url=..., id=..., ...).
Ça me semble convenir pour remplacer
par
Bien vu ! Merci
@ -0,0 +132,4 @@
@responses.activate
def test_caldav_check_status_ok(app, caldav_conn):
url = tests.utils.generic_endpoint_url('caldav', 'check_status')
mock = responses.add( # pylint: disable=E1128
Il est possible d'indiquer le nom de l'erreur plutôt que son numéro : disable=assignment-from-none
Mais ici pylint a plutôt raison, vérifier responses.calls comme tu le fais déjà semble être la manière recommandée, pourquoi cette double vérification ?
Ben bof, visiblement
responses.add
ne renvoie pas None ;)Pour vérifier qu'il n'y a pas eu d'autres appels que celui prévu (et que l'appel unique qui a été fait était bien l'appel prévu). C'est pas la bonne manière de faire ?
À partir du moment où ce n'est pas documenté, alors que pour le coup la doc de responses est plutôt touffue, on peut dire que non (et par extension que ça risque de casser sans prévenir au gré des mises à jour de la lib)
À mon avis le test échoue si responses voit que la réponse que tu as ajouté n'a pas été utilisée, donc juste vérifier
len(responses.calls)
garantit bien ces deux pointsC'est dans la doc ;) https://github.com/getsentry/responses/blob/master/README.rst#assert-based-on-response-object
Alors oui mais non :P Le test va fail vu que caldav va se prendre une ConnectionRefused de requests, mais si des responses sont définit et ne sont jamais appelées ça ne fait pas fail le test.
@ -0,0 +136,4 @@
'PROPFIND', DAV_URL, headers={'Content-Type': 'text/xml'}, body=PROPFIND_REPLY
)
resp = app.get(url)
calls = responses.calls
On peut se passer de cette variable intermédiaire
@ -0,0 +185,4 @@
params = {'cal_path': cal_path}
with patch('caldav.Calendar', return_value=cal_mock):
resp = app.post(url + '?' + urllib.parse.urlencode(params), json.dumps(event))
Ça serait vraiment plus lisible d'avoir
et je pense qu'on peut même
Je suis passé par une petite fonction utilitaire, je trouve effectivement ça plus lisible, j'espère que toi aussi.
79aa098a2e
tode062ccd1e
de062ccd1e
to8620efd33c
Merci pour la prise en compte du premier train de remarques, en voici encore quelques unes
@ -0,0 +142,4 @@
name='check_status',
methods=['get'],
)
def check_status(self, request):
Cette méthode peut-être largement simplifiée j'ai l'impression : il n'y a pas besoin que ce soit un endpoint, et il n'y a même pas besoin de rattraper les exceptions (voir ce qu'il se passe dans BaseResource.availability()).
Donc
me semble faire le job !
Bien vu, merci :)
@ -0,0 +174,4 @@
self._process_event_properties(post_data)
evt = cal.save_event(**post_data)
ret = {'data': {'path': self.get_path(evt), 'uid': evt.id}}
Pas besoin de variable intermédiaire ici
@ -0,0 +227,4 @@
ical.delete()
return {}
def get_path(self, obj):
Cette méthode est juste utilisée pour les retours du connecteur, mais puisqu'on ne prend plus de chemin en entrée, je pense qu'on peut les retirer de la sortie également
ok :)
@ -0,0 +250,4 @@
event_path = self.get_event_path(username, event_uid)
cal = self.get_calendar(calendar_path)
try:
ical = cal.event_by_url(event_path)
Peut-être que la méthode cal.object_by_uid fonctionne et permet de ne pas avoir à construire le chemin vers l'évènement ?
Tristement non :/ Même la RFC explique qu'il faut s'attendre à ce que ça ne fonctionne pas ...
EDIT : Désolé, je retrouve plus la source... La seule trace c'est ce warning dans la doc de caldav : https://caldav.readthedocs.io/en/latest/caldav/objects.html#caldav.objects.CalendarObjectResource.save
Mais les tests que j'avais fais autour de object_by_uid (y compris en passant un comp_filter) n'ont rien donnés sur EGW :/
@ -0,0 +276,4 @@
calendar = caldav.Calendar(client=self.dav_client, url=path)
return calendar
def get_calendar_path(self, username):
On peut faire un sort à cette méthode en construisant l'URL au seul endroit où il y en a besoin il me semble, dans get_calendar
Et la même chose pour get_event_path du coup :)
8620efd33c
to2248d03d22
2248d03d22
to65bfdcf710
65bfdcf710
to25e4cc9aec
25e4cc9aec
toc51cee7d20
c51cee7d20
tob4edd015a5
J'ai testé et ça marche au top ! Dernières broutilles après je valide :)
@ -0,0 +126,4 @@
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.calendar_path_fmt = '%(username)s/calendar'
self.event_path_fmt = '%(username)s/calendar/%(uid)s.ics'
Ces attributs sont à déplacer au seul endroit de leur utilisation respective :)
Soit :P
@ -0,0 +140,4 @@
rep.find_objects_and_props()
except caldav.lib.error.AuthorizationError:
raise Exception(_('Not authorized: bad login/password ?'))
except Exception as ex:
Pas besoin de rattraper, c'est géré dans BaseResource.availability()
ok :)
@ -0,0 +146,4 @@
@endpoint(
name='event',
pattern='^create$',
example_pattern='event/create',
Sur la vue du connecteur ça indique comme url
/caldav/egw/event/event/create
alors que la bonne est juste/caldav/egw/event/create
En effet !
@ -0,0 +159,4 @@
self._process_event_properties(post_data)
evt = cal.save_event(**post_data)
return {'data': {'uid': evt.id}}
Idéalement il faudrait nommer cette clé event_id (un œil de CPF a de forte chances de glisser sur u de uid qui n'est pas une dénomination habituelle)
Je fais ça :)
J'avais peut être mal compris, mais il me semblait qu'il vallait mieux éviter les
_
dans les retour à wcs (vu que ce caractère est dédié a la séparation de "namespace").@ -0,0 +170,4 @@
post={'request_body': {'schema': {'application/json': EVENT_SCHEMA}}},
parameters={
'username': USERNAME_PARAM,
'event_uid': EVENT_UID_PARAM,
Pareil event_id ici serait moins étonnant
ok, c'est fait :)
@ -0,0 +135,4 @@
def app_post(app, url, params, data, expect_errors=False):
'''Allows to post data on an URL with query string params and JSON data'''
return app.post(qs_url(url, params), json.dumps(data), expect_errors=expect_errors)
Il me semble que plutôt que expect_errors on peut direct dire le statut d'erreur attendu, genre
status=400
fait la même chose queexpect_errors=True
en plus précisAussi utiliser post_json et passer
params=data
évite d'encoder data explicitement avec json.dumpsBien vu, merci ! Du coup ça a levé une série de tests qui n'était pas ouf. C'est corrigé.
b4edd015a5
to17ab6b6061
Des détails à corriger dans la déclaration des dépendances et ensuite let's go !
@ -1,3 +1,4 @@
caldav
Sur la ligne du dessous on lit django<1.12, ce fichier est visiblement inutilisé, mieux vaut ne pas y toucher
Merci ! C'est fait :)
@ -13,6 +13,7 @@ setenv =
fast: FAST=--nomigrations
coverage: COVERAGE=--junitxml=junit-{envname}.xml --cov-report xml --cov-report html --cov=passerelle/ --cov-config .coveragerc -Wignore
deps =
caldav
Cet ajout devrait être déplacé dans le setup.py
Merci ! C'est fait :)
17ab6b6061
to66a8c3a0d4
66a8c3a0d4
to7783727b0f
7783727b0f
to665c16bca2