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

Include more information in iMIP email and show diff information on updating an event #35743

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Dec 12, 2022

Summary

Instead of taking the date and time of the main recurrence, the changed VEVENT is filtered from the main VCALENDAR event and used to build a diff to accurately represent what changed.

Screenshots to follow

To Do

  • Check if the logic is still sound if there is a recurrence that has a recurrence exception - what happens if I update the whole event - done, the logic is not sound yo
  • Instead, filter old and new events and only leave what's been modified. Signifiers for this change are LAST-MODIFIED, SEQUENCE, RECURRENCE-ID / RRULE
  • Build a visual diff for the email

As discussed with @ChristophWurst:

  • Extract the event comparisons into its own service
  • refactor the iMIP plugin to use that service instead of private methods
  • make the data array backwards- compatible by introducing new array params for the new values and leaving the old ones unchanged (as templates that have overwritten ours might break otherwise)
  • Adjust PHPUnit tests
    • ImipPluginTest
    • ImipServicetest

Testing manually

All events must have at least one attendee to trigger the iMIP logic

  • Manually test with a single event - creating, updating
  • Manually test with a new recurring event without a preexisting old event - test changing the main recurrence, creating a single or multiple recurrence exceptions in the middle
  • Accept invitation via Mail app: Organizer should get an email that the invitation to the event was accepted (reply imip)

Checklist

@miaulalala miaulalala self-assigned this Dec 12, 2022
@miaulalala miaulalala added 2. developing Work in progress feature: caldav Related to CalDAV internals bug labels Dec 12, 2022
@miaulalala
Copy link
Contributor Author

/backport to stable25

@miaulalala
Copy link
Contributor Author

/backport to stable24

@miaulalala
Copy link
Contributor Author

/backport to stable23

@szaimen szaimen added this to the Nextcloud 26 milestone Dec 12, 2022
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 13, 2022
@miaulalala
Copy link
Contributor Author

miaulalala commented Dec 13, 2022

unfortunately, the approach with just the reccurence-id doesn't work correctly if I first update a single recurrence and then the whole instance.

@miaulalala miaulalala added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 13, 2022
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
@@ -131,6 +140,51 @@
$this->userManager = $userManager;
}

public function initialize(DAV\Server $server) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\CalDAV\Schedule\IMipPlugin::initialize does not have a return type, expecting void
* @param resource $data data
* @param bool $modified modified
*/
public function beforeWriteContent($uri, INode $node, $data, $modified) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\CalDAV\Schedule\IMipPlugin::beforeWriteContent does not have a return type, expecting void
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
@miaulalala miaulalala changed the title Use recurrence exception VEVENT to build iMip email Include more information in iMIP email and show diff information on updating an event Dec 15, 2022
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
// No changed events after all - this shouldn't happen if there is significant change yet here we are
// The scheduling status is debatable
// @todo handle this error case
if(!is_array($newEventComponents) || empty($newEventComponents)) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction

Docblock-defined type array<array-key, mixed> for $newEventComponents is always array
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@miaulalala
Copy link
Contributor Author

I had a look, it's not in there yet. Will have to be a followup ticket due to time constraints this week. Sorry @onny

@miaulalala miaulalala force-pushed the fix/use-recurrence-for-imip-email branch from a39a850 to ca4728c Compare January 30, 2023 17:33
@blizzz blizzz mentioned this pull request Feb 1, 2023
@miaulalala miaulalala force-pushed the fix/use-recurrence-for-imip-email branch 2 times, most recently from d5fd00b to 37e1141 Compare February 2, 2023 14:32
instead of the main VEVENT of a repeating event

Fixes part of nextcloud/calendar#3919

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala force-pushed the fix/use-recurrence-for-imip-email branch from 37e1141 to 38e9cb6 Compare February 2, 2023 15:26
@blizzz blizzz merged commit 58e4a83 into master Feb 2, 2023
@blizzz blizzz deleted the fix/use-recurrence-for-imip-email branch February 2, 2023 17:09
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable24
git pull origin/stable24

# Create the new backport branch
git checkout -b fix/foo-stable24

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable24

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable23
git pull origin/stable23

# Create the new backport branch
git checkout -b fix/foo-stable23

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable23

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@miaulalala
Copy link
Contributor Author

/backport to stable25

@miaulalala
Copy link
Contributor Author

/backport to stable24

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable24
git pull origin/stable24

# Create the new backport branch
git checkout -b fix/foo-stable24

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable24

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable23
git pull origin/stable23

# Create the new backport branch
git checkout -b fix/foo-stable23

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable23

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@st3iny
Copy link
Member

st3iny commented Feb 8, 2023

/backport to stable25

@st3iny
Copy link
Member

st3iny commented Feb 8, 2023

/backport to stable24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug feature: caldav Related to CalDAV internals
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants