T15074
Details
Details
- Reviewers
avivey speck - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- rP7d4357683a31: Hide the blurb of a user when that user is disabled
Disabled a user, then made sure their blurb disappeared.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Thank you for the review, @avivey !
I'm having trouble landing this, I keep getting 403 errors. I suspect it's a local configuration issue, though...
Comment Actions
I'm having trouble landing this, I keep getting 403 errors. I suspect it's a local configuration issue, though...
All that should be required to land is being in Blessed Committers I think, which you are a member of
| src/applications/people/storage/PhabricatorUser.php | ||
|---|---|---|
| 323 | Maybe include some comment/doc just indicating the intent of cleaning up the profile is to scrub away user-created content which will likely be spam from most of our disabled accounts. | |
Comment Actions
I did confirm that is the case. After some troubleshooting, I think my local Git install is not aware of my ssh key.
Comment Actions
Real quick before landing -- should this change be made here in PhabricatorUser or would it be sufficient in PhabricatorPeopleProfileController? Placing it here affects the profile at the data model source which would likely cause the same blurb-scrub in any other location it might render, but it might also cause problems in areas which need to access the profile data for other reasons other than rendering, e.g. if a profile gets copied/cloned in memory then this might result in losing the profile data altogether. Updating only PhabricatoPeopleProfileController to call cleanupProfile() instead of within PhabricatorUser would only scrub it at the time it's being rendered (to the profile page at least).
Comment Actions
Good question! I went with the PhabricatorUser route because it's entirely possible for the PhabricatorUserProfile object $this->profile to be null for a large part of the lifecycle of these page. I could clean the profile in the PhabricatorPeopleProfileController::buildProfileHeader() because that's where the profile is loaded... Would you rather that?
Comment Actions
Ah interesting. My own preference would be updating PhabricatoPeopleProfileController as I would associate this more as a view-level change but looking again at how this is structured I don't think it would cause any issues and I don't feel too strongly about changing it.
Comment Actions
fwiw, this is how I handled it in the wikimedia fork:
commit 8282af06e8948f7d3a9c197f10ea99b005e5c252
Refs: <release/2020-05-14/1>
Author: Mukunda Modell <mmodell@wikimedia.org>
AuthorDate: Fri May 1 04:53:08 2020 -0500
Commit: Mukunda Modell <mmodell@wikimedia.org>
CommitDate: Fri May 1 04:53:08 2020 -0500
Don't display user 'blurb' for disabled accounts.
Bug: T250946
---
src/applications/people/customfield/PhabricatorUserBlurbField.php | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/applications/people/customfield/PhabricatorUserBlurbField.php b/src/applications/people/customfield/PhabricatorUserBlurbField.php
index 222061ad0444..e0c1946619aa 100644
--- a/src/applications/people/customfield/PhabricatorUserBlurbField.php
+++ b/src/applications/people/customfield/PhabricatorUserBlurbField.php
@@ -83,6 +83,11 @@ final class PhabricatorUserBlurbField
}
public function renderPropertyViewValue(array $handles) {
+ // WMF Hack: Don't display profile of disabled users. See T250946
+ if ($this->getObject()->getIsDisabled()) {
+ return pht('This account has been disabled.');
+ }
+
$blurb = $this->getObject()->loadUserProfile()->getBlurb();
if (!strlen($blurb)) {
return null;