Page MenuHomePhorge

Display warning about commenting on a task which is closed as a duplicate
Closed, ResolvedPublic

Description

Upstreaming from https://phabricator.wikimedia.org/T354769

Launchpad.net shows a warning above the comment field to logged-in users for tasks closed as a duplicate:

This task is a duplicate of T1234567. Comment here only if you think that it is not a duplicate.

This can make it clearer to users (who may have followed an anchor link and not see the task status at the top) not to fragment conversations, and thus save triagers' time explaining how things are supposed to work in Phorge.

Event Timeline

Maybe put the warning above the box, so it catches all actions, not just comments?

Maybe put the warning above the box, so it catches all actions, not just comments?

That's another option (and would allow additional text formatting). My personal preference for a placeholder text is probably based on not using additional screen estate, especially on small screens. The main question in any case though is how to get the current EditEngine in an acceptable (sufficiently performant) way.

I remain unhappy with my code in D25546:

  • Phorge lacks a method "give me all engines for this application".
  • PhabricatorApplication offers nothing related to engines.
  • PhabricatorEditEngine::getApplication() does what I want exactly the other way round: it returns the application for a given engine.
  • PhabricatorEditEngine::getAllEditEngines() is confusingly named. It only returns EditEngine keys like calendar.export or maniphest.task but not actual engines or engine classes. ($engines = id(new PhabricatorEditEngineQuery())->setViewer($this->getViewer())->execute(); returns the actual engines, as already used by this code.)
  • I cannot find some mapping between EditEngine keys (like maniphest.task) and either PhabricatorPHIDTypes (like ManiphestTaskPHIDType) or their TypeConstants (like TASK) either.
  • getEngineClassName() exists but only in a SearchEngine/SearchQuery context, not in a EditEngine content (and I cannot find its constructor)

I think the EditEngine is what's used to create the actions form, and it has some sense of the object's status (see for example the available actions on Revisions - these change based on the revision's state).
Maybe it can get an additional "field" for this warning, and display it based on task status.

Alternatively, the Task controller does have the full object, so it can add a warning in the right place.

Thanks for the pointers, I really appreciate them.

What is being passed around between the EditEngine and the CommentView is:

src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php:  public function setCommentActions(array $comment_actions) {
src/applications/transactions/editengine/PhabricatorEditEngine.php:    $view->setCommentActions($comment_actions);

That seems to be the closest I can get - $comment_actions is an array including for tasks {"owner":{},"status":{},"priority":{},"points":{},"std:maniphest:train.backup":{},"subtype":{},"projectPHIDs":{},"subscriberPHIDs":{},"mfa":{}} but does not "directly" allow to know which $object (e.g. ManiphestTask) was passed to final public function buildEditEngineCommentView($object) in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editengine/PhabricatorEditEngine.php$1625

So I am tempted to introduce setter/getter methods in PhabricatorApplicationTransactionCommentView for the corresponding EditEngine, and update the seven id(new PhabricatorApplicationTransactionCommentView()) calls in the codebase accordingly to pass an additional ->setEditEngine().
That seems to be the cleanest approach I can come up with.

I just discovered that sometime we can spawn warnings above the comment box.

Maybe in the future we may want to use this feature instead of the placeholder.

But hey, the placeholder does its job and I like it. Thanks again.

image.png (394×1 px, 33 KB)