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

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

Details

Reviewers

amckinley

Maniphest Tasks

T4900: Workboards updating in real time

Commits

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

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

Branch

refresh12

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/project/controller/PhabricatorProjectBoardReloadController.php:30	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 23108
Build 31727: Run Core Tests
Build 31726: arc lint + arc unit

Event Timeline

epriestley created this revision.Jul 2 2019, 8:57 PM

Harbormaster completed remote builds in B23094: Diff 49225.Jul 2 2019, 8:59 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).