Page MenuHomePhabricator

Fix literally thousands of drag-to-reorder priority bugs
ClosedPublic

Authored by epriestley on Mar 21 2015, 12:11 AM.
Tags
None
Referenced Files
F14067499: D12121.diff
Tue, Nov 19, 3:17 PM
F14061676: D12121.diff
Mon, Nov 18, 7:45 AM
F14049476: D12121.id29145.diff
Thu, Nov 14, 1:15 PM
F14034972: D12121.id29145.diff
Sun, Nov 10, 3:14 AM
F13970704: D12121.id29146.diff
Oct 17 2024, 9:26 AM
F13967495: D12121.id.diff
Oct 16 2024, 1:58 PM
Unknown Object (File)
Oct 9 2024, 12:41 AM
Unknown Object (File)
Sep 17 2024, 3:58 PM
Subscribers
Tokens
"Mountain of Wealth" token, awarded by btrahan."Yellow Medal" token, awarded by chad."Mountain of Wealth" token, awarded by nicolast."Love" token, awarded by Taskle.

Details

Summary

Fixes T7563. Fixes T5201. Reframe this as two separate operations:

  • Move before or after a task.
  • Move to the beginning or end of a priority.

Then:

  • Make all the order queries unambiguous and properly reversible, with an explicit id order.
  • Just reuse ManiphestTask to get results in the correct order.
  • Simplify the actual transaction apply logic.
  • Detect and recover from cases where tasks have identical or similar subpriorities.
Test Plan
  • Wrote and executed unit tests.
  • Dragged and dropped tasks within priorities and between priorities in the main Maniphest view.
  • Dragged and dropped tasks within priorities in the workboard view, when ordered by priority.
  • Also poked at the "natural" order, but that shouldn't be affected.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix literally thousands of drag-to-reorder priority bugs.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
src/applications/maniphest/query/ManiphestTaskQuery.php
771–775

This change guarantees we get an explicit task.id order, which is important for reverse-order queries. MySQL will order by ID by default, but won't know how to reverse it.

1070–1072

This selection clause was not consistent with the ORDER clause.

chad added a reviewer: chad.
chad added a subscriber: chad.

My opinion means little, but this is a fine diff with many test cases that look a-ok to me.

This revision is now accepted and ready to land.Mar 21 2015, 12:27 AM

I'll land this if you arc patch it locally and can't break it either? I don't think @btrahan needs to look at it but you might be able to find something I missed.

(Or your local test tasks could be spookier than mine.)

patched and tested Maniphest, Workboards, and reordering Application Pins

This revision was automatically updated to reflect the committed changes.