Page MenuHomePhabricator

Allow triggers to be attached to and removed from workboard columns
ClosedPublic

Authored by epriestley on Mar 14 2019, 5:23 PM.

Details

Summary

Depends on D20286. Ref T5474. Attaches triggers to columns and makes "Remove Trigger" work.

(There's no "pick an existing named trigger from a list" UI yet, but I plan to add that at some point.)

Test Plan

Attached and removed triggers, saw column UI update appropriately.

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 14 2019, 5:23 PM
epriestley requested review of this revision.Mar 14 2019, 5:25 PM
amckinley accepted this revision.Mar 18 2019, 7:14 PM
amckinley added inline comments.
src/applications/project/controller/PhabricatorProjectColumnRemoveTriggerController.php
55–59

Maybe add a count of the number of other columns which currently use the trigger? I guess you'd have to do a policy-violating query to include the comfort-inducing "no other columns use this trigger" text, but I think that's pretty ok? Or inversely, just a warning "X other columns are currently using this trigger".

NB: I assumed this code was invoking destroyObjectPermanently; disregard.

src/applications/project/storage/PhabricatorProjectTrigger.php
108–124

This code isn't actually invoked by PhabricatorProjectColumnRemoveTriggerController, right? Also, pretty sure my above comment is now irrelevant.

src/applications/project/xaction/column/PhabricatorProjectColumnTriggerTransaction.php
7

Just out of curiosity, what's the scope on these constants? Do they need to be globally unique? Would we ever know if we reused one by accident? In practice I guess we scope transactions per-application since they have their own DBs anyway.

This revision is now accepted and ready to land.Mar 18 2019, 7:14 PM
epriestley added inline comments.Mar 18 2019, 7:49 PM
src/applications/project/controller/PhabricatorProjectColumnRemoveTriggerController.php
55–59

Yeah, this is just "Detach trigger from this column".

Possibly we should count and give the user an option to destroy it (or, more likely, banish it to the shadow realm). Elsewhere, I'm policy-violating on count/existence anyway to provide users a hint before they edit a trigger in use on a lot of boards they can't see, so I think this is fine from a policy perspective. It's generally okay to know that objects exist and what their PHIDs are, just rarely much more than that.

(For example, you can generally see all the edges attached to an object, even if D123 is attached to T123 which you can't see.)

src/applications/project/storage/PhabricatorProjectTrigger.php
108–124

Right, this is just so that when you bin/remove destroy <phid> a trigger we end up with minimally-weird column state since I don't really want to write all the UI for "this column references a trigger which no longer exists" and doubt the outcome would ever be any different from removing it, practically.

src/applications/project/xaction/column/PhabricatorProjectColumnTriggerTransaction.php
7

They only have to be unique per object type.

Some of the older stuff namespaces them heavily, but in practice all the transaction stuff is sharded into per-object tables anyway and there's no transaction.search mode which says "give me every subscription change to every object ever", nor is there likely to ever be, I think. I currently believe the handful of cases for this are probably better served by fact extraction in the Facts ETL pipeline, so we don't have to run 250 queries against every single whatever.transaction table.

epriestley added inline comments.Mar 25 2019, 6:49 PM
src/applications/project/controller/PhabricatorProjectColumnRemoveTriggerController.php
55–59

(In D20308, I ended up just counting use in active columns instead of having a separate "status".)

This revision was automatically updated to reflect the committed changes.