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

Fixnextcloud/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

 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 masterFeb 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!

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!

schiessleFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsvFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsvFeb 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