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.

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

Fix: Birthday calendar issue with shared calendars #31859

Merged
merged 1 commit into from Feb 20, 2023

Conversation

max65482
Copy link
Contributor

@max65482 max65482 commented Apr 6, 2022

Fixes #3914 in nextcloud/calendar.

My understanding is that {http://owncloud.org/ns}group-share is wrongly assigned. It is false although an address book is shared with a group.

This leads the BirthdayService to not update birthday calendars of group members. See

if ($share['{http://owncloud.org/ns}group-share']) {

I must admit that I don't really understand the rationale of {http://owncloud.org/ns}group-share. So please review carefully :)

@tcitworld
Copy link
Member

Seems a good idea but also seems to have a lot of side effects. 🤔

@max65482
Copy link
Contributor Author

max65482 commented Apr 6, 2022

I don't get how
'{http://owncloud.org/ns}group-share' => is_null($p)
was supposed to work in the first place.

{http://owncloud.org/ns}group-share indicates if something is shared with a group, right? In the error case, an address book was shared with a group and {http://owncloud.org/ns}group-share was false.

@max65482
Copy link
Contributor Author

seems to have a lot of side effects

I cannot find {http://owncloud.org/ns}group-share being used anywhere but in the BirthdayService (see my first comment) and tests.

@tcitworld
Copy link
Member

I was referring to the failed tests, didn't get a chance to look at this yet.

@max65482
Copy link
Contributor Author

Ok, fixed the problem. Let's see what the tests say now.

@tcitworld tcitworld requested review from a team, PVince81, skjnldsv, come-nc, tcitworld, ChristophWurst and miaulalala and removed request for a team April 18, 2022 12:09
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@demlak
Copy link

demlak commented Jun 25, 2022

i tried to implement it.. get this error:

[PHP] Error: Trying to access array offset on value of type null at /var/www/html/apps/dav/lib/DAV/Sharing/Backend.php#210

this is line 210 right now:
'{http://owncloud.org/ns}group-share' => substr_compare($p['uri'], 'principals/groups', 0, strlen('principals/groups')) === 0

@max65482
Copy link
Contributor Author

Are you sure you checked out the latest commit?
This looks like you are working with the previous commit. With that, the tests failed due to the null reference.
With the latest commit this is resolved.

@demlak
Copy link

demlak commented Jun 25, 2022

thx.. you are correct.. my fault

This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
apps/dav/lib/DAV/Sharing/Backend.php Outdated Show resolved Hide resolved
@szaimen szaimen added 3. to review Waiting for reviews bug labels Jan 19, 2023
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Small comment about the code, but I do not understand what this does and the implications.

apps/dav/lib/DAV/Sharing/Backend.php Outdated Show resolved Hide resolved
@miaulalala
Copy link
Contributor

/rebase

@blizzz blizzz mentioned this pull request Feb 1, 2023
@miaulalala
Copy link
Contributor

@max65482 please rebase :)

Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>
@max65482
Copy link
Contributor Author

Done!

@miaulalala miaulalala merged commit 6bb0985 into nextcloud:master Feb 20, 2023
@miaulalala
Copy link
Contributor

Thanks for the work @max65482 🚀

@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Birthdays: some show up, some don’t
7 participants