Page MenuHomePhabricator

Let Herald know when workboard cards have been moved to another column
AbandonedPublic

Authored by epriestley on Nov 14 2016, 10:52 PM.

Details

Summary

Ref T5474. It wasn't possible to automatically do something when a workboard card had been moved to another column.

  • Create a Herald project column field and a respective typeahead datasource.
  • Make project column handles unambiguous by showing the project name as prefix.
  • Make project column handles even more distinguishable by using the project color.
  • Extend the project column query for datasource queries.
  • Add a "moved to" condition to the Herald adapter.
Test Plan
  • Created projects, subprojects, and milestones with and without workboards.
    • Added visible and hidden columns.
    • Tagged different tasks with one or more than one of these projects.
  • Created Herald rules with the new condition and exhaustively checked...
    • ...typeahead in field and browser both when creating and editing rules.
    • ...typeahead results for visual consistency with project column handles.
    • ...general Herald UI like the summary for rules.
  • Moved workboard cards between columns and checked rule transcripts/correct execution of actions.

Diff Detail

Repository
rP Phabricator
Branch
T5474-HeraldWorkboardCardMovedToAnotherColumnV2
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 14520
Build 18934: arc lint + arc unit

Event Timeline

kislinsk retitled this revision from to Let Herald know when workboard cards have been moved to another column.
kislinsk updated this object.
kislinsk edited the test plan for this revision. (Show Details)
kislinsk added reviewers: epriestley, chad.
kislinsk updated this revision to Diff 40598.
kislinsk updated this object.Nov 14 2016, 11:03 PM
kislinsk added inline comments.Nov 14 2016, 11:18 PM
src/applications/project/herald/HeraldProjectColumnField.php
20

I'm not sure if this is the right/a safe approach. Herald fields do not provide a viewer, though.

src/applications/project/query/PhabricatorProjectColumnQuery.php
137

Joining is a bit tricky here as the project column table and the project table have common table column names like name and phid. Without the alias, LiskDAO::loadFromArray() would mess up the created project column object with the wrong name.

201

LOWER() is necessary for the project column name as it hasn't a case-insensitive collation in contrast to the project name.

kislinsk updated this object.Nov 14 2016, 11:20 PM
kislinsk updated this revision to Diff 40632.Nov 15 2016, 11:49 PM
  • Executed arc liberate
  • Executed arc diff from Linux machine to get linters and unit tests to work
epriestley edited edge metadata.Nov 17 2016, 3:03 PM

Sorry, I think we gave you the wrong impression about the upstreamability of this change.

I want to pursue T5474 by following the plan outlined in T5474#107048, not by extending Herald to try to accommodate this use case.

If this is built through Herald, we can not build a "Require assignment" or "Require Triage" or "Require Points" or "Require Comment" action which pops up a dialog asking the user to choose an assignee, priority, point score, enter a comment, etc. Herald can not interact with the user, so it can't prompt the user to provide additional information to satisfy a trigger. I think these use cases are potentially quite useful, and want to be able to accommodate them.

We also can't reasonably or efficiently have the UI preview the effects of a drop if this is built with Herald. Users already find priority behaviors confusing in some cases (see T8135 and linked tasks for discussion) and letting a drop into a column do anything is likely to make this worse. I think we can make this more clear by previewing the effects of a drop in the UI before users release the task (e.g., show "Will change priority to High; Will close task" in a little hint below the card), but we can't do -- at least, not easily and not efficiently -- this if Herald takes the actions.

Perhaps most importantly, none of the templating features descried in that comment (to let you define one set of triggers and apply them to multiple columns or boards) will work with this approach in Herald. If you want many or all of your boards to have a "Done" column that auto-closes tasks which are dropped into it, this approach will require you to enumerate every "Done" column in the Herald rule and keep updating it every time you create a new project. I believe this will rapidly become unmanageable, and do not want to ship a product feature which requires users to do this. In the very best case, it will make a real implementation later more difficult. Realistically, it is likely to cause at least some confusion and direct support costs between now and then.

Another subtle issue here is that Herald reacts to state, but I think users will expect a drop trigger to react to a change in state. That is, you have written this condition as:

[ Workboard card ][ moved to any of: ]

But this is not the actual behavior of the condition, which I think could be more accurately described like this:

[ Workboard card ][ is in any column: ]

This is a problem because these rules will fire every time any edit is made to a task.

