caldav: add caldav connector (#87227) #472

Merged
yweber merged 1 commits from wip/87227-create-caldav-connector into main 2024-03-05 14:34:29 +01:00
Owner
No description provided.
yweber added 1 commit 2024-02-22 14:01:39 +01:00
gitea/passerelle/pipeline/head There was a failure building this commit Details
b5e97d1604
caldav: add event creation/update/deletion (#87227)
yweber force-pushed wip/87227-create-caldav-connector from b5e97d1604 to 95bdcfca7e 2024-02-22 14:23:15 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 95bdcfca7e to 551e1e62b0 2024-02-22 14:46:54 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 551e1e62b0 to e484faa87f 2024-02-22 15:38:35 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from e484faa87f to 1ad8197c78 2024-02-22 15:46:32 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 1ad8197c78 to 4e5ad7ca30 2024-02-22 15:49:55 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 4e5ad7ca30 to 94374e444c 2024-02-26 12:11:02 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 94374e444c to a8148add6a 2024-02-26 12:16:41 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from a8148add6a to 62891f8dcd 2024-02-26 13:43:27 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 62891f8dcd to bea0fac1f6 2024-02-26 14:26:20 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from bea0fac1f6 to df4ac67d9a 2024-02-26 14:28:37 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from df4ac67d9a to b68b6f5a67 2024-02-26 14:42:00 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from b68b6f5a67 to 108dae6302 2024-02-26 14:47:42 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 108dae6302 to 89c9de400f 2024-02-26 14:54:03 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 89c9de400f to 7bce47d85c 2024-02-26 15:07:58 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 7bce47d85c to 79aa098a2e 2024-02-27 10:50:54 +01:00 Compare
yweber changed title from WIP: caldav: add caldav connector (#87227) to caldav: add caldav connector (#87227) 2024-02-27 10:57:18 +01:00
vdeniaud requested changes 2024-02-27 16:24:18 +01:00
vdeniaud left a comment
Owner

Beau boulot ! Quelques remarques tout de même sinon c'est pas drôle :)

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',
Owner

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.

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.
Author
Owner

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

The URL for each calendar object resource is entirely arbitrary and does not need to bear a specific relationship to the calendar object resource's iCalendar properties or other metadata.

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.

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 > The URL for each calendar object resource is entirely arbitrary and does not need to bear a specific relationship to the calendar object resource's iCalendar properties or other metadata. 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.
Owner

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 :)

> 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 :)
Author
Owner

Ca me va :)

Ca me va :)
yweber marked this conversation as resolved
@ -0,0 +138,4 @@
@property
def dav_client(self):
'''Instanciate a caldav.DAVCLient and return the instance'''
if self.__client is None:
Owner

Il me semble que le décorateur @cached_property fait tout ça à moindre frais

Il me semble que le décorateur @cached_property fait tout ça à moindre frais
Author
Owner

Bien vu ! Merci :)

Bien vu ! Merci :)
Owner

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 sauter

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 sauter
Author
Owner

Navré -_- ! Merci !

Navré -_- ! Merci !
yweber marked this conversation as resolved
@ -0,0 +201,4 @@
self._process_event_properties(post_data)
ical = self.get_event(cal_path, event_path)
vevt = ical.icalendar_instance.walk('VEVENT')
Owner

Je trouverais mieux sans abréviation ici, juste nommer la variable vevent

Je trouverais mieux sans abréviation ici, juste nommer la variable `vevent`
Author
Owner

Fait

Fait
yweber marked this conversation as resolved
@ -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)
Owner

Je pense qu'on peut faire sans ces commentaires

Je pense qu'on peut faire sans ces commentaires
Author
Owner

Je trouve que la tentation de faire un vevent.update() et de passer le no_create=True un peu forte ;)

Ca te vas si je les réduis à une seule ligne de commentaire ?

Je trouve que la tentation de faire un `vevent.update()` et de passer le `no_create=True` un peu forte ;) Ca te vas si je les réduis à une seule ligne de commentaire ?
Owner

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

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
yweber marked this conversation as resolved
@ -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
Owner

