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.

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

Fix: Inconsistencies due to ALTREP parameter #1916

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

max65482
Copy link
Contributor

This PR addresses an inconsistency issue with Thunderbird. In addition to the plain text description of a task, Thunderbird also saves a formatted HTML version inside the ALTREP parameter. NC calendar does not alter the ALTREP parameter when the plain text description is changed. This results in inconsistencies.

The proposed solution deletes the ALTREP parameter upon modification. This prevents inconsistencies. Thunderbird keeps accepting plaintext-only descriptions.

I have opened a similar PR in nextcloud/calendar as it is equally concerned.

@benbucksch
Copy link

benbucksch commented Jan 29, 2022

Thanks, max, for the patch!

As mentioned in nextcloud/calendar#3863 (comment) , I would recommend to delete all unknown parameters. They give additional information about the description specifically (not the entire calendar item), but the description changed, so any parameter that is still here is in risk of being out of sync now.

Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #1916 (b48ae73) into master (24a67fe) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1916      +/-   ##
============================================
- Coverage     30.31%   30.30%   -0.01%     
  Complexity       41       41              
============================================
  Files            66       66              
  Lines          3216     3217       +1     
  Branches        665      665              
============================================
  Hits            975      975              
- Misses         2241     2242       +1     

Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>
@max65482
Copy link
Contributor Author

Agreed! Now all parameters are being removed.
As I could not find a clean way to access the single parameters, I simple delete the property and recreate it.

Copy link

@benbucksch benbucksch left a comment

Choose a reason for hiding this comment

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

I like the idea

src/models/task.js Outdated Show resolved Hide resolved
Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>
@raimund-schluessler raimund-schluessler merged commit bffcd45 into nextcloud:master Jan 31, 2022
@Gorendal
Copy link

Gorendal commented Mar 8, 2023

Hi there!
Looks like the X-ALT-DESC parameter is not yet cleared for Tasks like for Calendar items. (see nextcloud/calendar#3863 (comment)).
I guess you would like me to create a pull-request again - but I still don't know how.
Could you please support?

@max65482
Copy link
Contributor Author

Do you want to report the problem? Then you can file an issue (click "New issue" here and fill in the gaps).
If you want to implement the solution yourself maybe look at #4744 (the fix for calendar) and #1916 (a similar fix in tasks).

@Gorendal
Copy link

Hi Max!
Sorry for the late reply. I hoped like the similar fix for Nextcloud Calendar this might not require all the efforts for creating a new issue. However as you see above I created one right now.
Unfortunatelly I have no Nextcloud development environment set up and only little Github knowledge so far. So I am not able to fix this by myself without excessive preparation efforts. So I hope you or somebody else might help here?

@max65482
Copy link
Contributor Author

Fix is in #2240.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants