Skip to content

Enable caldav push notifications by default - #40103

Merged
hamza221 merged 1 commit into
masterfrom
enh/enable-caldav-push-notifications-by-default
Sep 13, 2023
Merged

Enable caldav push notifications by default#40103
hamza221 merged 1 commit into
masterfrom
enh/enable-caldav-push-notifications-by-default

Conversation

@hamza221

Copy link
Copy Markdown
Contributor

Summary

Checklist

@tcitworld

Copy link
Copy Markdown
Member

Reverting everything, not just changing the default value of the setting?

@hamza221

Copy link
Copy Markdown
ContributorAuthor

Reverting everything, not just changing the default value of the setting?

In the issue #39862 the solution is reverting but I think this #39158 would be the next step

@tcitworld

Copy link
Copy Markdown
Member

I don't understand how it answers #39158, sorry. :(

@hamza221

Copy link
Copy Markdown
ContributorAuthor

I don't understand how it answers #39158, sorry. :(

it doesn't, it answers #39862

@GretaDGretaD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i only checked the vue and js changes

@hamza221
hamza221force-pushed the enh/enable-caldav-push-notifications-by-default branch from d84a49c to 5193cd5CompareAugust 31, 2023 13:09
?string $calendarDisplayName,
array $principalEmailAddresses,
array $users = []):void {
if ($this->config->getAppValue('dav', 'sendEventRemindersPush', 'no') !== 'yes') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

like @tcitworld said we should flip the default value but leave the option to turn the feature off

@hamza221
hamza221force-pushed the enh/enable-caldav-push-notifications-by-default branch from 5193cd5 to c9e5dbdCompareSeptember 8, 2023 14:32
@ChristophWurst

Copy link
Copy Markdown
Member

Failure of OCA\DAV\Tests\unit\CalDAV\Reminder\NotificationProvider\PushProviderTest::testNotSend and similar are related

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Time to squash

@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 12, 2023
Signed-off-by: hamza221 <hamzamahjoubi221@gmail.com>
@hamza221
hamza221force-pushed the enh/enable-caldav-push-notifications-by-default branch from 4d778a7 to 823908bCompareSeptember 12, 2023 11:58
@hamza221
hamza221 merged commit 4f0a151 into masterSep 13, 2023
@hamza221
hamza221 deleted the enh/enable-caldav-push-notifications-by-default branch September 13, 2023 08:50
@AndyScherzingerAndyScherzinger added this to the Nextcloud 28 milestone Sep 14, 2023
@ChristophWurstChristophWurst added the pending documentation This pull request needs an associated documentation update label Feb 6, 2024
@ChristophWurst

Copy link
Copy Markdown
Member

Needs admin docs for how to set sendEventRemindersPush and an entry in the admin upgrade section for 28

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

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancementfeature: caldavRelated to CalDAV internalsfeature: davpending documentationThis pull request needs an associated documentation update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable caldav push notifications by default

6 participants

@hamza221@tcitworld@ChristophWurst@miaulalala@GretaD@AndyScherzinger