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

Fix #31303: Make reminder notification behaviour selectable.. #31337

Merged
merged 2 commits into from
Mar 21, 2022
Merged

Fix #31303: Make reminder notification behaviour selectable.. #31337

merged 2 commits into from
Mar 21, 2022

Conversation

dzatoah
Copy link
Contributor

@dzatoah dzatoah commented Feb 24, 2022

Fixes #31303 and nextcloud/calendar#3946.

grafik

I'm open for your opinions. ^^
And please check my spelling, I'm not native. :D

@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 2 times, most recently from b065cb0 to 8e5d394 Compare February 24, 2022 13:25
tcitworld
tcitworld requested changes Feb 24, 2022
View reviewed changes
apps/dav/lib/CalDAV/Reminder/ReminderService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Reminder/ReminderService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Reminder/ReminderService.php Outdated Show resolved Hide resolved
@tcitworld tcitworld added 3. to review Waiting for reviews enhancement feature: caldav Related to CalDAV internals labels Feb 24, 2022
@tcitworld
Copy link
Member

Maybe we can add left-padding to this element to make it clear it belongs to the same group as above, like in the sharing admin settings?

@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 4 times, most recently from 27a0913 to e96c9f7 Compare February 24, 2022 15:09
@dzatoah
Copy link
Contributor Author

dzatoah commented Feb 24, 2022

Maybe we can add left-padding to this element to make it clear it belongs to the same group as above, like in the sharing admin settings?

grafik

Requested changes are made.

tcitworld
tcitworld reviewed Feb 24, 2022
View reviewed changes
apps/dav/src/views/CalDavSettings.vue Outdated Show resolved Hide resolved
apps/dav/src/views/CalDavSettings.vue Outdated
class="checkbox"
:disabled="!sendEventReminders">
<label for="caldavSendEventRemindersToSharedGroupMembers">
{{ $t('dav', 'Send reminder notifications not to shared group members.') }}
Copy link
Member

Choose a reason for hiding this comment

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

Might not be group shares but direct user-shares too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you phrase this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Send notifications to all shared users of an event and not only to the organizer and attendees. ?

Copy link
Member

Choose a reason for hiding this comment

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

Something like Send notifications to calendar sharees as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Send notifications to calendar sharees as well as to the organizer and attendees. ?

Copy link
Member

Choose a reason for hiding this comment

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

We can move the second part of the sentence above, something like : reminders are always sent to organizers and attendees.

Copy link
Contributor Author

@dzatoah dzatoah Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Like this? (Without the dot after well)

grafik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, should it be reminder notification instead of plain notification?

@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 3 times, most recently from c853af9 to cfe58dc Compare February 25, 2022 10:48
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 9 times, most recently from 8ed0994 to 27f5a0d Compare March 15, 2022 15:38
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 4 times, most recently from 86520f6 to 34f3022 Compare March 16, 2022 12:01
@dzatoah
Copy link
Contributor Author

dzatoah commented Mar 16, 2022

@tcitworld, @miaulalala, @ChristophWurst

I've had some problems with CI but think this PR is ready for another review now.

tcitworld
tcitworld reviewed Mar 17, 2022
View reviewed changes
apps/dav/src/views/CalDavSettings.vue Outdated Show resolved Hide resolved
apps/dav/src/views/CalDavSettings.vue Outdated Show resolved Hide resolved
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch from 34f3022 to 92aa0e2 Compare March 18, 2022 09:56
@dzatoah
Copy link
Contributor Author

dzatoah commented Mar 18, 2022

Hey @tcitworld,

Changes are done. Can you review again? ^^

tcitworld
tcitworld approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Just needs to test that the failing test is not a flaky one but LGTM overall.

apps/dav/src/views/__snapshots__/CalDavSettings.spec.js.snap Outdated Show resolved Hide resolved
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch from 92aa0e2 to 2bca603 Compare March 18, 2022 11:01
@dzatoah dzatoah requested a review from tcitworld March 18, 2022 11:40
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch 2 times, most recently from c256554 to 08e2f37 Compare March 21, 2022 11:43
@dzatoah
Fix nextcloud#31303: Make reminder notification behaviour selectable.
73a7ced 
Signed-off-by: Daniel Teichmann <daniel.teichmann@das-netzwerkteam.de>
@dzatoah
Add JS Unit tests for new 'sendEventRemindersToSharedGroupMembers' se…
f414882 
…tting.

Signed-off-by: Daniel Teichmann <daniel.teichmann@das-netzwerkteam.de>
@dzatoah dzatoah force-pushed the fix/issue-31303_irritating-event-reminder-behaviour branch from 08e2f37 to f414882 Compare March 21, 2022 12:03
@dzatoah
Copy link
Contributor Author

dzatoah commented Mar 21, 2022

Just needs to test that the failing test is not a flaky one but LGTM overall.

All checks pass now: LGTM? ^^

ChristophWurst
ChristophWurst approved these changes Mar 21, 2022
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.

LGTM

@ChristophWurst ChristophWurst merged commit 47dce35 into nextcloud:master Mar 21, 2022
@welcome
Copy link

welcome bot commented Mar 21, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@dzatoah dzatoah mentioned this pull request Mar 22, 2022
Merged
@ChristophWurst ChristophWurst mentioned this pull request May 30, 2022
Open
@dzatoah dzatoah mentioned this pull request Oct 10, 2022
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcitworld tcitworld tcitworld approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: caldav Related to CalDAV internals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Irritating behavior: Reminders get send to all users with write access within a shared group.
3 participants
@dzatoah @tcitworld @ChristophWurst