If a task is in two different columns on two different workboards, and both columns have some kind of behavior, adding a comment to the task will trigger both behaviors. In some cases this is fine (both rules do the same thing, or don't change the state); in other cases it will be bizarre and perplexing.

An extreme case of this is that you drag a task to an "open" column which closes it, because it's in a "close" column on a different workboard and the rules happen to apply in open + close order.

Overall, I think this pathway implementation pathway inherently leads to many confusing edge cases which we will not be able to make the UI explain. This approach is reasonable for users who know what they're doing, understand the limitations of the implementation, and are diligent and careful about writing and applying column-based Herald rules. But large organizations (and us, as a meta-support organization in general) have too many users for a feature with this level of complexity to be palatable.

I believe you can make a reasonable version of this change entirely as an extension, without requiring any part of it to be upstreamed. It would look something like this:

  • Use the existing STANDARD_PHID_LIST standard field type and existing conditions, following fields like "Project Tags", rather than defining a new condition.
    • Conceptually, it may help to relabel this as "Task's columns" instead of "Workboard card". This would result in "[ Task's columns ] [ include any of: ]", which aligns cleanly with existing standard field types and fields, and more specifically describes the actual behavior.
  • For performance, you may want to avoid calling array_merge() in a loop. See PHP Pitfalls for some discussion.
  • HeraldProjectColumnField is implemented incorrectly (or, at least, in a fragile way), and will return only one arbitrary column. There is no guarantee that this column has any particular properties, and particularly no guarantee that it is the column which the card was most recently moved to. We do not track this information and there is no reliable, robust way to retrieve it (except, I suppose, by reconstructing it from the transaction record). Instead, you should return a list of all columns the card belongs to.
  • You can do your own filtering and result construction in PhabricatorProjectColumnDatasource, at a small performance penalty.
  • You can accept the existing rendering of column handles at a moderate usability penalty, or just retain that smaller patch as a fork.

(Half that comment assumes that the field is rewritten to return a list; it doesn't apply with "return one arbitrary column", but I don't think that behavior is useful anyway.)

kislinsk added a comment.EditedNov 17 2016, 4:50 PM

Thank you very much for the detailed answer, @epriestley.

I understand that my approach has a major conceptional flaw as I wasn't aware of the state vs. change of state thing of Herald rules and that the result therefore must be a list instead of a single item. You're absolutely right. I also like the different concept of a "Task's columns" field instead of a "Workboard card" field, which feels more natural (when implementing the field, I was struggling with the decision, where this class should be located (maniphest or project) and how it should be named).

I agree that this feature can be implemented as an extension, which I actually did in the beginning and then switched to a contribution for the following reasons:

  1. The moved-to condition, which is obsolete regarding your naming suggestion
  2. Efficiency of the query, which is a nice to have but not necessary
  3. Naming/color of project column handles

The latter point is something, though, I really would like to still contribute/suggest, as the project column handles are currently kind of rudimentary and in contrast to any other type of handle ambiguous and therefore highly confusing for the users. My changes in PhabricatorProjectColumnPHIDType did really improve the user experience for us and I can highly recommend the change. What do you think?

To give a real world example: Without the handle change, I'm not even sure if I can implement this as an extension with "a moderate usability penalty" as you said, since it is impossible to differentiate between tons of blue-grey "Resolved" handles, instead of choosing between a yellow "Project A (Milestone 1): Resolved" and a green "Project B: Resolved" handle and so on.

edit: See my demo video in T5474 for a visual impression of the handles.

[...]
I agree that this feature can be implemented as an extension, which I actually did in the beginning and then switched to a contribution for the following reasons:
[...]

Hi @kislinsk, do you still have this functionality as an extension? If so, can you share it?

[...]
I agree that this feature can be implemented as an extension, which I actually did in the beginning and then switched to a contribution for the following reasons:
[...]

Hi @kislinsk, do you still have this functionality as an extension? If so, can you share it?

My extension is this revision. You can apply it with arc or use one of the download options above. I do not know if the diff still works without conflicts but Git will tell you. Also see the notes of epriestley above to understand why this revision was rejected in the end.

urzds added a subscriber: urzds.Sep 20 2017, 9:56 AM
pasik added a subscriber: pasik.Apr 24 2018, 11:14 AM
epriestley edited reviewers, added: kislinsk; removed: epriestley.Mar 21 2019, 7:08 PM
epriestley commandeered this revision.
epriestley abandoned this revision.

Presumably soon-to-be obsoleted by changes on T5474.