The responsibleUsers datasource returns the selected users as
well as all projects and owner packages they're members of. This is
useful for the reviewers constraint, but harms the authors
constraint. To fix this, limit authors additions to PHID-USER types.
Details
Details
- Reviewers
aklapper - Group Reviewers
O1: Blessed Committers
Resulting query contains only PHID-USER identifiers.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- j/fix-responsible-users (branched from master)
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 1312 Build 1312: arc lint + arc unit
Event Timeline
Comment Actions
@jmeador Hi, what would be the Test Plan to get a "resulting query"? Going to /differential/query/advanced/, selecting the magnifier button for the Responsible Users field, and checking that after applying the patch only user accounts are listed, and no more projects or packages, I assume?
Comment Actions
More specifically, examining DarkConsole service logs, you would notice a change in the query to differential_revisions:
Prior to the patch, the query would contain authorPHID in (PHID-USER-xxx, PHID-PROJ-xxxx) in the first part of the query, assuming the responsible user is a member of at least one Project. After the patch, the values list would only contain PHID-USER-xxx, eliminating project/owner packages from the query.
Comment Actions
Note that the responsible users' projects and packages should absolutely still be included in the reviewers query join (line 487 right-side). This is why $this->responsibles remains unmodified.
Comment Actions
Ah, thanks, makes sense now. I'd probably rephrase "but harms the authors constraint" a little bit to explicitly cover what you explained well in D25676#18549 - an author PHID in $this->responsibles might not always be a member of a project or owner of a package, so the query results were incomplete beforehand.
Also, I think defining $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; outside of the foreach loop would be slightly more performant?