-
Notifications
You must be signed in to change notification settings - Fork 348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CalDAV to sync properly when limit is set in PDO backend #1248
CalDAV to sync properly when limit is set in PDO backend #1248
Conversation
65bfd29
to
50a253b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1248 +/- ##
============================================
+ Coverage 97.35% 97.37% +0.01%
- Complexity 2810 2817 +7
============================================
Files 174 174
Lines 7979 7996 +17
============================================
+ Hits 7768 7786 +18
+ Misses 211 210 -1
Continue to review full report at Codecov.
|
It's a bit hard for me to eye this and see if it's correct, but it looks good. I feel that this can use a good amount of unittests to ensure that edge cases are sufficiently covered and increase confidence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests! THX
bad5b51
to
cc150c0
Compare
I added test case for each of two changes. |
@DeepDiver1975 I added some tests after your comments, should I add some more change? |
Hi, is there something I can do to advance this pull request? |
@DeepDiver1975 Hi, do you have some comments about added tests? |
@phil-davis @DeepDiver1975 Hi, do you know the status about this? I added some tests according to your comment, long ago, that I'm not sure about my memory, anything more I should do? |
7cbe940
to
73cccc4
Compare
73cccc4
to
69f114f
Compare
Rebased and squashed. LGTM, thanks. Merging. |
Hi,
I altered some codes to make CalDAV sync properly even when limit is set. Changes is mainly as below two:
the DAV:sync-token value returned in the response MUST represent the correct state for the partial set of changes returned.
OneCalendar in Windows Store sends a request with limit 1000, and I checked using this, but with the changes below:
lib/CalDAV/Backend/PDO.php
while testing.I think CardDAV has the same issue, but I did not find an environment to reproduce and test with, so I wrote a code only for CalDAV, PDO backend.
This is my first code around WebDAV, so could you take a look including my understanding is correct or not?