Page MenuHomePhabricator

Make pressing "R" on your keyboard reload the card state on workboards
ClosedPublic

Authored by epriestley on Jul 2 2019, 8:57 PM.

Details

Summary

Depends on D20638. Ref T4900. This is an incremental step toward proper workboard updates.

Currently, the client can mostly update its view because we do updates when you edit or move a card, and the client and server know how to send lists of card updates, so a lot of the work is already done.

However, the code assumes we're only updating/redrawing one card at a time. Make the client accept and process multiple card updates.

In future changes, I'll add versioning (so we only update cards that have actually changed), fix the "TODO" around ordering, and move toward actual Aphlict-based real-time updates.

Test Plan
  • Opened the same workboard in two windows.
  • Edited cards in one window, pressed "R" (capital letter, with no modifier keys) to reload the second window.
  • Saw edits and moves reflected accurately after sync, except for some special cases of header/order interaction (see "TODO").

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.Jul 2 2019, 8:57 PM
epriestley requested review of this revision.Jul 2 2019, 8:59 PM
amckinley accepted this revision.Jul 2 2019, 11:31 PM
amckinley added inline comments.
src/applications/project/engine/PhabricatorBoardResponseEngine.php
154

Debugging code, but I'm sure you're going to land this is as a big stack anyway.

webroot/rsrc/js/application/projects/WorkboardBoard.js
132–137

We're definitely going to get rid of this, right? https://xkcd.com/1172/

577–579

Can any of this interact weirdly with policy? If a card already rendered by my browser becomes not-visible to me, could I engage in silly buggers to disclose information about the now-restricted task? I'm guessing "no" since we're doing viewer checks server-side in PhabricatorProjectBoardReloadController, but stale information on the task will presumably stick around until I reload the page. I can't really call this an attack because at best it just means I can see the state of an object as of the last time I was legitimately able to view it.

Actually, won't the hidden card just stick around until I reload the page? If I can't see it, PhabricatorProjectBoardReloadController just won't return it in the reload result set, and it should stay where it is.

587

Oui oui!

667

Oh, re: my above, this will presumably remove cards from the UI if they weren't present in the result set.

This revision is now accepted and ready to land.Jul 2 2019, 11:31 PM
epriestley added inline comments.Jul 2 2019, 11:46 PM
webroot/rsrc/js/application/projects/WorkboardBoard.js
577–579

It definitely won't disclose any information, since the Query on the server side won't pull it.

You're right that we do end up with a ghosting issue though, see below...

667

We don't actually make it here since we're iterating over card_phid in response.cards, which no longer includes the deleted/hidden card!

I'll tweak things here to check both all the cards the client knows about and the cards the server gave us information about. This also has a slightly tricky interaction with edit flows if you move the card off the board by changing projects, or by dropping it into a column which kicks it off the board (which is silly, but I think currently allowed).

epriestley updated this revision to Diff 49239.Jul 3 2019, 5:14 PM
  • Remove debugging code.
  • Fix "oui oui".