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

apps/dav/lib/CalDAV/BirthdayService.php

@@ -317,6 +317,11 @@
  }
 
  /**
+ * The birthday event is considered changed if either
+ * - the start date has changed
+ * - the title has changed
+ * - the time for the alarm has changed
+ *
  * @param string $existingCalendarData
  * @param VCalendar $newCalendarData
  * @return bool
@@ -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())
  );
  }
 

apps/dav/lib/Migration/RegenerateBirthdayCalendars.php

@@ -4,6 +4,7 @@
  *
  * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
  * @author Georg Ehrke <oc.list@georgehrke.com>
+ * @author Thomas Citharel <nextcloud@tcit.fr>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -30,36 +31,22 @@
 use OCP\Migration\IRepairStep;
 
 class RegenerateBirthdayCalendars implements IRepairStep {
+ private IJobList $jobList;
+ private IConfig $config;
 
- /** @var IJobList */
- private $jobList;
-
- /** @var IConfig */
- private $config;
-
- /**
- * @param IJobList $jobList
- * @param IConfig $config
- */
  public function __construct(IJobList $jobList,
  IConfig $config) {
  $this->jobList = $jobList;
  $this->config = $config;
  }
 
- /**
- * @return string
- */
- public function getName() {
+ public function getName(): string {
  return 'Regenerating birthday calendars to use new icons and fix old birthday events without year';
  }
 
- /**
- * @param IOutput $output
- */
- public function run(IOutput $output) {
+ public function run(IOutput $output): void {
  // only run once
- if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes') {
+ if ($this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForYearFix') === 'yes' && $this->config->getAppValue('dav', 'regeneratedBirthdayCalendarsForAlarmFix') === 'yes') {
  $output->info('Repair step already executed');
  return;
  }
@@ -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');
  }
 }

apps/dav/tests/unit/Migration/RegenerateBirthdayCalendarsTest.php

@@ -31,18 +31,17 @@
 use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
 use OCP\Migration\IOutput;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class RegenerateBirthdayCalendarsTest extends TestCase {
-
- /** @var IJobList | \PHPUnit\Framework\MockObject\MockObject */
+ /** @var IJobList | MockObject */
  private $jobList;
 
- /** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */
+ /** @var IConfig | MockObject */
  private $config;
 
- /** @var RegenerateBirthdayCalendars */
- private $migration;
+ private RegenerateBirthdayCalendars $migration;
 
  protected function setUp(): void {
  parent::setUp();
@@ -61,41 +60,47 @@ public function testGetName(): void {
  );
  }
 
- public function testRun(): void {
- $this->config->expects($this->once())
+ public function dataForTestRun(): array {
+ return [
+ ['', '', true],
+ ['yes', '', true],
+ ['yes', 'yes', false]
+ ];
+ }
+
+ /**
+ * @dataProvider dataForTestRun
+ */
+ public function testRun(string $yearFix, string $alarmFix, bool $run): void {
+ $this->config->expects($this->exactly($yearFix === '' ? 1 : 2))
  ->method('getAppValue')
- ->with('dav', 'regeneratedBirthdayCalendarsForYearFix')
- ->willReturn(null);
+ ->withConsecutive(['dav', 'regeneratedBirthdayCalendarsForYearFix'], ['dav', 'regeneratedBirthdayCalendarsForAlarmFix'])
+ ->willReturnOnConsecutiveCalls($yearFix, $alarmFix);
 
  $output = $this->createMock(IOutput::class);
- $output->expects($this->once())
- ->method('info')
- ->with('Adding background jobs to regenerate birthday calendar');
-
- $this->jobList->expects($this->once())
- ->method('add')
- ->with(RegisterRegenerateBirthdayCalendars::class);
 
- $this->config->expects($this->once())
- ->method('setAppValue')
- ->with('dav', 'regeneratedBirthdayCalendarsForYearFix', 'yes');
+ if ($run) {
+ $output->expects($this->once())
+ ->method('info')
+ ->with('Adding background jobs to regenerate birthday calendar');
 
- $this->migration->run($output);
- }
+ $this->jobList->expects($this->once())
+ ->method('add')
+ ->with(RegisterRegenerateBirthdayCalendars::class);
 
- public function testRunSecondTime(): void {
- $this->config->expects($this->once())
- ->method('getAppValue')
- ->with('dav', 'regeneratedBirthdayCalendarsForYearFix')
- ->willReturn('yes');
+ $this->config->expects($this->exactly(2))
+ ->method('setAppValue')
+ ->withConsecutive(['dav', 'regeneratedBirthdayCalendarsForYearFix', 'yes'], ['dav', 'regeneratedBirthdayCalendarsForAlarmFix', 'yes']);
+ } else {
+ $output->expects($this->once())
+ ->method('info')
+ ->with('Repair step already executed');
 
- $output = $this->createMock(IOutput::class);
- $output->expects($this->once())
- ->method('info')
- ->with('Repair step already executed');
+ $this->jobList->expects($this->never())
+ ->method('add');
 
- $this->jobList->expects($this->never())
-  ->method('add');
+  $this->config->expects($this->never())->method('setAppValue');
+ }
 
  $this->migration->run($output);
  }