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
jmeador - Group Reviewers
O1: Blessed Committers
Resulting query contains only PHID-USER identifiers.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- arcpatch-D25676
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 1909 Build 1909: 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?
Comment Actions
Uh there's also phid_group_by_type($type) for extra readability and less code. Also maybe more modular for future things.
| src/applications/differential/query/DifferentialRevisionQuery.php | ||
|---|---|---|
| 470 | This MAY need the idx trick: $authors_by_type = phid_group_by_type($this->responsibles); $responsible_authors = idx($authors_by_type, PhabricatorPeopleUserPHIDType::TYPECONST); Or something similar (sorry I'm from mobile) To do not cause an error if that array key is not available for unkown reasons | |
Comment Actions
Ah...I may take another look (jmeador: Feel of course also very free to commandeer this back to you)