Ça pourrait plutôt être un commentaire à côté de la ligne en question url = str(obj.url.canonical()) # normalize URL

Ça pourrait plutôt être un commentaire à côté de la ligne en question `url = str(obj.url.canonical()) # normalize URL`
Author
Owner

J'avais mis ce commentaire vu que j'étais pas sur de moi. La classe URL de caldav indique en commentaire :

It's used internally in the library, end users should not need to know anything about this class.

En réalité, en testant et en y pensant, c'est un truc "au cas ou", plutôt inutile à priori...

J'avais mis ce commentaire vu que j'étais pas sur de moi. La classe URL de caldav indique en commentaire : > It's used internally in the library, end users should not need to know anything about this class. En réalité, en testant et en y pensant, c'est un truc "au cas ou", plutôt inutile à priori...
yweber marked this conversation as resolved
@ -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())
Owner

Peut-être à faire une seule fois dans le __init__ ?

Peut-être à faire une seule fois dans le `__init__` ?
yweber marked this conversation as resolved
@ -0,0 +291,4 @@
def _match_params(self, name, params):
'''Match query parameters againts regex pattern
NOTE: should be implemented by endpoint
Owner

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 ?

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 ?
Author
Owner

Non pas les pattern dans le schéma JSON, dans la spécification des paramètres de la query string.

Non pas les pattern dans le schéma JSON, dans la spécification des paramètres de la query string.
Owner

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

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
Author
Owner

Exactement

Exactement
yweber marked this conversation as resolved
@ -0,0 +321,4 @@
else:
cls = datetime.datetime
try:
data[dt_field] = cls.fromisoformat(value)
Owner

Les autres connecteurs utilisent la fonction dateparse de Django pour gérer ça, ce serait sûrement plus simple

Les autres connecteurs utilisent la fonction dateparse de Django pour gérer ça, ce serait sûrement plus simple
Author
Owner

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.

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.
Owner

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

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
yweber marked this conversation as resolved
@ -0,0 +90,4 @@
@pytest.fixture()
def mocks():
Owner

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

    get_mock, conf = mocks
    conf['Event']['url'] = DAV_URL + cal_path
    conf['Event']['id'] = 42
    cal_mock = get_mock(conf)

par

calendar = get_mocked_calendar(url=DAV_URL + cal_path, id=42)
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 ``` get_mock, conf = mocks conf['Event']['url'] = DAV_URL + cal_path conf['Event']['id'] = 42 cal_mock = get_mock(conf) ``` par ``` calendar = get_mocked_calendar(url=DAV_URL + cal_path, id=42) ```
Author
Owner

Bien vu ! Merci

Bien vu ! Merci
yweber marked this conversation as resolved
@ -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
Owner

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 ?

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 ?
Author
Owner

Mais ici pylint a plutôt raison

Ben bof, visiblement responses.add ne renvoie pas None ;)

vérifier responses.calls comme tu le fais déjà semble être la manière recommandée, pourquoi cette double vérification ?

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 ?

> Mais ici pylint a plutôt raison Ben bof, visiblement `responses.add` ne renvoie pas None ;) > vérifier responses.calls comme tu le fais déjà semble être la manière recommandée, pourquoi cette double vérification ? 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 ?
Owner

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)

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)

À 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 points

> 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) > 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) À 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 points
Author
Owner

À 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)

C'est dans la doc ;) https://github.com/getsentry/responses/blob/master/README.rst#assert-based-on-response-object

À 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 points

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.

> À 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) C'est dans la doc ;) https://github.com/getsentry/responses/blob/master/README.rst#assert-based-on-response-object > À 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 points 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.
yweber marked this conversation as resolved
@ -0,0 +136,4 @@
'PROPFIND', DAV_URL, headers={'Content-Type': 'text/xml'}, body=PROPFIND_REPLY
)
resp = app.get(url)
calls = responses.calls
Owner

On peut se passer de cette variable intermédiaire

On peut se passer de cette variable intermédiaire
yweber marked this conversation as resolved
@ -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))
Owner

Ça serait vraiment plus lisible d'avoir

        resp = app.post(url + '?cal_path=foo/calendar', json.dumps(event)) 

et je pense qu'on peut même

        resp = app.post_json(url + '?cal_path=foo/calendar', params=event)
Ça serait vraiment plus lisible d'avoir ``` resp = app.post(url + '?cal_path=foo/calendar', json.dumps(event)) ``` et je pense qu'on peut même ``` resp = app.post_json(url + '?cal_path=foo/calendar', params=event) ```
Author
Owner

Je suis passé par une petite fonction utilitaire, je trouve effectivement ça plus lisible, j'espère que toi aussi.

Je suis passé par une petite fonction utilitaire, je trouve effectivement ça plus lisible, j'espère que toi aussi.
vdeniaud marked this conversation as resolved
yweber force-pushed wip/87227-create-caldav-connector from 79aa098a2e to de062ccd1e 2024-02-28 10:06:44 +01:00 Compare
yweber requested review from vdeniaud 2024-02-28 10:10:43 +01:00
yweber force-pushed wip/87227-create-caldav-connector from de062ccd1e to 8620efd33c 2024-02-28 12:19:11 +01:00 Compare
vdeniaud requested changes 2024-02-28 14:10:39 +01:00
vdeniaud left a comment
Owner

Merci pour la prise en compte du premier train de remarques, en voici encore quelques unes

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):
Owner

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

def check_status(self):
    rep = self.dav_client.propfind()
    rep.find_objects_and_props()

me semble faire le job !

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 ``` def check_status(self): rep = self.dav_client.propfind() rep.find_objects_and_props() ``` me semble faire le job !
Author
Owner

Bien vu, merci :)

Bien vu, merci :)
yweber marked this conversation as resolved
@ -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}}
Owner

Pas besoin de variable intermédiaire ici

Pas besoin de variable intermédiaire ici
yweber marked this conversation as resolved
@ -0,0 +227,4 @@
ical.delete()
return {}
def get_path(self, obj):
Owner

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

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
Author
Owner

ok :)

ok :)
yweber marked this conversation as resolved
@ -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)
Owner

Peut-être que la méthode cal.object_by_uid fonctionne et permet de ne pas avoir à construire le chemin vers l'évènement ?

Peut-être que la méthode cal.object_by_uid fonctionne et permet de ne pas avoir à construire le chemin vers l'évènement ?
Author
Owner

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

Some servers don’t support searching for an object uid without explicitly specifying what kind of object it should be, hence obj_type can be passed.

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 :/

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 > Some servers don’t support searching for an object uid without explicitly specifying what kind of object it should be, hence obj_type can be passed. 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 :/
vdeniaud marked this conversation as resolved
@ -0,0 +276,4 @@
calendar = caldav.Calendar(client=self.dav_client, url=path)
return calendar
def get_calendar_path(self, username):
Owner

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

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
Author
Owner

Et la même chose pour get_event_path du coup :)

Et la même chose pour get_event_path du coup :)
yweber marked this conversation as resolved
yweber force-pushed wip/87227-create-caldav-connector from 8620efd33c to 2248d03d22 2024-02-28 15:13:18 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 2248d03d22 to 65bfdcf710 2024-02-28 15:57:45 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 65bfdcf710 to 25e4cc9aec 2024-02-28 16:01:06 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 25e4cc9aec to c51cee7d20 2024-02-28 16:02:57 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from c51cee7d20 to b4edd015a5 2024-02-28 16:06:46 +01:00 Compare
yweber requested review from vdeniaud 2024-02-28 16:36:18 +01:00
vdeniaud requested changes 2024-02-28 17:14:48 +01:00
vdeniaud left a comment
Owner

J'ai testé et ça marche au top ! Dernières broutilles après je valide :)

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'
Owner

Ces attributs sont à déplacer au seul endroit de leur utilisation respective :)

Ces attributs sont à déplacer au seul endroit de leur utilisation respective :)
Author
Owner

Soit :P

Soit :P
yweber marked this conversation as resolved
@ -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:
Owner

Pas besoin de rattraper, c'est géré dans BaseResource.availability()

Pas besoin de rattraper, c'est géré dans BaseResource.availability()
Author
Owner

ok :)

ok :)
yweber marked this conversation as resolved
@ -0,0 +146,4 @@
@endpoint(
name='event',
pattern='^create$',
example_pattern='event/create',
Owner

Sur la vue du connecteur ça indique comme url /caldav/egw/event/event/create alors que la bonne est juste /caldav/egw/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`
Author
Owner

En effet !

En effet !
yweber marked this conversation as resolved
@ -0,0 +159,4 @@
self._process_event_properties(post_data)
evt = cal.save_event(**post_data)
return {'data': {'uid': evt.id}}
Owner

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)

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)
Author
Owner

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").

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").
vdeniaud marked this conversation as resolved
@ -0,0 +170,4 @@
post={'request_body': {'schema': {'application/json': EVENT_SCHEMA}}},
parameters={
'username': USERNAME_PARAM,
'event_uid': EVENT_UID_PARAM,
Owner

Pareil event_id ici serait moins étonnant

Pareil event_id ici serait moins étonnant
Author
Owner

ok, c'est fait :)

ok, c'est fait :)
yweber marked this conversation as resolved
@ -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)
Owner

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 que expect_errors=True en plus précis

Aussi utiliser post_json et passer params=data évite d'encoder data explicitement avec json.dumps

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 que `expect_errors=True` en plus précis Aussi utiliser post_json et passer `params=data` évite d'encoder data explicitement avec json.dumps
Author
Owner

Bien vu, merci ! Du coup ça a levé une série de tests qui n'était pas ouf. C'est corrigé.

Bien vu, merci ! Du coup ça a levé une série de tests qui n'était pas ouf. C'est corrigé.
yweber marked this conversation as resolved
yweber force-pushed wip/87227-create-caldav-connector from b4edd015a5 to 17ab6b6061 2024-02-28 19:40:08 +01:00 Compare
yweber requested review from vdeniaud 2024-02-28 19:44:12 +01:00
vdeniaud approved these changes 2024-02-29 14:29:58 +01:00
vdeniaud left a comment
Owner

Des détails à corriger dans la déclaration des dépendances et ensuite let's go !

Des détails à corriger dans la déclaration des dépendances et ensuite let's go !
requirements.txt Outdated
@ -1,3 +1,4 @@
caldav
Owner

Sur la ligne du dessous on lit django<1.12, ce fichier est visiblement inutilisé, mieux vaut ne pas y toucher

Sur la ligne du dessous on lit django<1.12, ce fichier est visiblement inutilisé, mieux vaut ne pas y toucher
Author
Owner

Merci ! C'est fait :)

Merci ! C'est fait :)
yweber marked this conversation as resolved
tox.ini Outdated
@ -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
Owner

Cet ajout devrait être déplacé dans le setup.py

Cet ajout devrait être déplacé dans le setup.py
Author
Owner

Merci ! C'est fait :)

Merci ! C'est fait :)
yweber marked this conversation as resolved
yweber force-pushed wip/87227-create-caldav-connector from 17ab6b6061 to 66a8c3a0d4 2024-02-29 14:34:17 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 66a8c3a0d4 to 7783727b0f 2024-02-29 16:42:00 +01:00 Compare
yweber force-pushed wip/87227-create-caldav-connector from 7783727b0f to 665c16bca2 2024-03-05 14:30:22 +01:00 Compare
yweber merged commit 665c16bca2 into main 2024-03-05 14:34:29 +01:00
yweber deleted branch wip/87227-create-caldav-connector 2024-03-05 14:34:29 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/passerelle#472
No description provided.