Do not hardcode default Priority names in Project Reports tooltip
ClosedPublic
Actions

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

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15799: Reports tooltip hardcodes Maniphest Priority field names

Commits

rPe6925c83c187: Do not hardcode default Priority names in Project Reports tooltip

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

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

aklapper created this revision.Apr 26 2024, 10:33

Owners added a reviewer: O1: Blessed Committers.Apr 26 2024, 10:33

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptApr 26 2024, 10:33

aklapper requested review of this revision.Apr 26 2024, 10:33

Harbormaster completed remote builds in B1197: Diff 1895.Apr 26 2024, 10:33

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"