Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18db635
Add method to remove the alternate description if not modified but th…
djon2003 Dec 1, 2024
a232ddd
Add docker support for unit tests
djon2003 Dec 12, 2024
a8a1dce
Remove debugging commented line
djon2003 Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions lib/CalDAV/Schedule/Plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,8 +597,9 @@
}

/**
* This method looks at an old iCalendar object, a new iCalendar object and
* starts sending scheduling messages based on the changes.
* This method looks at an old iCalendar object, a new iCalendar object and:
* - starts sending scheduling messages based on the changes.
* - ensures the description fields are coherent
*
* A list of addresses needs to be specified, so the system knows who made
* the update, because the behavior may be different based on if it's an
Expand All @@ -612,6 +613,8 @@
*/
protected function processICalendarChange($oldObject, VCalendar $newObject, array $addresses, array $ignore = [], &$modified = false)
{
$this->ensureDescriptionConsistency($oldObject, $newObject, $modified);

$broker = $this->createITipBroker();
$messages = $broker->parseEvent($newObject, $addresses, $oldObject);

Expand Down Expand Up @@ -1003,4 +1006,41 @@
{
return new Broker();
}

/**
* Ensure the alternate version of the description is removed if only the main one is changed
*/
private function ensureDescriptionConsistency($oldObj, VCalendar $vCal, &$modified) {
Comment on lines +1011 to +1013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!$oldObj) {
return; // No previous version to compare
}

$xAltDescPropName = "X-ALT-DESC";

// Get presence of description fields
$hasOldDescription = isset($oldObj->VTODO) && isset($oldObj->VTODO->DESCRIPTION);
$hasNewDescription = isset($vCal->VTODO) && isset($vCal->VTODO->DESCRIPTION);
$hasOldXAltDesc = isset($oldObj->VTODO) && isset($oldObj->VTODO->{$xAltDescPropName});
$hasNewXAltDesc = isset($vCal->VTODO) && isset($vCal->VTODO->{$xAltDescPropName});
$hasAllDesc = $hasOldDescription && $hasNewDescription && $hasOldXAltDesc && $hasNewXAltDesc;

// If all description fields are present, then verify consistency
if ($hasAllDesc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@djon2003 djon2003 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Get descriptions
$oldDescription = (string) $oldObj->VTODO->DESCRIPTION;
$newDescription = (string) $vCal->VTODO->DESCRIPTION;
$oldXAltDesc = (string) $oldObj->VTODO->{$xAltDescPropName};
$newXAltDesc = (string) $vCal->VTODO->{$xAltDescPropName};

Check warning on line 1033 in lib/CalDAV/Schedule/Plugin.php

View check run for this annotation

Codecov / codecov/patch

lib/CalDAV/Schedule/Plugin.php#L1030-L1033 

Added lines #L1030 - L1033 were not covered by tests

// Compare descriptions
$isSameDescription = $oldDescription === $newDescription;
$isSameXAltDesc = $oldXAltDesc === $newXAltDesc;

Check warning on line 1037 in lib/CalDAV/Schedule/Plugin.php

View check run for this annotation

Codecov / codecov/patch

lib/CalDAV/Schedule/Plugin.php#L1036-L1037 

Added lines #L1036 - L1037 were not covered by tests

// If the description changed, but not the alternate one, then delete the latest
if (!$isSameDescription && $isSameXAltDesc) {
unset($vCal->VTODO->{$xAltDescPropName});
$modified = true;

Check warning on line 1042 in lib/CalDAV/Schedule/Plugin.php

View check run for this annotation

Codecov / codecov/patch

lib/CalDAV/Schedule/Plugin.php#L1040-L1042 

Added lines #L1040 - L1042 were not covered by tests
}
}
}
}
Loading