Page MenuHome

Show ancestor path for sub-projects and milestones.
AcceptedPublic

Authored by Nathan Letwory (jesterking) on Mon, Sep 2, 11:09 AM.

Details

Summary

To understand better to what projects a sub-project or
milestone belongs show the entire ancestor path.

Solves part of T69306.

Diff Detail

Repository
rP Phabricator
Branch
jesterKing/T69306
Build Status
Buildable 4839
Build 4839: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested changes to this revision.Mon, Sep 2, 12:00 PM

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

This revision now requires changes to proceed.Mon, Sep 2, 12:00 PM
  • Clean up ancestor path implementation.
Nathan Letwory (jesterking) marked 3 inline comments as done.Mon, Sep 2, 11:10 PM

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

Sergey Sharybin (sergey) requested changes to this revision.Tue, Sep 3, 12:13 PM

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.

This revision now requires changes to proceed.Tue, Sep 3, 12:13 PM

Actually, after close inspection, i think getDisplayNameWithAncestorPath() needs more work to give same result as getDisplayName() for milestone.

Actually, after close inspection, i think getDisplayNameWithAncestorPath() needs more work to give same result as getDisplayName() for milestone.

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?

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.

  • Show entire path for all projects.
  • Render milestones as original getDisplayName.
  • Proper commenting style.
Nathan Letwory (jesterking) marked an inline comment as done.Mon, Sep 9, 5:53 AM

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.

This revision is now accepted and ready to land.Wed, Sep 18, 10:37 AM