Page MenuHomePhabricator

When moving cards on workboards, treat before/after cards as optional hints, not strict requirements
ClosedPublic

Authored by epriestley on Mar 25 2019, 5:49 PM.

Details

Summary

Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one afterPHID or beforePHID and moves the card before or after the specified card.

This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.

In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.

In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:

You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.

...but this is pretty hostile and probably rarely/never what the user wants.

Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.

(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)

We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.

Test Plan
  • (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
  • (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mar 25 2019, 5:49 PM
epriestley requested review of this revision.Mar 25 2019, 5:51 PM
epriestley added inline comments.Mar 25 2019, 6:56 PM
webroot/rsrc/js/application/projects/WorkboardBoard.js
498

This is a bugfix for a minor issue I introduced during recent refactoring. It could cause certain drops to the top of a column above valid cards to omit "beforePHIDs" and end up in a slightly-wrong place.

This feels weird, because if anyone ever complains about this behavior we can reasonably claim to "fix" it by changing the code to just pass N+1 before/after PHIDs. I don't know the guts of this code very well, but why not more accurately capture the user's intent by passing "natural" column ordering IDs around instead of card PHIDs, so the drop target would be (column foo, index 12)?

The user's view of the column may be arbitrarily far out of date, and the position isn't "absolute position 12", it's "position 12 with such-and-such filter/order applied, in group G, possibly some time in the past".

Ignoring the time/out-of-date issue, we'd have to pass <columnPHID, filterKey, order, position> and then rebuild the whole query on the server. So this would be significantly more complicated, I think.

And even if we did this, when a user drops a card somewhere I think they don't really mean/expect "put it in position 12" -- they expect "put it after/before the cards I dropped it near".

Oh, plus policy filtering.

And D20322 does just "fix" this in the general case by passing N+99999 adjacent cards -- I think this is legitimately the most faithful thing we can do to match user intent, even though it seems a little weird.

The user's view of the column may be arbitrarily far out of date, and the position isn't "absolute position 12", it's "position 12 with such-and-such filter/order applied, in group G, possibly some time in the past".

That makes more sense. I'm just worried that this would be a net increase in spookiness: I can imagine repros that involve needing hundreds of cards, arranged Just So, and hitting exactly the wrong race across 15 different browser windows, where we just have to say 🤷 and hope it doesn't happen again. What if we changed this to "if the target card is gone, just put it in the default position"? That seems like it would capture most of the intent that this does, while still being able to reason about how a board ended up in a particular state.

epriestley added a comment.EditedMar 25 2019, 10:12 PM

If we do that (try one, default position otherwise), and have these columns:

SOON            LATER
A1              A2
B1              B2
C1              C2
D1              D2
E1              E2
F1              F2
G1              G2
H1              H2
I1              I2
J1              J2
K1

...and I drag E2 left to "Soon", near the bottom (say, between H1 and I1), and just before I do that someone drags H1 to "Later", my card ends up at the very top of "Soon".

With the ruleset here, it ends up between G1 and I1, which I think is probably least confusing / the best match for user intent.

Of course, if someone has dragged H1 to a different position in the same column (say, near the top), E2 could end up somewhere less-expected. But we have that problem in either case (whether we keep looking or not), since we'll still "stick to" H1 even if we only try one card. If this turns out to be spooky in practice, we could maybe refine the rule a bit and look for, say, two adjacent cards in the lists as a preferred position, or disregard positions which have changed recently, and try to refine the behavior.

This isn't necessarily perfect, but it gives us more tools to try to make guesses, and I think it's always better than "Fatal Exception" or "A card has moved to a different column, reload and try again".

amckinley accepted this revision.Mar 25 2019, 10:13 PM
This revision is now accepted and ready to land.Mar 25 2019, 10:13 PM
epriestley added a comment.EditedMar 25 2019, 10:19 PM

Another possible approach is that after T4900, we may be able to fast-forward the board and catch up. Then "the board has been updated" might be more okay, possibly, since we could fix it for you. But if you release your mouse button while we're doing that I think it will feel like we threw away your work to drag the card? Like this seems like it would feel bad:

  • You drag a card over a column.
  • The UI goes red and says "HOLD ON, CATCHING UP".
  • You have to hold it there for a while?
  • If you release the mouse button you have to go pick the thing up again?
  • A split second before you release the button we detect a change and reject the drop, sometimes?

So maybe this is a bad idea.

I guess we could force the update to happen on pickup, but then we're kind of in the same spot since you can spend as long as you want dragging the card around.

But maybe T4900 will be so good/consistent that it will basically moot any weird behavior here anyway.

If this feels spooky (and, particularly after whatever happens with T4900) we can take another look at it.

This revision was automatically updated to reflect the committed changes.