Page MenuHomePhorge

Hide the blurb of a user when that user is disabled
ClosedPublic

Authored by Matthew on Mar 22 2022, 04:07.
Tags
None
Referenced Files
F184009: D25035.diff
Tue, Sep 27, 03:08
Unknown Object (File)
Tue, Sep 20, 15:37
Unknown Object (File)
Tue, Sep 20, 15:37
Unknown Object (File)
Tue, Sep 20, 15:37
Unknown Object (File)
Tue, Sep 20, 15:36
Unknown Object (File)
Mon, Sep 19, 00:15
Unknown Object (File)
Sat, Sep 10, 13:47
Unknown Object (File)
Fri, Sep 9, 10:14

Details

Reviewers
avivey
speck
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
Unknown Object (Maniphest Task)
Commits
rP7d4357683a31: Hide the blurb of a user when that user is disabled
Summary

T15074

Test Plan

Disabled a user, then made sure their blurb disappeared.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Matthew requested review of this revision.Mar 22 2022, 04:07
This revision is now accepted and ready to land.Mar 24 2022, 18:03
Matthew added a task: Unknown Object (Maniphest Task).Mar 24 2022, 22:10

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...

Macro shipit:

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.

Address code review comments

In D25035#1051, @speck wrote:

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

I did confirm that is the case. After some troubleshooting, I think my local Git install is not aware of my ssh key.

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).

In D25035#1059, @speck wrote:

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).

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?

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.

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;