Page MenuHomePhorge

Warn in comment field if task is closed as duplicate
ClosedPublic

Authored by aklapper on Feb 29 2024, 17:07.

Details

Summary

Display a placeholder text in the text comment field of a Maniphest task if the task status has been set to Duplicate.
This makes it clearer to users (who may have not checked the task status at the top of the page) not to fragment conversations.

Closes T15749

Test Plan
  • Be logged in and go to a task which is closed as a duplicate and see the placeholder text in the field to add a comment.
  • Be logged in and go to tasks which are not closed as a duplicate and see no placeholder text in the field to add a comment.
  • Go to other places whose code calls a PhabricatorApplicationTransactionCommentView constructor and check that it still renders correctly, for example Ponder in http://phorge.localhost/Q1, Slowvote in http://phorge.localhost/V1, Differential in http://phorge.localhost/D1

Diff Detail

Repository
rP Phorge
Branch
warnDuplicate (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1082
Build 1082: arc lint + arc unit

Event Timeline

I like the result. Do you also think that the phrase is probably a bit too much in serious business mode? 🤔 Any idea about how to make it a bit more soft?

Anyway maybe we can wrap in pht('...')

I also don't get if in this part we have access to any thing related to Maniphest, ManiphestEditEngine or this kind of stuff, to just call $something->getCommentPanelPlaceholder() and have the business logic there in the specific application itself.

This revision is now accepted and ready to land.Feb 29 2024, 20:13
src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
304

Have you already tried this?

I also don't get if in this part we have access to any thing related to Maniphest, ManiphestEditEngine or this kind of stuff, to just call $something->getCommentPanelPlaceholder() and have the business logic there in the specific application itself.

I fail to find anything in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php which would allow us to know in which Phorge application this comment field is rendered :(

I also don't get if in this part we have access to any thing related to Maniphest, ManiphestEditEngine or this kind of stuff, to just call $something->getCommentPanelPlaceholder() and have the business logic there in the specific application itself.

Yeah, let me refactor this. It'll be more lines of code but way cleaner and expandable.

I see quite some potential in our downstream instance where folks do not get exposed to hints in a Form Preamble when commenting on an existing task, and will not always find and read hints in a custom red box manually added to a task description either, while this comment field placeholder will be nice automated server-side logic.

Abstract logic: Move application specific conditions to display a comment field placeholder text into the corresponding EditEngine of the object that the comment field is displayed in, by defining a public getCommentFieldPlaceholderText() function.

Once acceptable, this patch should likely be split into two separate patches: One for the general placeholder display functionality in PhabricatorApplicationTransactionCommentView, one for application-specific conditions and placeholder string (in this use case, ManiphestEditEngine for tasks).

Test by going to a task closed as a duplicate and looking at the comment field. Also test by replacing the final return ''; in ManiphestEditEngine with a string and going to any other task not closed as a duplicate.

aklapper requested review of this revision.Apr 8 2024, 11:01
aklapper marked an inline comment as done.

Ouch I see that receiving the engine was very complicated.

I wonder how this was used. I mean. If we have something like this:

(new PhabricatorApplicationTransactionCommentView())
  ->render();

Maybe we can add our dependency:

(new PhabricatorApplicationTransactionCommentView())
  ->setEngine($engine)
  ->render();

But this is just a stupid idea - and I'm sorry that this is becoming so time expensive

I guess another question is whether to define an abstract public function getCommentFieldPlaceholderText() in parent PhabricatorEditEngine and then add to all and every child EditEngine a default public function getCommentFieldPlaceholderText($object_phid) { return '' };. Feels cleaner to me.

and I'm sorry that this is becoming so time expensive

Nah, I see some potential in this, so I wanted to have the code properly split between general functionality implementation and per-app conditions and behavior. :)

Maybe we can add our dependency:

(new PhabricatorApplicationTransactionCommentView())
  ->setEngine($engine)
  ->render();

ManiphestEditEngine does not instantiate a PhabricatorApplicationTransactionCommentView directly, it goes via the parent class PhabricatorEditEngine...

Use phid_get_type instead of substr as it does the same job

I remain unhappy with this code.

  • 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)

Allow to setEditEngine when constructing a PhabricatorApplicationTransactionCommentView

After all the handstands in the previous attempts, the costs of this approach are only an unconditional single $this->getEditEngine() call in PhabricatorApplicationTransactionCommentView everytime the <textarea> comment panel is rendered, and only in case of rendering it in a Maniphest task another two checks plus passing the EditEngine object in the constructor call.

I still don't like the solution of using place-holder text for that; But this implementation also adds another DB access which I think is redundant.

src/applications/maniphest/editor/ManiphestEditEngine.php
78 ↗(On Diff #2113)

How come we don't already have the task instance on-hand at this point? The extra search will cost us.

src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
303

method_exists can be replaced with a concrete method in the base class.

Thanks for the review (and for your patience!).

But this implementation also adds another DB access which I think is redundant.

Right. We do not have the actual $object (=task) itself in ManiphestEditEngine or its parent PhabricatorEditEngine. It's either passed as a method parameter or there is code like $object = $this->newEditableObject(); $this->editEngineConfiguration = $config; to get the environment (e.g. which action dropdown items to render).

The function which constructs the PhabricatorApplicationTransactionCommentView in PhabricatorEditEngine does have the $object so I'll pass it down (together with the EditEngine) to PhabricatorApplicationTransactionCommentView which then passes the $object as a call parameter in getCommentFieldPlaceholderText(ManiphestTask $task).

As introducing {get|set}Object() overlapped with previously existing {get|set}ObjectPHID() within PhabricatorApplicationTransactionCommentView, I also replaced the two getObjectPHID() calls within PhabricatorApplicationTransactionCommentView with getObject()->getPHID() and passing the $object instead of its PHID in the PhabricatorApplicationTransactionCommentView constructor calls in five classes.

I did not add an additional if ($object instanceof ManiphestTask) check in ManiphestEditEngine as that felt redundant.

I was thinking of making getCommentFieldPlaceholderText($object) an abstract method in parent PhabricatorEditEngine but that would require adding the function to 54 other EditEngine classes just to return an empty string, so I refrained from such little gain.

I still don't like the solution of using place-holder text for that

I'm merely after not using more limited space on small screens (with the obvious limitation that the placeholder text must be plain text) plus once you've read the placeholder text you are done with interacting with it so it can go away once you start typing in the comment textarea field.
But if you prefer/insist that this should be rendered as a PHUIInfoView instead I can look into that - this small feature is not a hill I plan to die on. :)

Screen Shot 2024-06-18 at 11.52.32.png (600×400 px, 35 KB)

Remove a needless ->setObjectPHID($object_phid) call I missed now that we pass $object

Also remove now unused private $objectPHID

Looks good to me.

Once this lands, I'll update this instance so we can see if it breaks anything before next Stable (also I should officially release the last RC).

I'll see how I feel about the placeholder over time :)

nice work!

This revision is now accepted and ready to land.Jun 19 2024, 07:33