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

Add user setting to hide birthdays from deceased contacts #36327

Closed
wants to merge 1 commit into from
Closed

Add user setting to hide birthdays from deceased contacts #36327

wants to merge 1 commit into from

Conversation

tcitworld
Copy link
Member

TODO

  • Tests

Checklist

@tcitworld
feat(dav): Add user setting to hide birthdays from deceased contacts
c6a6af1 
Closes #24742

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld added 2. developing Work in progress feature: dav feature: caldav Related to CalDAV internals labels Jan 24, 2023
@tcitworld tcitworld added this to the Nextcloud 26 milestone Jan 24, 2023
@tcitworld tcitworld self-assigned this Jan 24, 2023
@tcitworld tcitworld requested a review from miaulalala as a code owner January 24, 2023 10:32
@tcitworld tcitworld marked this pull request as draft January 24, 2023 10:33
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 24, 2023
View reviewed changes
apps/dav/lib/CalDAV/BirthdayService.php
foreach ($datesToSync as $type) {
$this->updateCalendar($cardUri, $cardData, $book, (int) $calendar['id'], $type, $reminderOffset);
$this->updateCalendar($cardUri, $cardData, $book, (int) $calendar['id'], $type, $reminderOffset, $showBirthdaysFromDeceasedContacts);

Check failure

Code scanning / Psalm

InvalidArgument

Argument 7 of OCA\DAV\CalDAV\BirthdayService::updateCalendar expects bool, but null|string provided
apps/dav/lib/CalDAV/BirthdayService.php
* PT9H) or null if no reminder is wanted.
*
* @param string $userPrincipal
* @return string|null

Check failure

Code scanning / Psalm

MismatchingDocblockReturnType

Docblock has incorrect return type 'null|string', should be 'bool'
ChristophWurst
ChristophWurst reviewed Jan 25, 2023
View reviewed changes
apps/dav/appinfo/info.xml
@@ -62,6 +62,7 @@
<settings>
<admin>OCA\DAV\Settings\CalDAVSettings</admin>
<personal>OCA\DAV\Settings\AvailabilitySettings</personal>
<personal>OCA\DAV\Settings\PersonalCalendarSettings</personal>
Copy link
Member

Choose a reason for hiding this comment

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

#34626 (comment) you were right

@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@blizzz blizzz mentioned this pull request Mar 7, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
Merged
Closed
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
Merged
This was referenced Mar 14, 2024
Merged
Merged
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@tcitworld
Copy link
Member Author

tcitworld commented Aug 26, 2024 •
edited
Loading

I'm guessing this kind of setting makes more sense in Calendar anyway? @nextcloud/designers @nextcloud/calendar @skjnldsv
Context: #24742

@skjnldsv
Copy link
Member

Probably yes, https://github.com/nextcloud/calendar/blob/3288a29eadab24cd8341b550c6ad6ccb0e23afd3/src/components/AppNavigation/Settings.vue#L11-L16

@skjnldsv skjnldsv deleted the add-setting-to-hide-birthday-deceased-contacts branch August 27, 2024 09:01
@jancborchardt
Copy link
Member

@tcitworld if it would be a general setting like "Show birthdays after death" then yeah, Calendar makes sense.
However it’s a bit weirdly specific, and also you might want to see the birthday of people close to you still, while not for others.

So since this general setting would rely on the "Death date" being set in the contact anyway, this should ideally be a per-contact setting in contacts. When "Death date" is set, there can be a checkbox "Show birthday in calendar" below.

What do you think?

@tcitworld
Copy link
Member Author

So since this general setting would rely on the "Death date" being set in the contact anyway, this should ideally be a per-contact setting in contacts. When "Death date" is set, there can be a checkbox "Show birthday in calendar" below.

@jancborchardt We already have the feature to exclude specific contacts from the birthday calendar. This is about having a global setting to avoid setting this for all deceased contacts manually. It may not be needed, and we can just close #24742.
image

@jancborchardt jancborchardt mentioned this pull request Sep 13, 2024
Closed
@jancborchardt
Copy link
Member

@tcitworld thanks! Continued at #24742 (comment)

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

@ChristophWurst ChristophWurst ChristophWurst left review comments

@miaulalala miaulalala Awaiting requested review from miaulalala

@st3iny st3iny Awaiting requested review from st3iny st3iny will be requested when the pull request is marked ready for review st3iny is a code owner

@SebastianKrupinski SebastianKrupinski Awaiting requested review from SebastianKrupinski SebastianKrupinski will be requested when the pull request is marked ready for review SebastianKrupinski is a code owner

Assignees

@tcitworld tcitworld

Labels
2. developing Work in progress feature: caldav Related to CalDAV internals feature: dav
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Birthdays continue to be shown in calendar after death
5 participants
@tcitworld @skjnldsv @jancborchardt @ChristophWurst @blizzz