Pull the names of Priority field values instead of hardcoding them.
Closes T15799
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15799: Reports tooltip hardcodes Maniphest Priority field names
- 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
Diff Detail
- Repository
- rP Phorge
- Branch
- T15799repPrioTooltip (branched from master)
- Lint
Lint Passed Severity Location Code Message Advice src/applications/maniphest/controller/ManiphestReportController.php:706 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 1197 Build 1197: arc lint + arc unit
Event Timeline
Comment Actions
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 | ||
|---|---|---|
| 712 | 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 | |
| 719 | 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" | |