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

fix(dav): fix event birthday alarms not being updated #39977

Open

tcitworld wants to merge 2 commits into master from dav-fix-birthday-sync

+51 −52

Member

tcitworld commented Aug 19, 2023

In #33251 the default offset for a birthday event alarm was changed to 9AM on the day of the event.

However the birthdayEvenChanged method did not account for alarm changes, so it was never propagated to existing birthday events, which were kept on midnight.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

tcitworld added bug 3. to review feature: caldav feature: carddav labels

tcitworld added this to the Nextcloud 28 milestone

tcitworld requested a review from a team

August 19, 2023 09:47

tcitworld requested review from ChristophWurst and miaulalala as code owners

August 19, 2023 09:47

Member Author

tcitworld commented Aug 19, 2023

/backport to stable27

backportbot-nextcloud bot added the backport-request label

Member Author

tcitworld commented Aug 19, 2023

/backport to stable26

Member Author

tcitworld commented Aug 19, 2023

/backport to stable25

github-advanced-security bot found potential problems

View reviewed changes

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||
+              			($newCalendarData->VEVENT->VALARM && $existingBirthday->VEVENT->VALARM && $newCalendarData->VEVENT->VALARM->TRIGGER->getValue() !== $existingBirthday->VEVENT->VALARM->TRIGGER->getValue())

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||
+              			($newCalendarData->VEVENT->VALARM && $existingBirthday->VEVENT->VALARM && $newCalendarData->VEVENT->VALARM->TRIGGER->getValue() !== $existingBirthday->VEVENT->VALARM->TRIGGER->getValue())

Check failure

Code scanning / Psalm

UndefinedPropertyFetch Error

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

UndefinedPropertyFetch Note

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getValue on possibly null value

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

UndefinedPropertyFetch Note

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getValue on possibly null value

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||
+              			($newCalendarData->VEVENT->VALARM && $existingBirthday->VEVENT->VALARM && $newCalendarData->VEVENT->VALARM->TRIGGER->getValue() !== $existingBirthday->VEVENT->VALARM->TRIGGER->getValue())

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

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

apps/dav/lib/CalDAV/BirthdayService.php

@@ @@ -331,7 +336,8 @@ @@
               		return (
               			$newCalendarData->VEVENT->DTSTART->getValue() !== $existingBirthday->VEVENT->DTSTART->getValue() ||
-              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue()
+              			$newCalendarData->VEVENT->SUMMARY->getValue() !== $existingBirthday->VEVENT->SUMMARY->getValue() ||
+              			($newCalendarData->VEVENT->VALARM && $existingBirthday->VEVENT->VALARM && $newCalendarData->VEVENT->VALARM->TRIGGER->getValue() !== $existingBirthday->VEVENT->VALARM->TRIGGER->getValue())

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

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

nfebe approved these changes

View reviewed changes

Contributor

nfebe left a comment

LGTM

come-nc approved these changes

View reviewed changes

miaulalala force-pushed the dav-fix-birthday-sync branch from 03a300c to f3ccb5d Compare

August 28, 2023 10:35

tcitworld force-pushed the dav-fix-birthday-sync branch from f3ccb5d to 24e6ef5 Compare

October 2, 2023 09:33

Member Author

tcitworld commented Oct 2, 2023

Should we add a repair job just like RegenerateBirthdayCalendars to fix all existing events? Or just edit the app config value from regeneratedBirthdayCalendarsForYearFix to something like regeneratedBirthdayCalendarsForAlarmFix so that it's triggered again?

Contributor

miaulalala commented Oct 2, 2023

Should we add a repair job just like RegenerateBirthdayCalendars to fix all existing events? Or just edit the app config value from regeneratedBirthdayCalendarsForYearFix to something like regeneratedBirthdayCalendarsForAlarmFix so that it's triggered again?

Repair step sounds good to me.

tcitworld force-pushed the dav-fix-birthday-sync branch 2 times, most recently from 3ba9335 to c2c2c8b Compare

October 2, 2023 15:12

miaulalala approved these changes

View reviewed changes

blizzz force-pushed the dav-fix-birthday-sync branch from c2c2c8b to 87c8581 Compare

October 11, 2023 13:33

skjnldsv mentioned this pull request

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29

tcitworld added 2 commits

February 1, 2024 18:29


          fix(dav): fix event birthday alarms not being updated

8939a2e

In #33251 the default offset for
a birthday event alarm was changed to 9AM on the day of the event.

However the birthdayEvenChanged method did not account for alarm
changes, so it was never propagated to existing birthday events, which
were kept on midnight.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>


          fix(dav): add a new config key to check to retrigger regenerating bir…

…thday calendars

So that birthday calendars are immediately updated and we don't need to wait for user to change a
card or disable/enable the calendar. We reuse the existing RegenerateBirthdayCalendars repair step
instead of adding a new one.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

tcitworld force-pushed the dav-fix-birthday-sync branch from 87c8581 to 4451809 Compare

February 1, 2024 17:29

Member Author

tcitworld commented Feb 1, 2024

Rebased

Member Author

tcitworld commented Feb 1, 2024

/backport to stable28

github-advanced-security bot found potential problems

View reviewed changes

apps/dav/lib/Migration/RegenerateBirthdayCalendars.php

               		// only run once
-              		if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes') {
+              		if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes' && $this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForAlarmFix') === 'yes') {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated

apps/dav/lib/Migration/RegenerateBirthdayCalendars.php

               		// only run once
-              		if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes') {
+              		if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes' && $this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForAlarmFix') === 'yes') {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated

apps/dav/lib/Migration/RegenerateBirthdayCalendars.php

@@ @@ -69,5 +56,6 @@ @@
               		// if all were done, no need to redo the repair during next upgrade
               		$this->config->setAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix', 'yes');
+              		$this->config->setAppValue('dav', 'regeneratedBirthdayCalendarsForAlarmFix', 'yes');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::setAppValue has been marked as deprecated

Member

skjnldsv commented Feb 23, 2024

psalm says

skjnldsv added 2. developing and removed 3. to review labels

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

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request

30.0.0 RC1 #47190

Merged

skjnldsv removed this from the Nextcloud 30 milestone

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

Labels

2. developing backport-request bug feature: caldav feature: carddav