Skip to content

Conversation

@max65482
Copy link
Contributor

@max65482 max65482 commented Jan 27, 2022

This PR addresses issue #3863: In addition to the plain text description of an event, 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 will have opened a similar PR in nextcloud/tasks as it is equally concerned.

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #3918 (30469aa) into main (dab0515) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3918      +/-   ##
============================================
- Coverage     29.61%   29.42%   -0.20%     
  Complexity      322      322              
============================================
  Files           220      221       +1     
  Lines          7529     7578      +49     
  Branches        993     1001       +8     
============================================
  Hits           2230     2230              
- Misses         5299     5348      +49     
Flag Coverage Δ
javascript 20.80% <0.00%> (-0.17%) ⬇️
php 67.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/store/calendarObjectInstance.js 0.00% <0.00%> (ø)
src/views/EditSimple.vue 0.00% <0.00%> (ø)
src/mixins/EditorMixin.js 0.00% <0.00%> (ø)
src/views/EditSidebar.vue 0.00% <0.00%> (ø)
...rc/components/Editor/InvitationResponseButtons.vue 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dab0515...30469aa. Read the comment docs.

@tcitworld
Copy link
Member

I think this could be handled here instead, which would be cleaner.

changeDescription(state, { calendarObjectInstance, description }) {
calendarObjectInstance.eventComponent.description = description
calendarObjectInstance.description = description
},

@max65482
Copy link
Contributor Author

max65482 commented Jan 28, 2022

This method is called upon every keystroke in the description field. Deleting ALTREP here would cause the loss of formatting even if the user discards changes by closing the dialog. I would prefer to delete ALTREP only if the user explicitly clicks "Update" and changes were made (isDirty).

@max65482
Copy link
Contributor Author

Sorry, I misunderstood how changeDescription works. The same behavior can be achieved there as well. I now moved the fix to this function.

@szaimen szaimen added the 3. to review Waiting for reviews label Jan 28, 2022
@szaimen szaimen added this to the v3.1.0 milestone Jan 28, 2022
@szaimen szaimen added the enhancement New feature request label Jan 28, 2022
@szaimen szaimen requested review from miaulalala and st3iny January 28, 2022 11:24
@st3iny st3iny added bug and removed enhancement New feature request labels Jan 28, 2022
@st3iny
Copy link
Member

st3iny commented Jan 28, 2022

/backport to stable3.0

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request A backport was requested for this pull request label Jan 28, 2022
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix. I tested it and it works fine.

Please sign off your commits before we can merge them. To add your Signed-off-by line to every commit in this branch:

  1. Ensure you have a local copy of your branch by checking out the pull request locally via command line.
  2. In your local branch, run: git rebase HEAD~2 --signoff
  3. Force push your changes to overwrite the branch: git push --force-with-lease origin fix_altrep

@max65482
Copy link
Contributor Author

Perfect! The commits are signed off now.

@tcitworld
Copy link
Member

As DESCRIPTION can have more parameters than ALTREP, such as LANGUAGE or vendors parameters (surely there must be some client that uses X-VENDOR-DESC-HTML), should we remove all of them instead of just ALTREP?

@max65482
Copy link
Contributor Author

I agree that it makes sense to remove all parameters that are not updated consistently (so, currently all of them).
I got the implementation ready. Let me know if I should submit it. And where (this PR?).

@benbucksch
Copy link

@max65482 Thanks so much for the fix! 👍

should we remove all of them instead of just ALTREP?

+1

This is also what I recommended in #3863 (comment)

@max65482
Copy link
Contributor Author

As this PR is merged and fixes the issue, I think we should consider it complete.
For the deletion of all parameters I opened #3924.

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

Labels

3. to review Waiting for reviews backport-request A backport was requested for this pull request bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Calendar ignores ical DESCRIPTION;ALTREP when updating event description

5 participants