Page MenuHomePhorge

Do not hardcode default Priority names in Project Reports tooltip
ClosedPublic

Authored by aklapper on Apr 26 2024, 10:33.

Details

Summary

Pull the names of Priority field values instead of hardcoding them.

Closes T15799

Test Plan
  • As an admin, go to /config/edit/maniphest.priorities/ and change the value of "name" of a Priority value with a value < 50 (e.g.: Low, Wishlist)
  • Open /maniphest/report/project/
  • Hover over the Oldest (Pri) column and check the tooltip text before and after applying this patch

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Thanks!

Please evaluate extended names:

$low_priorities = array();
$priorities = ManiphestTaskPriority::getTaskPriorityMap();

...

$priority_v => $priority_label

Also please evaluate the creation of:

private function getNormalPriority() {
  // TODO: This is sort of a hard-code for the default "normal" status.
  // When reports are more powerful, this should be made more general.
  return 50;
}

So we can adopt that, removing both current TODOs. If you like that, let's micro-optimize, doing this before loops:

$normal_priority = $this->getNormalPriority();
src/applications/maniphest/controller/ManiphestReportController.php
716

In this way we have less need of pht(), and we avoid to have pht(' or ') that would have spaces to be kept in translation

722

Maybe better to have the placeholder in the end, so we have less need to make a meaningful readable phrase with "1, 2 or 3"

This revision is now accepted and ready to land.May 9 2024, 08:57

Improvicate some things that valerio mentioned. (I prefer "Average" to "Normal" though; I consider "normal" a problematic term in this context.)

src/applications/maniphest/controller/ManiphestReportController.php
709

Eheh, normal = average

src/applications/maniphest/controller/ManiphestReportController.php
709

Nuooooooo 😱 lol