entrouvert
/
passerelle
15
0
Fork 0
Code Issues Pull Requests 15 Releases Activity

caldav: add SEQUENCE field when missing on event update (#88417) #500

Merged
yweber merged 1 commits from wip/88417-caldav-SEQUENCE-handling into main 2024-03-21 16:41:57 +01:00
Conversation 0 Commits 1 Files Changed 2 +66 -11
yweber commented 2024-03-20 18:22:26 +01:00
Owner
Copy Link
No description provided.
yweber added 3 commits 2024-03-20 18:22:27 +01:00
gitea/passerelle/pipeline/head This commit looks good Details
fd7a2d487d caldav: add rrule/until convertion to date (#88393)
yweber changed title from WIP: wip/88417-caldav-SEQUENCE-handling to WIP: caldav: add SEQUENCE field when missing on event update (#88417) 2024-03-20 18:23:11 +01:00
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 20a214bbec to 96a1caf4aa 2024-03-21 10:24:22 +01:00 Compare
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 96a1caf4aa to cc19f8db39 2024-03-21 10:24:51 +01:00 Compare
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from cc19f8db39 to 6fc0551d86 2024-03-21 10:27:12 +01:00 Compare
yweber changed title from WIP: caldav: add SEQUENCE field when missing on event update (#88417) to caldav: add SEQUENCE field when missing on event update (#88417) 2024-03-21 10:32:29 +01:00
yweber changed title from caldav: add SEQUENCE field when missing on event update (#88417) to WIP: caldav: add SEQUENCE field when missing on event update (#88417) 2024-03-21 10:35:04 +01:00
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 6fc0551d86 to fd153460e3 2024-03-21 10:41:29 +01:00 Compare
yweber changed title from WIP: caldav: add SEQUENCE field when missing on event update (#88417) to caldav: add SEQUENCE field when missing on event update (#88417) 2024-03-21 10:45:41 +01:00
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from fd153460e3 to 4db7ad5694 2024-03-21 10:46:06 +01:00 Compare
vdeniaud reviewed 2024-03-21 12:21:25 +01:00
tests/test_caldav.py Outdated
@ -446,0 +451,4 @@
def test_ics(request):
calendar = icalendar.Calendar.from_ical(request.body.decode('utf-8'))
vevent = calendar.walk('VEVENT')[0]
assert vevent['SEQUENCE'] == expt_sequence
vdeniaud commented 2024-03-21 12:21:25 +01:00
Owner
Copy Link

Si le but ici c'est de vérifier que le connecteur envoie bien le bon numéro, pourquoi le test ne va pas plus haut dans test_caldav_event_update_ok qui il me semble contient la partie vérification de ce qui est envoyé ?

Si le but ici c'est de vérifier que le connecteur envoie bien le bon numéro, pourquoi le test ne va pas plus haut dans test_caldav_event_update_ok qui il me semble contient la partie vérification de ce qui est envoyé ?
yweber commented 2024-03-21 12:35:35 +01:00
Author
Owner
Copy Link

C'est pas exactement ça :

  • le test au dessus (test_caldav_event_update_ok) mock la lib caldav. On vérifie que la modification de l'objet icalendar fonctionne : on vérifie les arguments envoyés à caldav...save(), mais ça ne nous permet pas de vérifier ce que caldav envoie réelement
  • le test en question (test_caldav_event_update_ok_nomock) utilise responses pour récupérer l'ICS envoyé finalement par caldav (c'est donc ici qu'on peut vérifier si caldav incrémente bien automatiquement l'objet icalendar qu'on file en argument à caldav...save())

Et, en réalité, le test test_caldav_event_update_ok_nomock a été implémenté car, dans certain cas, les modifications effectués sur l'instance icalendar provoque une erreur dans la méthode caldav...save()

C'est pas exactement ça : - le test au dessus (`test_caldav_event_update_ok`) mock la lib caldav. On vérifie que la modification de l'objet `icalendar` fonctionne : on vérifie les arguments envoyés à `caldav...save()`, mais ça ne nous permet pas de vérifier ce que caldav envoie réelement - le test en question (`test_caldav_event_update_ok_nomock`) utilise `responses` pour récupérer l'ICS envoyé finalement par caldav (c'est donc ici qu'on peut vérifier si caldav incrémente bien automatiquement l'objet icalendar qu'on file en argument à `caldav...save()`) Et, en réalité, le test `test_caldav_event_update_ok_nomock` a été implémenté car, dans certain cas, les modifications effectués sur l'instance `icalendar` provoque une erreur dans la méthode `caldav...save()`
vdeniaud commented 2024-03-21 13:58:48 +01:00
Owner
Copy Link

Oui dac et dans notre cas l'incrémentation est fait par caldav. En tout cas je trouve cette manière de tester préférable aux mocks, sûrement qu'on pourrait nettoyer un peu tout ça, mais ça serait pour un autre ticket :)

Par contre toujours à propos de cette ligne, plutôt qu'un callback il faudrait effectuer la vérification à partir de responses.calls[1].request

Oui dac et dans notre cas l'incrémentation est fait par caldav. En tout cas je trouve cette manière de tester préférable aux mocks, sûrement qu'on pourrait nettoyer un peu tout ça, mais ça serait pour un autre ticket :) Par contre toujours à propos de cette ligne, plutôt qu'un callback il faudrait effectuer la vérification à partir de `responses.calls[1].request`
yweber commented 2024-03-21 14:20:08 +01:00
Author
Owner
Copy Link

Oui dac et dans notre cas l'incrémentation est fait par caldav. En tout cas je trouve cette manière de tester préférable aux mocks, sûrement qu'on pourrait nettoyer un peu tout ça, mais ça serait pour un autre ticket :)

Ouaip, je suis d'accord ! De base j'avais pas prévu de mettre les doigts dans les échanges entre la lib et EGW, mais comme j'ai finis avec du tcpdump j'étais plus à ça pret :P Après il faudra voir, je ne suis pas certain que la création soit aussi simple à mocker avec responses. Je créé le ticket à ce propos !

Par contre toujours à propos de cette ligne, plutôt qu'un callback il faudrait effectuer la vérification à partir de responses.calls[1].request

Ah ben oui ! Je suis bête, je me suis laissé emporté par les derniers patch que j'avais fais pour remplacer httmock (ou l'utilisation des callbacks est généralisée). Merci !

> Oui dac et dans notre cas l'incrémentation est fait par caldav. En tout cas je trouve cette manière de tester préférable aux mocks, sûrement qu'on pourrait nettoyer un peu tout ça, mais ça serait pour un autre ticket :) Ouaip, je suis d'accord ! De base j'avais pas prévu de mettre les doigts dans les échanges entre la lib et EGW, mais comme j'ai finis avec du tcpdump j'étais plus à ça pret :P Après il faudra voir, je ne suis pas certain que la création soit aussi simple à mocker avec `responses`. Je créé le ticket à ce propos ! > Par contre toujours à propos de cette ligne, plutôt qu'un callback il faudrait effectuer la vérification à partir de `responses.calls[1].request` Ah ben oui ! Je suis bête, je me suis laissé emporté par les derniers patch que j'avais fais pour remplacer httmock (ou l'utilisation des callbacks est généralisée). Merci !
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 4db7ad5694 to 3834a6b136 2024-03-21 14:23:01 +01:00 Compare
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 3834a6b136 to 12592397d8 2024-03-21 14:23:32 +01:00 Compare
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 12592397d8 to 02cda13d92 2024-03-21 14:28:19 +01:00 Compare
vdeniaud reviewed 2024-03-21 15:07:11 +01:00
tests/test_caldav.py Outdated
@ -423,2 +436,3 @@
@pytest.mark.parametrize('sequence,expt_sequence', [('', 1), ('SEQUENCE:41', 42)])
@responses.activate
def test_caldav_event_update_ok_nomock(app, caldav_conn):
def test_caldav_event_update_ok_nomock(app, caldav_conn, sequence, expt_sequence, event_update):
vdeniaud commented 2024-03-21 15:07:11 +01:00
Owner
Copy Link

C'est un peu dommage de passer par la machinerie de paramétrisation pour deux valeurs, pour tester la même chose on pourrait juste faire une deuxième requête après la première et vérifier que la séquence passe à 2 :

    # check sequence is incremented
    responses.reset()
    responses.add(responses.GET, evt_url, body=put_mock.request.body)
    responses.add(responses.PUT, evt_url, status=204, body='')
   
    resp = app_post(app, url, qs_params, event_update)
    assert resp.json['err'] == 0
   
    calendar = icalendar.Calendar.from_ical(responses.calls[1].request.body)
    vevent = calendar.walk('VEVENT')[0]
    assert vevent['SEQUENCE'] == 2

bonus ça permet de se passer du %(vevent_extra)s dans l'ICS. Tu en penses quoi ?

C'est un peu dommage de passer par la machinerie de paramétrisation pour deux valeurs, pour tester la même chose on pourrait juste faire une deuxième requête après la première et vérifier que la séquence passe à 2 : ``` # check sequence is incremented responses.reset() responses.add(responses.GET, evt_url, body=put_mock.request.body) responses.add(responses.PUT, evt_url, status=204, body='') resp = app_post(app, url, qs_params, event_update) assert resp.json['err'] == 0 calendar = icalendar.Calendar.from_ical(responses.calls[1].request.body) vevent = calendar.walk('VEVENT')[0] assert vevent['SEQUENCE'] == 2 ``` bonus ça permet de se passer du %(vevent_extra)s dans l'ICS. Tu en penses quoi ?
yweber commented 2024-03-21 15:31:56 +01:00
Author
Owner
Copy Link

Ca m'ennuie un peu dans le sens ou ça serait tester qu'on ne respecte pas la RFC .

L'idée serait de reconfiguré responses pour que le prochain GET renvoie l'ics qu'on a envoyé lors de notre PUT. Hors, à ce moment, le test serait : est-ce qu'on a bien incrémenté SEQUENCE alors qu'on a apporté aucune modification à l'évènement .

Ca me donne presque envie de vérifier si la lib est bien foutu et gère ce cas ou il ne faudrait pas toucher à SEQUENCE ;)

Ca m'ennuie un peu dans le sens ou ça serait tester qu'on ne respecte pas la RFC . L'idée serait de reconfiguré `responses` pour que le prochain `GET` renvoie l'ics qu'on a envoyé lors de notre `PUT`. Hors, à ce moment, le test serait : est-ce qu'on a bien incrémenté SEQUENCE alors qu'on a apporté aucune modification à l'évènement . Ca me donne presque envie de vérifier si la lib est bien foutu et gère ce cas ou il ne faudrait pas toucher à SEQUENCE ;)
vdeniaud commented 2024-03-21 15:36:04 +01:00
Owner
Copy Link

