Page MenuHomePhabricator

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

Authored by epriestley on Mar 15 2019, 5:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 19 2024, 11:35 PM
Unknown Object (File)
Feb 17 2024, 9:40 PM
Unknown Object (File)
Feb 15 2024, 3:10 AM
Unknown Object (File)
Feb 3 2024, 9:45 PM
Unknown Object (File)
Jan 25 2024, 1:55 AM
Unknown Object (File)
Dec 31 2023, 11:39 PM
Unknown Object (File)
Dec 29 2023, 10:01 AM
Unknown Object (File)
Dec 18 2023, 5:28 PM
Subscribers
None

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
Branch
trigger4
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/project/storage/PhabricatorProjectTrigger.php:68XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 22362
Build 30597: Run Core Tests
Build 30596: arc lint + arc unit

Event Timeline

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
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.

  • Remove unused property.
  • Rename "status" to be specific to Maniphest, just in case.
This revision was automatically updated to reflect the committed changes.