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

Jump to bottom

Ensure no description changes are lost #41370

Open

djon2003 wants to merge 1 commit into nextcloud:master from djon2003:fix_description_discrepency

djon2003 commented Nov 10, 2023

Resolves: X-ALT-DESC parameter is not cleared for tasks when description is changed tasks#2239

Summary

The first resolution was fixing it when using the NextCloud web interface, but not when using an application like OpenTasks on Android. This ensures that the alternate description is deleted if the description is modified alone. So, it better supports application only supporting textual description used in conjonction with others supporting also HTML.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included ==> No, as I'm new to modifying your software and as such I am not familiar with your structure, but if someone tells me where to add tests, I will.
Screenshots before/after for front-end changes ==> No screenshot.
Documentation (manuals or wiki) has been updated or is not required ==> Not required
Backports requested where applicable (ex: critical bugfixes)

I am joining the previous person who solved the issue (@max65482) and the original issue requester (@Gorendal).


          Ensure no description changes are lot

a15f946

Signed-off-by: Jonathan Boivin <djon2003@hotmail.com>

djon2003 requested review from ChristophWurst, miaulalala and tcitworld as code owners

November 10, 2023 02:58

djon2003 changed the title ~~Ensure no description changes are lot~~ Ensure no description changes are lost

Author

djon2003 commented Nov 10, 2023

Maybe should I implement also for ALTREP like nextcloud/tasks#1916 ?

ChristophWurst added 3. to review feature: dav feature: caldav bug labels

github-advanced-security bot found potential problems

View reviewed changes

apps/dav/lib/CalDAV/Schedule/Plugin.php

+              	    // Obtain previous version
+              	    $node = $this->server->tree->getNodeForPath($request->getPath());
+              	    $oldObj = Reader::read($node->get());

Check failure

Code scanning / Psalm

UndefinedInterfaceMethod Error

Method Sabre\DAV\INode::get does not exist

apps/dav/lib/CalDAV/Schedule/Plugin.php

+              	    // If all description fields are present, then verify consistency
+              	    if ($hasAllDesc) {
+              	        // Get descriptions
+              	        $oldDescription = (string) $oldObj->VTODO->DESCRIPTION;

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

Instance property Sabre\VObject\Property::$DESCRIPTION is not defined

apps/dav/lib/CalDAV/Schedule/Plugin.php

+              	    if ($hasAllDesc) {
+              	        // Get descriptions
+              	        $oldDescription = (string) $oldObj->VTODO->DESCRIPTION;
+              	        $newDescription = (string) $vCal->VTODO->DESCRIPTION;

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

Instance property Sabre\VObject\Property::$DESCRIPTION is not defined

apps/dav/lib/CalDAV/Schedule/Plugin.php

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

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

Instance property Sabre\VObject\Property::$X-ALT-DESC is not defined

apps/dav/lib/CalDAV/Schedule/Plugin.php

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

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

Instance property Sabre\VObject\Property::$X-ALT-DESC is not defined

apps/dav/lib/CalDAV/Schedule/Plugin.php

@@ @@ -632,4 +637,38 @@ @@
               			'{DAV:}displayname' => $displayName,
               		]);
               	}
+              	private function ensureDescriptionConsistency(RequestInterface $request, VCalendar $vCal, &$modified) {

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OCA\DAV\CalDAV\Schedule\Plugin::ensureDescriptionConsistency does not have a return type, expecting void

apps/dav/lib/CalDAV/Schedule/Plugin.php

@@ @@ -632,4 +637,38 @@ @@
               			'{DAV:}displayname' => $displayName,
               		]);
               	}
