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

Jump to bottom

Consider only reminders with calendar data #20766

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Consider only reminders with calendar data #20766

merged 2 commits into from
Feb 18, 2022

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented May 1, 2020 •
edited
Loading

Encountered an issue once like this one https://help.nextcloud.com/t/subscribed-calendar-not-updating-in-webui-again-subscribed-calendars-are-a-total-mess-in-nc/79774/2

Not sure why we can have calendar objects with empty data referenced into reminders.

Fix #30112
Fix #30265

@tcitworld tcitworld added 3. to review Waiting for reviews feature: dav labels May 1, 2020
@georgehrke
Copy link
Member

Not sure why we can have calendar objects with empty data referenced into reminders.

https://github.com/nextcloud/server/blob/master/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php#L114
https://github.com/nextcloud/server/blob/master/apps/dav/lib/CalDAV/CalDavBackend.php

purgeAllCachedEventsForSubscription doesn't seem to clean the reminders.

@georgehrke georgehrke self-assigned this May 7, 2020
@rullzer
Copy link
Member

rullzer commented May 15, 2020

So what are the next steps here?

@MorrisJobke
Copy link
Member

So what are the next steps here?

@georgehrke 🏓

@georgehrke
Copy link
Member

There is an issue with the oc_calendar_reminders table. I will discuss that with tcit.

@PVince81
Copy link
Member

There is an issue with the oc_calendar_reminders table. I will discuss that with tcit.

was there any results from the discussion @tcitworld ?

@tcitworld
Copy link
Member Author

was there any results from the discussion @tcitworld ?

I can't remember we had this discussion, had forgotten about this. 🙉

@tcitworld tcitworld mentioned this pull request Dec 10, 2021
Closed
ChristophWurst
ChristophWurst approved these changes Dec 17, 2021
View reviewed changes
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.

Makes sense

@ChristophWurst
Copy link
Member

/backport to stable23

@ChristophWurst
Copy link
Member

/backport to stable22

@ChristophWurst
Copy link
Member

/backport to stable21

@tcitworld
Copy link
Member Author

I need to check what Georg linked to to see if there's a better fix.

@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch from c781e48 to 1b928a4 Compare December 17, 2021 15:14
tcitworld
tcitworld commented Dec 17, 2021
View reviewed changes
apps/dav/lib/AppInfo/Application.php

/** @var CalDavBackend $calDavBackend */
$calDavBackend = $container->query(CalDavBackend::class);
$calDavBackend->purgeAllCachedEventsForSubscription($subscriptionData['id']);
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice that we don't call purgeAllCachedEventsForSubscription anymore in SubscriptionDeletionListener, as the event is dispatched after the deletion of related calendarobjects, calendarchanges and calendarobjects_props, so it doesn't make sense to call it again.

purgeAllCachedEventsForSubscription is used only when we refresh the data, so that we can addChanges.

@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch from 1b928a4 to 1c5fc03 Compare December 17, 2021 15:46
@tcitworld

This comment has been minimized.

Sign in to view
@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch 3 times, most recently from f0387bd to 478c16d Compare December 17, 2021 16:30
@tcitworld tcitworld requested review from skjnldsv, juliusknorr and come-nc and removed request for rullzer and georgehrke December 17, 2021 16:33
@tcitworld

This comment has been minimized.

Sign in to view
@solracsf

This comment has been minimized.

Sign in to view
@tcitworld

This comment has been minimized.

Sign in to view
@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch from 478c16d to 8849d8d Compare December 19, 2021 19:47
@tcitworld tcitworld mentioned this pull request Dec 19, 2021
Closed
7 tasks
@tcitworld
Copy link
Member Author

Moved the new listener to a greater PR at #30335

@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch from 8849d8d to 32a9a5c Compare January 16, 2022 15:08
@tcitworld tcitworld mentioned this pull request Jan 27, 2022
Closed
@tcitworld tcitworld added this to the Nextcloud 24 milestone Jan 31, 2022
@tcitworld
Consider only reminders with calendar data
6f9afa7 
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
Clean reminders for calendar when deleting a subscription
22f8c42 
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the bug/fix-reminders-with-empty-calendar-data branch from 32a9a5c to 22f8c42 Compare February 15, 2022 09:15
@ChristophWurst ChristophWurst merged commit 6e0101b into master Feb 18, 2022
@ChristophWurst ChristophWurst deleted the bug/fix-reminders-with-empty-calendar-data branch February 18, 2022 14:52
This was referenced Feb 18, 2022
Merged
Merged
Merged
@tcitworld tcitworld mentioned this pull request Feb 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@miaulalala miaulalala miaulalala approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@come-nc come-nc Awaiting requested review from come-nc

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: dav
Projects
None yet
Milestone
Nextcloud 24
8 participants
@tcitworld @georgehrke @rullzer @MorrisJobke @PVince81 @ChristophWurst @solracsf @miaulalala