Ensure description is coherent when only the main field is modified #1577

djon2003 · 2024-12-01T18:17:40Z

The problem comes when one uses two software:

One using only the Description field
One using Description and the alternate version

Having a modification in the description field without the alternate one makes the software using both to show the alternate version and the other, the main one which are no more coherent. The solution applied was to remove the alternate description when only the description is changed.

A full description of this issue is described here:
nextcloud/tasks#2239

Initially, I did the modification in NextCloud Server, but I think it would be more relevant here.
nextcloud/server#41370

The webclient was fixed in that sense, but when using external clients, the server should be fixed also if we want to keep the data coherent.

…e main is. Signed-off-by: Jonathan Boivin <djon2003@hotmail.com>

codecov · 2024-12-09T06:01:29Z

Codecov Report

Attention: Patch coverage is 55.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 97.13%. Comparing base (ed901a9) to head (a8a1dce).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/CalDAV/Schedule/Plugin.php	55.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1577      +/-   ##
============================================
- Coverage     97.23%   97.13%   -0.10%     
- Complexity     2836     2848      +12     
============================================
  Files           175      175              
  Lines          9028     9048      +20     
============================================
+ Hits           8778     8789      +11     
- Misses          250      259       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

phil-davis · 2024-12-09T06:04:16Z

@djon2003 php-cs-fixer wants some minor format changes.

https://github.com/sabre-io/dav/actions/runs/12107906781/job/34110249468?pr=1577

You can do composer cs-fixer and commit and push the changes to get it to pass.

phil-davis · 2024-12-09T06:09:31Z

lib/CalDAV/Schedule/Plugin.php

+     * Ensure the alternate version of the description is removed if only the main one is changed
+     */
+    private function ensureDescriptionConsistency($oldObj, VCalendar $vCal, &$modified) {


Please add PHPdoc for the $oldObj and $modified parameters.
(They will be the same as for processICalendarChange)

phil-davis · 2024-12-09T06:21:49Z

lib/CalDAV/Schedule/Plugin.php

+        $hasAllDesc = $hasOldDescription && $hasNewDescription && $hasOldXAltDesc && $hasNewXAltDesc;
+
+        // If all description fields are present, then verify consistency
+        if ($hasAllDesc) {


Can you please add some unit test(s) that will exercise this new code block?

I tried to run actual tests either directly from a Windows terminal or WSL, but I am getting file_put_contents(/MY_PATH/../tempcol/test.txt): Failed to open stream: No such file or directory

I tried everything I thought. Nothing works.

Running composer install from WSL gives:

PHP Fatal error: Uncaught ArgumentCountError: array_merge() does not accept unknown named parameters in /usr/share/php/Composer/DependencyResolver/DefaultPolicy.php:84
Stack trace:
#0 [internal function]: array_merge()
#1 /usr/share/php/Composer/DependencyResolver/DefaultPolicy.php(84): call_user_func_array()

I don't know what version of composer you get in WSL.
To update to the latest composer I do:
sudo composer self-update

I have:

$ composer --version Composer version 2.8.3 2024-11-17 13:13:04 PHP version 7.4.33 (/usr/bin/php7.4)

I have an actual Ubuntu laptop, no Windows. The CI runs in Ubuntu Linux, no Windows anywhere. Maybe use VirtualBox and create a full Ubuntu VM?

The "Failed to open stream" message is maybe coming from a line in ServerPropsTest.php

file_put_contents(\Sabre\TestUtil::SABRE_TEMPDIR.'col/test.txt', 'Test contents');

SABRE_TEMPDIR should be a constant __DIR__.'/../temp/' that already has / on the end, but somehow the / must be missing and the code ends up trying to write to a directory called testcol.

Try adding a / in front of col

....SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 61 / 1645 ( 3%) SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS............ 122 / 1645 ( 7%) ............................................................. 183 / 1645 ( 11%) ............................................................. 244 / 1645 ( 14%) ............................................................. 305 / 1645 ( 18%) ............................................................. 366 / 1645 ( 22%) ............................................................. 427 / 1645 ( 25%) ............................................................. 488 / 1645 ( 29%) ............................................................. 549 / 1645 ( 33%) ............................................................. 610 / 1645 ( 37%) ............................................................. 671 / 1645 ( 40%) .........SSSSSSSSSSSSSSSSSSSSSSSSSSSS........................ 732 / 1645 ( 44%) ............................................................. 793 / 1645 ( 48%) .................................................SSSS........ 854 / 1645 ( 51%) ............................................................. 915 / 1645 ( 55%) ............................................................. 976 / 1645 ( 59%) .......................................SSSSSSSSSSSSSSSS...... 1037 / 1645 ( 63%) ............................................................. 1098 / 1645 ( 66%) .........................................SSSSSSSSSSSSSSSSSS.. 1159 / 1645 ( 70%) ............................................................. 1220 / 1645 ( 74%) ............................................................. 1281 / 1645 ( 77%) ............................................................. 1342 / 1645 ( 81%) ............................................................. 1403 / 1645 ( 85%) ............................................................. 1464 / 1645 ( 88%) ............................................................. 1525 / 1645 ( 92%) .....SSSSSSSSSSSSSSSSSSSSSSSSSS.............................. 1586 / 1645 ( 96%) ........................................................... 1645 / 1645 (100%) Time: 00:08.670, Memory: 54.00 MB OK, but incomplete, skipped, or risky tests! Tests: 1645, Assertions: 2909, Skipped: 198.

I created myself a Docker container to run the unit tests. Is this a normal execution result? (I still did not add mine, I will when I will be sure that this is the normal expections). I will add this Dockerfile with my PR so others could use it to test easily.

The docker is setup using Ubuntu 24.04, with your version of PHP and composer.

I have the same result with or without running php -S localhost:8000 -t vendor/sabre/http/tests/www.

BTW, the last part of the path (vendor/sabre/http/tests/www) does not exist when:

Cloning repo

composer install

Now I should run the command in CONTRIBUTION.md, the one above, but surely, it tells me that the folder doesn't exist

@phil-davis I pushed a commit including docker support for the unit tests.

That is the normal unit test result. Some unit tests need exgtra environment set up, so they are skipped by default (the "S").

djon2003 · 2024-12-12T05:04:48Z

tests/docker/run-tests.sh

+  composer install
+
+  mkdir -p vendor/sabre/http/tests/www
+  php -S localhost:8000 -t vendor/sabre/http/tests/www &


@phil-davis With or without this line (line 33), I obtain the same result.

Add method to remove the alternate description if not modified but th…

18db635

…e main is. Signed-off-by: Jonathan Boivin <djon2003@hotmail.com>

phil-davis reviewed Dec 9, 2024

View reviewed changes

Add docker support for unit tests

a232ddd

djon2003 commented Dec 12, 2024

View reviewed changes

Remove debugging commented line

a8a1dce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure description is coherent when only the main field is modified #1577

Ensure description is coherent when only the main field is modified #1577

djon2003 commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

phil-davis commented Dec 9, 2024

phil-davis Dec 9, 2024

phil-davis Dec 9, 2024

djon2003 Dec 10, 2024

djon2003 Dec 10, 2024

phil-davis Dec 10, 2024

djon2003 Dec 12, 2024 •

edited

Loading

djon2003 Dec 12, 2024

phil-davis Dec 12, 2024

djon2003 Dec 12, 2024 •

edited

Loading

Ensure description is coherent when only the main field is modified #1577

Are you sure you want to change the base?

Ensure description is coherent when only the main field is modified #1577

Conversation

djon2003 commented Dec 1, 2024 • edited Loading

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

phil-davis commented Dec 9, 2024

phil-davis Dec 9, 2024

Choose a reason for hiding this comment

phil-davis Dec 9, 2024

Choose a reason for hiding this comment

djon2003 Dec 10, 2024

Choose a reason for hiding this comment

djon2003 Dec 10, 2024

Choose a reason for hiding this comment

phil-davis Dec 10, 2024

Choose a reason for hiding this comment

djon2003 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

djon2003 Dec 12, 2024

Choose a reason for hiding this comment

phil-davis Dec 12, 2024

Choose a reason for hiding this comment

djon2003 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

djon2003 commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

djon2003 Dec 12, 2024 •

edited

Loading

djon2003 Dec 12, 2024 •

edited

Loading