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);
 	}