Si ce n'est que ça alors envoyer une modif genre {'DTSTART': '2020-02-21'} au lieu de event_update, sans toucher au reste du code que je suggère

(mais ça peut aussi pointer que cette histoire de vérif de sequence vivrait mieux dans son propre test)

Si ce n'est que ça alors envoyer une modif genre `{'DTSTART': '2020-02-21'}` au lieu de `event_update`, sans toucher au reste du code que je suggère (mais ça peut aussi pointer que cette histoire de vérif de sequence vivrait mieux dans son propre test)
yweber commented 2024-03-21 15:59:17 +01:00
Author
Owner
Copy Link

(mais ça peut aussi pointer que cette histoire de vérif de sequence vivrait mieux dans son propre test)

Yep, je pense que c'est plutôt ça le problème en fait :) Au final ce test avait été écrit pour une raison totalement différente et se retrouve à tester 50 choses en même temps, c'est pas ouf !

> (mais ça peut aussi pointer que cette histoire de vérif de sequence vivrait mieux dans son propre test) Yep, je pense que c'est plutôt ça le problème en fait :) Au final ce test avait été écrit pour une raison totalement différente et se retrouve à tester 50 choses en même temps, c'est pas ouf !
yweber force-pushed wip/88417-caldav-SEQUENCE-handling from 02cda13d92 to 923125e786 2024-03-21 16:11:36 +01:00 Compare
vdeniaud approved these changes 2024-03-21 16:19:56 +01:00
yweber merged commit 923125e786 into main 2024-03-21 16:41:57 +01:00
yweber deleted branch wip/88417-caldav-SEQUENCE-handling 2024-03-21 16:41:57 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
vdeniaud
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#500
No description provided.
Delete Branch "wip/88417-caldav-SEQUENCE-handling"

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?