Page MenuHomePhabricator

Hard code a "close task" action on every column Trigger
ClosedPublic

Authored by epriestley on Mar 15 2019, 5:10 PM.

Details

Summary

Depends on D20287. Ref T5474. This hard-codes a storage value for every trigger, with a "Change status to <default closed status>" rule and two bogus rules. Rules may now apply transactions when cards are dropped.

Test Plan

Dragged cards to a column with a trigger, saw them close.

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 15 2019, 5:10 PM
epriestley requested review of this revision.Mar 15 2019, 5:11 PM
amckinley accepted this revision.Mar 18 2019, 7:39 PM
amckinley added inline comments.
src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php
4–5

Why does this get its own class? Are we going to be catching it specifically somewhere?

src/applications/project/storage/PhabricatorProjectTrigger.php
69

Ok this was unclear on my first pass through this, but in reality, $rule_specifications is going to be a JSON blob, right? Which this code is turning back into objects?

194–198

Shouldn't we have a way to expand this info?

src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php
11

Maybe add a "valid values for rules of this type are..." blurb?

src/applications/project/trigger/PhabricatorProjectTriggerRule.php
37–39

Shouldn't $this->getRecord()->getType() also be exposed? Or is that only used for construction of the rules and we won't need it again?

src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php
9

Unused field.

src/applications/project/trigger/PhabricatorProjectTriggerStatusRule.php
3 ↗(On Diff #48427)

Shouldn't this be PhabricatorProjectTriggerManiphestStatusRule? I know that Maniphest is the only application that has workboards and could therefore have columns and triggers, but wasn't it somewhere in the planning docs that we want to keep this appropriately generalizable?

This revision is now accepted and ready to land.Mar 18 2019, 7:39 PM
epriestley added inline comments.Mar 18 2019, 8:00 PM
src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php
4–5

Yeah, I'm planning to eventually catch it on the trigger edit page and say "this trigger is garbo, you can edit if you want but you're going to overwrite the old stuff if you do."

src/applications/project/storage/PhabricatorProjectTrigger.php
69

Right, this is simulating a value coming out of the DB. In the future, this will just be $rule_specifications = $this->getRuleset(). I just haven't written a way to write to that property yet, since it involves a bunch of JS.

194–198

You can click "View Rule" to get everything (well, in a future diff). This is just a little tooltip that appears if you hover over the trigger icon on the workboard.

src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php
11

This is currently only used in the tooltip, so I didn't want to get a paragraph of text in. When the edit part happens, that should get more details.

(This is going to have a Herald-style visual editor eventually, so it should be hard for users to specify invalid values -- really just extension/upgrade/manually-edit-the-db stuff.)

src/applications/project/trigger/PhabricatorProjectTriggerRule.php
37–39

Maybe, we just don't need it for anything right now. (Today, getValue() is only really for building transactions, and rules generally don't need to know their type to build transactions since the type is implicit in whichever class the method lives in.)

Normally, $this->getTriggerType() and $this->getRecord()->getType() will be the same. The only case where they differ is "Unknown" rules, where the getTriggerType() is "unknown" but the record type is the underlying storage record type ("some-extension-type" or whatever).

src/applications/project/trigger/PhabricatorProjectTriggerStatusRule.php
3 ↗(On Diff #48427)

Yeah, this is probably a good idea. I'm sort of unsure we'll ever actually generalize, but no reason to make it harder than needbe.

epriestley updated this revision to Diff 48490.Mar 25 2019, 6:52 PM
  • Remove unused property.
  • Rename "status" to be specific to Maniphest, just in case.
This revision was automatically updated to reflect the committed changes.