Page MenuHomePhabricator

Allow herald rules to change task status in HeraldManiphestTaskAdapter Actions
Closed, ResolvedPublic

Description

Currently you can only detect a status on a task, but not set it by field conditions.

Basic idea behind that:
We have three groups working together:
One is responsible for validating incoming tasks - Duty: "Issue navigation"
The second one is fixing issues, inside the repository - Duty: "Coding"
The third one is meant to be a quality control instance, to make sure the code does on the target machine that had the initial issue.

However, for this to work, the restriction on repository commits, leading to "always closed" tasks, results in a requirement to move it out of the "resolved" state into another open state for the quality control team. (While on some projects, its okay, and wanted that the tasks are closed when a commit has occured.)

I assume a changed status on a task by herald, could even auto-reject tasks that break some productions rules or alike.

Related to: T7849 (description there)

Event Timeline

andypuettmann raised the priority of this task from to Needs Triage.
andypuettmann updated the task description. (Show Details)
andypuettmann added projects: Herald, Maniphest.
andypuettmann moved this task to Backlog on the Herald board.
andypuettmann added a subscriber: andypuettmann.

I've attached D14359 which is running in our local phab instance and mostly works. While the rule works as intended, the status doesn't want to display properly. I don't expect anyone to accept this as-is, but I thought I'd share as a starting point for anyone who has the time and knowledge to do it The Right Way.

When creating the rule, the entry box for status is rendered correctly, and auto-completes as expected. But when you save the rule, the status was being displayed as 'Unknown object (???)'. I modified the patch to display the key value instead, which at least lets you know what's configured.

And then when you re-edit the rule, the entry box now shows the status value as "Unknown" (looking at the source, it has the expected key but isn't being mapped correctly). If you delete the current value (or just add another one, see below) the status fields are mapped and rendered correctly.

The other issue, which seemed lower priority, is that it allows multiple statuses to be entered. I couldn't find an obvious way to limit it to a single status (and comments in ManiphestTaskAssignOtherHeraldAction.php make it seem like this is a known issue), but applyEffect() is only using the first status.

I'm also happy to fix up this patch myself, if anyone can give me some hints as to what I did wrong :)

epriestley claimed this task.

Oh, I should have read that comment more carefully. You tested more thoroughly than I did.

I'll see if I can fix the "Unknown Object" issue, it's not trivial and my comments on the revision don't address it.

I think it wasn't that bad thanks to a similar set of changes elsewhere from D14669, which I wrote yesterday.

The "only one token" thing is a known issue; it only affected one rule (I think?) before these two recent ones and I don't think it's a major usability issue (users are unlikely to enter multiple statuses, and I can't come up with a reasonable behavior users might expect entering multiple statuses or priorities to have) so I haven't bothered fixing it.