+              	private function ensureDescriptionConsistency(RequestInterface $request, VCalendar $vCal, &$modified) {

Check notice

Code scanning / Psalm

MissingParamType Note

Parameter $modified has no provided type

apps/dav/lib/CalDAV/Schedule/Plugin.php

+              	    // If all description fields are present, then verify consistency
+              	    if ($hasAllDesc) {
+              	        // Get descriptions
+              	        $oldDescription = (string) $oldObj->VTODO->DESCRIPTION;

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $oldObj->VTODO of type Sabre\VObject\Property|null

apps/dav/lib/CalDAV/Schedule/Plugin.php

+              	    if ($hasAllDesc) {
+              	        // Get descriptions
+              	        $oldDescription = (string) $oldObj->VTODO->DESCRIPTION;
+              	        $newDescription = (string) $vCal->VTODO->DESCRIPTION;

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $vCal->VTODO of type Sabre\VObject\Property|null

Contributor

miaulalala commented Nov 15, 2023

What's the purpose of the alternate description field? What do clients use it for? I've got some changes I'd like to see but I feel like I'm missing some context here.

Member

tcitworld commented Nov 15, 2023 •

edited

This is similar to nextcloud/calendar#3863 and other issues linked, and as you've mentioned there's more special keys or parameters to handle like ALTREP, X-APPLE-STRUCTURED-LOCATION, …

I'd say clients are responsible for handling data, and by using them users are responsible for inconsistent data, because we simply can't fix all their mistakes. You should avoid clients that use non-standard properties or parameters (STYLED-DESCRIPTION is the way to go), and OpenTasks in particular is not maintained anymore.

In any case, this should at least be extracted to a proper "repair" class, apply to VOBJECTs just like VTODOs, and be extensible, not in the scheduling plugin. Or possibly upstreamed directly to Sabre.

Author

djon2003 commented Nov 15, 2023 •

edited

@miaulalala The extra field is used to store the HTML version of the description by many clients. Those clients also manage the description field which is only textual. The issue comes when one uses a mix that includes one of those clients and other textual only clients. In my case: NextCloud, EmClient and OpenTasks.

@miaulalala AND @tcitworld Here is a link where all that started for me. https://forum.emclient.com/t/caldav-sync-of-notes-field-in-tasks-does-not-work-properly/76642/37 One of the guys asked for a NextCloud modification and it has been done, but only for the client side not the server side.

I agree, this is similar to the issue you mentioned, as I already mentioned it. I'm fixing only the description discrepancy, so I don't think X-APPLE-STRUCTURED-LOCATION or none related field to DESCRIPTION should be considered. That said, ALTREP is likely concerned and thus shall be included in my fix.

The STYLED-DESCRIPTION is an official property, but still in the PROPOSED STANDARD status. It was published on August 2021 which is quite recent for an RFC. Still, we could consider it, but it shall be implemented probably by the third party part of the code. Meanwhile and before the existence of this property, the developers didn't wait and used "X-" properties to add new features. The "X-ALT-DESC" property seems to be really common and supported by many clients. That said, I think it doesn't hurt to help storing data so nothing is lost. Because, that is what is happening right now. Moreover, the client side of NextCloud as been fixed, so why not the server side. I agree, that the clients shall behave perfectly, but if the goal of NextCloud is to serve as data storage, data lost is unacceptable. Hence, ensuring clients doesn't mess with that can be considered as part of the goal. I also agree, we can't fix all their mistakes, but this one is done, lets use it, no?

If you have suggestions for a free Android app that does a great job like OpenTasks, I am open to look at them, but this won't fix the issue, I am quite sure. OpenTasks, even though not maintained anymore, is doing the job correctly by ignoring the "X-ALT-DESC" property, but not EmClient. I already asked them to fix this, but the answer is not as that straight forward as I realized making this change (meaning this PR). In fact, this is a kind of gray zone where every actors in the issue is saying the other shall fix it. Bouncing the ball is clearly not helping the end users. You did not tell me to ask a fix from them, but saying not using it due to that small issue that I corrected was kinda bouncing or "not in my court" behavior.

That said, those had only objective to be constructive criticism / opinion.

I like the idea of a "repair" class. Because it was the first time I was coding NextCloud, I used reverse engineering to find where my modification could be applied. If you could guide me a bit about the structure and high level tips, I could extract it to another class. My first questions would be:

Where shall this new class file located?
How should it be named to respect current NextCloud naming standards?
Shall this new class act as a plugin, an helper class that would be called by the class it is extracted from, or else?

"Or possibly upstreamed directly to" ==> Not sure to understand. ==> After the correction of your message, I would say, yes, the fix could be incorporated into Sabre directly.

solracsf added this to the Nextcloud 28 milestone

blizzz mentioned this pull request

28.0.0 RC1 #41621

Merged

5 tasks

blizzz modified the milestones: Nextcloud 28, Nextcloud 29

Author

djon2003 commented Dec 15, 2023 •

edited

@miaulalala @tcitworld @blizzz @ChristophWurst

I put everyone currently named in the PR because I am not sure who to talk to.

I know, it is opensource and people may have not have the time to answer me, but a month had past and I am only sending a little ping.

There is a new major version that came out, so while this PR is not merged, I will have to:

Remove my patch
Update
Reapply my patch

That's not so bad. It was only to mention it.

So, if anyone of you have some time, I would appreciate your answers to my questions so I could start changing the code to reflect those and to eventually merge this PR. Thank you!

tcitworld mentioned this pull request

feat(caldav): add repair steps in sabre calendarobject change hook #42347

Open

7 tasks

Member

tcitworld commented Dec 18, 2023

@djon2003 See #42347 for a more complete generic repair middleware plugin.

Author

djon2003 commented Dec 18, 2023 •

edited

@djon2003 See #42347 for a more complete generic repair middleware plugin.

Oh! Great! I thought you would have give me tips and I would have done it, but it is OK.

Few things:

Would you like help to complete your TODOs?
If you don't mind mentioning my name somewhere in the code, that would be awesome.
Shall I close this PR? I think I do.

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

This was referenced Mar 20, 2024

29.0.0 beta 5 #44361

Merged

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ChristophWurst Awaiting requested review from ChristophWurst ChristophWurst is a code owner

miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

At least 2 approving reviews are required to merge this pull request.