Skip to content

Remove avatar on user deletion - #6668

Merged
MorrisJobke merged 3 commits into
masterfrom
fix_6621
Dec 11, 2017
Merged

Remove avatar on user deletion#6668
MorrisJobke merged 3 commits into
masterfrom
fix_6621

Conversation

@rullzer

@rullzerrullzer commented Sep 27, 2017

Copy link
Copy Markdown
Member

Fixes#6621

Basically I added a new event to the event dispatcher: OCP\IUser::preDelete
And then connected this to remove the avatar.

@rullzerrullzer added this to the Nextcloud 13 milestone Sep 27, 2017
ChristophWurst
ChristophWurst reviewed Oct 24, 2017
View reviewed changes
Comment threadlib/private/Server.php
$avatar = $manager->getAvatar($user->getUID());
$avatar->remove();
} catch (\Exception $e) {
// Ignore exceptions

@ChristophWurstChristophWurstOct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO would still make sense to log this, even if it's just level debug or info but if this code ever triggers errors nobody would notice and it makes this feature useless.

@rullzerrullzerOct 30, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair enough, let me do that.

@rullzerrullzerOct 30, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done

@blizzzblizzzNov 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could have removed the comment 🙊

@rullzerrullzerNov 2, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We still kind of ignore it. We just also log it :P

@codecov

codecovBot commented Oct 30, 2017•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #6668 into master will decrease coverage by 1.57%.
The diff coverage is 84.21%.

@@ Coverage Diff @@## master #6668 +/- ##
============================================
- Coverage 52.66% 51.09% -1.57% - Complexity 23542 24865 +1323 
============================================
Files 1439 1596 +157 Lines 80218 94608 +14390 Branches 0 1367 +1367 ============================================
+ Hits 42246 48339 +6093 - Misses 37972 46269 +8297
Impacted FilesCoverage ΔComplexity Δ
lib/private/Files/View.php84.38% <100%> (+0.16%)373 <0> (+2)⬆️
lib/private/Server.php81.93% <80%> (-0.06%)133 <6> (+5)
.../tests/Unit/Collaboration/CommentersSorterTest.php25.55% <0%> (-66.45%)6% <0%> (ø)
apps/sharebymail/tests/SettingsTest.php52.17% <0%> (-47.83%)3% <0%> (ø)
lib/private/Security/RateLimiting/Limiter.php55.55% <0%> (-44.45%)5% <0%> (ø)
settings/Controller/EncryptionController.php54.71% <0%> (-38.84%)8% <0%> (ø)
settings/Controller/GroupsController.php64.61% <0%> (-35.39%)9% <0%> (ø)
...ps/comments/tests/Unit/AppInfo/ApplicationTest.php69.56% <0%> (-30.44%)4% <0%> (ø)
apps/user_ldap/lib/Configuration.php42.02% <0%> (-27.86%)87% <0%> (ø)
apps/encryption/lib/Command/EnableMasterKey.php75% <0%> (-25%)5% <0%> (ø)
... and 342 more

ChristophWurst
ChristophWurst approved these changes Oct 30, 2017
View reviewed changes

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

@rullzerrullzer mentioned this pull request Nov 2, 2017
Closed
blizzz
blizzz reviewed Nov 2, 2017
View reviewed changes
Comment threadlib/private/Server.php
$dispatcher = $this->getEventDispatcher();

// Delete avatar on user deletion
$dispatcher->addListener('OCP\IUser::preDelete', function(GenericEvent $e) {

@blizzzblizzzNov 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Server container is not the proper place to put place this imho. Rather the AvatarManager.

@rullzerrullzerNov 2, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I can't put it in the avatarmanager as that is only called once we need the avatarmanager.

Like if you delete a user via the provisioning API. The avatarManager is never initialised. And as such there is nothing listening in there as well.

@blizzzblizzzNov 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, rights, that's why all the other listeners are registered in base.php :)

@ChristophWurstChristophWurstNov 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI: a service provider, as the Laravel framework calls them, would be nice in this case.

https://laravel.com/docs/5.5/providers

@rullzerrullzerNov 2, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes :P But a bit out of scope. I moved it for this to the server as base.php is a long huge list of code and functions. IMO this has more of a place in Server.php to link everything together than in base.php

@ChristophWurstChristophWurstNov 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes :P But a bit out of scope

I know. But well, maybe you feel highly motivated during a late night hacking session at the hackweek and want to integrate that :P

@blizzz

blizzz commented Nov 3, 2017

Copy link
Copy Markdown
Member

tests are failing

@MorrisJobkeMorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 9, 2017
@MorrisJobke

MorrisJobke commented Nov 9, 2017

Copy link
Copy Markdown
Member

@rullzer Ping

@rullzer

rullzer commented Nov 27, 2017

Copy link
Copy Markdown
MemberAuthor

@icewind1991 the errors happen in UpdaterLegacyTest because we try to access storage that isn't there. I'm not sure but it seems those tests to evil stuff. Can you have a look?

@MorrisJobke

MorrisJobke commented Dec 8, 2017

Copy link
Copy Markdown
Member

@icewind1991 the errors happen in UpdaterLegacyTest because we try to access storage that isn't there. I'm not sure but it seems those tests to evil stuff. Can you have a look?

🏓

@MorrisJobkeMorrisJobke mentioned this pull request Dec 8, 2017
Merged
28 tasks
@rullzer@icewind1991
Dispatch event on User predelete
949fc50
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer@icewind1991
Remove avatar on user deletion
10aa287
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@MorrisJobke

MorrisJobke commented Dec 11, 2017

Copy link
Copy Markdown
Member

@icewind1991 Still failing: PHP Fatal error: Call to a member function getStorage() on null in /drone/src/github.com/nextcloud/server/lib/private/Files/View.php on line 1365

@icewind1991
handle not found mounts when cleaning avatars
05d2f61
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991

icewind1991 commented Dec 11, 2017

Copy link
Copy Markdown
Member

all fixed

MorrisJobke
MorrisJobke approved these changes Dec 11, 2017
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 11, 2017
@MorrisJobke
MorrisJobke merged commit 4597187 into masterDec 11, 2017
@MorrisJobke
MorrisJobke deleted the fix_6621 branch December 11, 2017 17:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurstChristophWurstChristophWurst approved these changes
@blizzzblizzzblizzz left review comments
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@nickvergessennickvergessenAwaiting requested review from nickvergessen
@LukasReschkeLukasReschkeAwaiting requested review from LukasReschke

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugenhancement

Projects

None yet

Milestone

Nextcloud 13

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer@blizzz@MorrisJobke@icewind1991@ChristophWurst