Page MenuHome

Phabricator tweaks for bug triaging, code review and subprojects
AbandonedPublic

Authored by Sergey Sharybin (sergey) on Aug 18 2019, 6:21 PM.

Details

Summary
  • Maniphest: limit task reopen and setting priorities to developers/moderators
  • Menus: only show create task/project/repository buttons for developers/moderators
  • Projects: display parent hierarchy in project tags
  • Differential: exclude fields like subscribers from commit messages, to follow our commit guidelines
  • Differential: set git commit author for "arc patch" from user account, if "arc diff" was not used

This is a set of simple changes, the motivation behind them is here:
https://docs.google.com/document/d/1gxuBv6aeZjKgzMQmGYdaERKbd7VnllSa9s_ejl2hMGo/

Diff Detail

Repository
rP Phabricator
Branch
blender-tweaks
Build Status
Buildable 4485
Build 4485: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested changes to this revision.Aug 18 2019, 6:59 PM

Sorry, I can not accept this. This is a quick hack which:

  • Does not try to find a generic solution to be upstreamed.
  • Does not try to live within existing Phabricator design and adds hacks in obscure places.
  • Performs restrictions in the interface level, leaving "backdoors" in requests handlers (this is just how not to do development of something what faces the internets).

To be moving forward, i'd like to see:

  • Upstreamed changes as much as possible (as it leads to least amount of headache from our side).
  • All the rest of modifications living within the existing design (as it solves pain of merging upstream in, which redesigns backends every so often).

If you really think such quick hacks in our branch is the way to go, please find someone else to maintain the system.

src/applications/differential/field/DifferentialAuditorsCommitMessageField.php
45–46

I don't think this is a correct way of achieving the goal: Phabricator has a generic way of enabling/disabling fields.

The difference with those is that they are marked as "Permanent Fields". Perhaps not without a reasoning.

More correct way is to make those fields treated as every other one, without local hacks to hide them.

src/applications/differential/storage/DifferentialDiff.php
282–288

Phabricator treats e-mail as a personal data, which is not "leaking" without author's agreement.

We are violating this rule in a widely spread collaboration platform, without even having any contributor's agreement.

src/applications/maniphest/editor/ManiphestEditEngine.php
339–340

The code is not what the comment says: it forbids changing status between any open ones.
I had to dig into specific settings of our install to verify that it works correct.

375

This only limits UI? The direct POST and Conduit will still allow to triage?

src/applications/people/storage/PhabricatorUser.php
1597–1604

While it might be intended that anyone who is in project to have those "advance" buttons, but it can not be called "developer": we have documentation projects and artist members of projects.

Additionally, we only disallow people from joining to projects which are controlling push-ability to repositories. The rest of projects are more relaxed about joining to them. This means, users are free to join (as per default policy) and then all of a sudden their interface changes.

This revision now requires changes to proceed.Aug 18 2019, 6:59 PM

This took me maybe one hour to implement, and it's solving real problems that we are dealing with every day. I could spend days doing a more complex implementation, and maybe getting it accepted upstream. However reading upstream discussions around these topics, I expect at least half of these would not be accepted. And when it doesn't get accepted, the complex implementation will be harder to maintain.

The lifetime cost of maintaining these patches is probably less than it takes us to have this discussion. If was something like the password changes I could understand. But I'd rather be responsible for doing the Phabricator merges than either having to spend days working on a better implementation that will likely go nowhere, or having to continue explaining to users how they are using the bug tracker wrong and continue explaining to our developers how to format their commit messages.

I think these are important issues to solve, they have been a pain for years. Maybe @Nathan Letwory (jesterking) can take over maintenance of this.

src/applications/differential/storage/DifferentialDiff.php
282–288

It's not exposing the user's email. Rather it's filling in the email part of the git commit with the username, which is public. We have been doing this manually for years.

src/applications/maniphest/editor/ManiphestEditEngine.php
339–340

We have only one open status so this effectively does what the comment says, but the code can be changed so it matches the comment in case we add multiple open statuses in the future.

375

It's not a security feature, only something to guide users to do the right thing.

src/applications/people/storage/PhabricatorUser.php
1597–1604

The purpose is only the help new users report things in the correct way. I can rename developer to contributor if that helps.

Hiding a button is simple and is unlikely to have unexpected side effects, whereas changing actual permission is likely to affect things we don't expect.

Better naming and comments, checking of closed/open status.

However reading upstream discussions around these topics, I expect at least half of these would not be accepted.

The result of reading and conclusion of inevitability of doing local hacks is respected in the description of this review how?

src/applications/differential/storage/DifferentialDiff.php
282–288

Ah, indeed.

src/applications/maniphest/editor/ManiphestEditEngine.php
339–340

The point was: code was not doing what the comment said. Is fine to live within specifics of setup, but that is to be reflected in the comment as that looked totally suspicious and bogous.

The updated check looks fine.

375

This is a wrong explanation but with same outcome.
The conduit is actually "allow everything", even for the status rules happening in getTaskStatusMap: those are simply ignored.

This something for either corresponding commit message or for // NOTE below // Blender though.

378

Should be based on "triage" priority keyword.

src/applications/people/storage/PhabricatorUser.php
1597–1604

Think of it from the users perspective. They register, they see simple interface. Then they find some interesting project and join (and maybe the project is not even meant to be joinable, but because so far we were relaxed about this we did not harden policies of all projects) and then all of a sudden interface changes.

Well, fine. After some WTF moment they get fine with the interface, and even are using those "Contributor UI" features.

But then they will try to help new comers, trying to explain which button or thing to choose/press.

Even existing state is already confusing for users. And they are often asking about confusion in the chat. This change increases such confusion even further.

src/applications/project/phid/PhabricatorProjectProjectPHIDType.php
40

Is the readable name ignored from hashtags parsing? Are the hashtags still appear to be working?

src/applications/project/storage/PhabricatorProject.php
578
// If this is a milestone, show it as "Parent (Sprint 99)".

I do see the comment in an original function is wrong as well, but this isn't an excuse to spread the cancer.

Sergey Sharybin (sergey) commandeered this revision.

@Brecht Van Lommel (brecht), i'll commandeer this and push changes one by one.

With the desired behavior which we discussed here it's more matter of putting those into the comments, to avoid confusion in the future when need to deal with things like resolving conflicts on merge.

Committed in a162f18, c7a2133, 01fea69, remainder to be done in D5656.