Page MenuHomePhabricator

Fix confusing ordering of similar actions in transaction groups
ClosedPublic

Authored by epriestley on Dec 5 2015, 6:08 PM.

Details

Summary

Fixes T7250. Currently, if a display group of transactions (multiple transactions by the same author in a short period of time with no intervening comments) has several transactions of similar strength (e.g., several status change transactions) we can end up displaying them in reverse chronological order, which is confusing.

Instead, make sure transactions of the same type/strength are always in logical order.

Test Plan
  • Merged a task into another task, then reopened the merged task.
  • Before patch: merge/reopen showed in wrong order.

Screen Shot 2015-12-05 at 10.04.04 AM.png (78×431 px, 16 KB)

  • After patch: merge/reopen show in correct order.

Screen Shot 2015-12-05 at 10.05.39 AM.png (77×440 px, 16 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix confusing ordering of similar actions in transaction groups.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Dec 5 2015, 6:20 PM
This revision was automatically updated to reflect the committed changes.
src/applications/transactions/view/PhabricatorApplicationTransactionView.php
388–389

what did I even write here

Maybe this would be easier as msort($group, array('getActionStrength', 'getId')) (with appropriate modifications to msort())

I think those modifications would need to involve usort(), which has huge overhead.

In most cases I add a getSortKey() and build a composite scalar, but I think getActionStrength() returns a float and I don't have a clever way to combine a float and an int into a single sortable scalar offhand that I trust. Something like %016.8f%012d probably works I don't know how many places the float needs offhand.

(We could also just make getActionStrength() return integers instead, which is probably more consistent and simpler.)