Address book data lost when any user receiving a share is deleted by skjnldsv · Pull Request #3473 · nextcloud/server

skjnldsv · 2017-02-14T08:24:26Z

Fix nextcloud/contacts#64
Reference: #1545

@nextcloud/contacts @cemrich @jeweloper @nickallevato

Steps to reproduce

Create user1 with an address book shared to group1

Create user2 of group1 to view the address book

Delete user2

Expected behaviour

User2 gets deleted but the shared address book of user1 remains intact.

Actual behaviour

The shared address book of user1 gets deleted. The tables oc_addressbooks and oc_cards are wiped of all information regarding this address book. If the given address book is the only one of user1, the "infinite loading bug" as described in #58 appears.

Server configuration

Operating system: ?

Web server: ?

Database: mysql 5.1.73

PHP version: 7.0.13

Nextcloud version: 11.0.0 (stable)

Contacts version: 1.5.2

Updated from an older Nextcloud or fresh install: fresh install

Signing status:
No errors have been found.
Are you using an external user-backend, if yes which one: no

Client configuration

Browser: Firefox 50.0.2 and Chrome 54.0.2840.99 m (64-bit)

Operating system: Windows 10

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

jancborchardt · 2017-02-14T09:23:27Z

Cc @nickvergessen @MorrisJobke @schiessle @blizzz for testing too since this is high impact :O

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

nickvergessen · 2017-02-14T11:57:52Z

Ouch, sounds valid and the diff makes sense. Is it the same for calendars? Or how is the situation handled there?

tcitworld · 2017-02-14T12:08:57Z

Calendar backend already has getUsersOwnCalendars which is properly used. Good catch.

skjnldsv · 2017-02-14T13:53:52Z

Yeah, I based my works on @tcitworld's previous pr :)
We need to backport too 😞

skjnldsv · 2017-02-14T13:54:23Z

Test fail unrelated. Restarted it.

schiessle · 2017-02-14T15:53:00Z

apps/dav/lib/CardDAV/CardDavBackend.php

 		return array_values($addressBooks);
 	}

+	function getUsersOwnAddressBooks($principalUri) {


please put a explicit "public" in front of the function. Seems like this file has a wired mixture here but I would like to keep it consistent with the other code.

Maybe it would also make sense to write a unit test for it?

There is already some unit tests for it in testDeleteCalendar. Is it enough?

And if you want bigger test, i'm sorry but I don't have the necessary knowledge to do correct ones here.
I'm too unfamiliar with the whole phpunit section here and don't have enough time to read to assimilate it :(

schiessle · 2017-02-14T15:53:57Z

Tested it.... Beside the small stuff I mentioned above the code looks good and it works as expected!

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

codecov-io · 2017-02-14T16:32:53Z

Codecov Report

Merging #3473 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3473      +/-   ##
============================================
+ Coverage     54.16%   54.16%   +<.01%     
- Complexity    21007    21185     +178     
============================================
  Files          1306     1306              
  Lines         80206    81105     +899     
  Branches       1255     1300      +45     
============================================
+ Hits          43441    43932     +491     
- Misses        36765    37173     +408

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/HookManager.php	`55.76% <100%> (ø)`	`15 <ø> (ø)`	❌
apps/dav/lib/CardDAV/CardDavBackend.php	`83.66% <100%> (+0.76%)`	`84 <3> (+3)`	✅
lib/private/legacy/template.php	`43.19% <ø> (-1.84%)`	`59% <ø> (+23%)`
lib/private/Files/Storage/Common.php	`73.73% <ø> (-1.69%)`	`200% <ø> (+70%)`
core/js/shareconfigmodel.js	`76.66% <ø> (-1.6%)`	`0% <ø> (ø)`
lib/private/Files/Cache/Propagator.php	`94.93% <ø> (-1.27%)`	`16% <ø> (ø)`
settings/templates/users/part.userlist.php	`0% <ø> (ø)`	`0% <ø> (ø)`	❌
settings/templates/personal.php	`0% <ø> (ø)`	`0% <ø> (ø)`	❌
settings/users.php	`0% <ø> (ø)`	`0% <ø> (ø)`	❌
config/config.sample.php	`0% <ø> (ø)`	`0% <ø> (ø)`	❌
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7113af3...7e9411b. Read the comment docs.

nickvergessen · 2017-02-14T21:11:05Z

Please also make a PR against stable11 so we can fix it before the release on friday, thanks!

public fixed

blizzz · 2017-02-15T10:35:04Z

@skjnldsv great stuff, thx! Could you also create a backport PR against stable11?

skjnldsv · 2017-02-15T15:01:12Z

Of course!

MorrisJobke · 2017-03-20T07:25:12Z

@skjnldsv @blizzz @schiessle @nickvergessen @tcitworld Would it make sense to adopt the stuff from upstream: owncloud/core#27169 ... it looks similar but seems to go a different way. Which code is better? Or should we just keep it how it is?

nickvergessen · 2017-03-20T10:37:48Z

I would keep ours, theirs looks incomplete to me.

getUsersOwnAddressBooks

b8fcd3c

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv added 2. developing Work in progress bug feature: dav high labels Feb 14, 2017

skjnldsv added this to the Nextcloud 12.0 milestone Feb 14, 2017

skjnldsv self-assigned this Feb 14, 2017

skjnldsv requested review from LukasReschke, MorrisJobke and tcitworld February 14, 2017 08:26

tests

36b8558

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 14, 2017

skjnldsv requested review from blizzz, jancborchardt and schiessle February 14, 2017 09:43

Test fix

57122e8

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

schiessle previously requested changes Feb 14, 2017

View reviewed changes

this is public

7e9411b

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv added the backport-request label Feb 15, 2017

blizzz merged commit 44186ca into master Feb 15, 2017

blizzz deleted the fix-addressbook-deletion branch February 15, 2017 10:34

skjnldsv mentioned this pull request Feb 15, 2017

[stable11] Address book data lost when any user receiving a share is deleted backport #3499

Merged

skjnldsv removed the backport-request label Feb 15, 2017

Uh oh!

Conversation

skjnldsv commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to reproduce

Expected behaviour

Actual behaviour

Server configuration

Client configuration

Uh oh!

jancborchardt commented Feb 14, 2017

Uh oh!

nickvergessen commented Feb 14, 2017

Uh oh!

tcitworld commented Feb 14, 2017

Uh oh!

skjnldsv commented Feb 14, 2017

Uh oh!

skjnldsv commented Feb 14, 2017

Uh oh!

schiessle Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

schiessle commented Feb 14, 2017

Uh oh!

codecov-io commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nickvergessen commented Feb 14, 2017

Uh oh!

blizzz commented Feb 15, 2017

Uh oh!

skjnldsv commented Feb 15, 2017

Uh oh!

MorrisJobke commented Mar 20, 2017

Uh oh!

nickvergessen commented Mar 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

skjnldsv commented Feb 14, 2017 •

edited

Loading

codecov-io commented Feb 14, 2017 •

edited

Loading