To understand better to what projects a sub-project or
milestone belongs show the entire ancestor path.
Solves part of T69306.
Details
Details
- Reviewers
Sergey Sharybin (sergey)
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- jesterKing/T69306
- Build Status
Buildable 5067 Build 5067: arc lint + arc unit
Excuse: not my code
| Time | Test | |
|---|---|---|
| 1,149 ms | PhabricatorLibraryTestCase::testMethodVisibility Assertion failed, expected 'true' (at PhutilLibraryTestCase.php:146):
Class "ManiphestTaskQuery" implements method "loadPage" with the wrong visibility. The method has visibility "public", but it is defined in parent "PhabricatorCursorPagedPolicyAwareQuery" with visibility "protected". In Phabricator, a method which overrides another must always have the same visibility.
| |
| 358 ms | PhabricatorCelerityTestCase::testCelerityMaps 1 assertion passed. | |
| 17 ms | PhabricatorConduitTestCase::testConduitMethods 1 assertion passed. | |
| 0 ms | PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed. | |
| 0 ms | PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed. | |
| View Full Test Results (1 Failed · 29 Passed) |
Event Timeline
Comment Actions
What is the relation of this change with what Brecht did in D5521?
| src/applications/project/typeahead/PhabricatorProjectDatasource.php | ||
|---|---|---|
| 111–114 | This is what the mpull is for. From it's documentation: $names = array();
foreach ($objects as $key => $object) {
$names[$key] = $object->getName();
}
You can express this more concisely with mpull():
$names = mpull($objects, 'getName'); | |
| 116 | There are cheaper ways to check whether string is empty. | |
| 117 | There is no need for this. Just append $proj->getDisplayName() to the end of the array and run implode once, without any further exception code paths. You would then need need to rename variable to something like $project_name_path or something like this since it's no longer an ancestor. Also, should probably be pht(implode(...)) | |
Comment Actions
Thanks for the review, I've addressed the issues.
I was aware of the patch by @Brecht Van Lommel (brecht) in D5521. Probably better I merge this patch into D5521. The difference between the D5521 and this patch is that this gives the entire ancestor path as per request from @Dalai Felinto (dfelinto) .
Comment Actions
The getDisplayNameWithAncestorPath seems fine now.
The place where it is used seems a bit weird. As the patch stays currently you will only see a full path in the search results pop-up when tying in "Change Project Tags" and when you initially click on the results (because that, i believe, just renders the tag on the client). If you then save the changes the project tags on a side of, say task, will only show subproject name, without full path.
Don't think this is a desired behavior, and think Brecht's-like change in PhabricatorProjectProjectPHIDType->loadHandles is needed.
| src/applications/project/storage/PhabricatorProject.php | ||
|---|---|---|
| 596 | Comment is a full sentence, starting with a capital and ending with a full stop. | |
Comment Actions
Actually, after close inspection, i think getDisplayNameWithAncestorPath() needs more work to give same result as getDisplayName() for milestone.
Comment Actions
I initially did use $proj->getDisplayName(), but that resulted in having the parent project of a milestone being repeated unnecessarily.
A > B > 1 vs A > B > B (1).
@Sergey Sharybin (sergey) Do you suggest to do the latter? Or just A > B (1)? @Dalai Felinto (dfelinto) what is your preference?
@Dalai Felinto (dfelinto), also regarding showing of ancestors elsewhere - only direct parent, all ancestors, or as currently is?
Comment Actions
We shouldn't have discontinuity in a way how milestone is presented by getDisplayName() and getDisplayNameWithAncestorPath(), otherwise you're having confusing situation when same thing is presented differently in different places.
@Sergey Sharybin (sergey) Do you suggest to do the latter? Or just A > B (1)?
Sorry, I don't know what A, B and 1 is supposed to mean in your example. Using more descriptive mock names is always a great idea.
What i am suggesting is: Grand Parent > Parent > Project (Sprint 99), or Rendering > Cycles (Sprint 99).
@Dalai Felinto (dfelinto) what is your preference?
I don't think this is a correct phrasing of the question. Unless there is constructive disagreement with what happens in upstream, you should not diverge.
Comment Actions
The last changes should address all open issues.
Rendering the entire path in all places may be a bit much, but lets go with this for now. We can always tweak if the need arises.
Comment Actions
- Clean up ancestor path implementation.
- Show entire path for all projects.
- Render milestones as original getDisplayName.
- Proper commenting style.
Comment Actions
Indeed it seems fully working - that said I still don't know why we use ">" instead of "→".