Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15106: "Upload file" in remarkup text fields should attach by default

Commits

rP136091ecd097: Fix missing file attachment in Conpherence

Summary

When you drag and drop a file in a Conpherence chat,
the file was only visible by the author.

Now the file is also attached to that chat, making it visible.

This is a follow-up from:

https://we.phorge.it/D25705?id=2178

Refs T15106

Test Plan

open a Conpherence chat or create a new one
drag and drop file in it and send the message
verify file is attached to the chat

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

valerio.bozzolan created this revision.Jul 1 2024, 11:58

Owners added 1 blocking reviewer(s): O1: Blessed Committers.Jul 1 2024, 11:58

valerio.bozzolan requested review of this revision.Jul 1 2024, 11:58

Harbormaster completed remote builds in B1397: Diff 2186.Jul 1 2024, 11:58

valerio.bozzolan foisted this revision upon mturdus.Jul 1 2024, 11:58

valerio.bozzolan added a reviewer: valerio.bozzolan.

valerio.bozzolan mentioned this in D25710: [STUB] Fix missing file attachment in Conpherence.Jul 1 2024, 12:17

To avoid that $xactions[0] we should probably find the desired transaction.

Note: At the moment we lack a similar commodity method to find transactions, but that would be probably appreciated:

$xaction_comment = PhabricatorTransactions::findOneByType(
  $xactions,
  PhabricatorTransactions::TYPE_COMMENT);

Here what I'm talking about adopting findOneByType():

https://we.phorge.it/D25710?vs=2188&id=2189#toc

This new method "find transaction by type" opens some creative space to apply some minor refactors here and there:

https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php$1306-1312

https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php$5564-5570

https://we.phorge.it/source/phorge/browse/master/src/applications/maniphest/editor/ManiphestTransactionEditor.php$330-337

https://we.phorge.it/source/phorge/browse/master/src/applications/differential/editor/DifferentialTransactionEditor.php$38-46

and other things related to grep -F -C10 -R 'foreach ($xactions as $x' . | grep "if.*Type.*==" -C 4

src/applications/conpherence/controller/ConpherenceUpdateController.php
100	The `xactions[0]` is a bit hardcoded. In the future the `generateTransactionsFromText()` may return different elements, and the transaction related to comments may be in a different position. Maybe better to find the one with type `PhabricatorTransactions::TYPE_COMMENT`.

shorter like in D25705

Harbormaster completed remote builds in B1404: Diff 2193.Jul 1 2024, 13:32

P.S. Why does Phriction have the field text_metadata? And why does Conpherence have content_metadata? 🤔

In D25709#19616, @valerio.bozzolan wrote:

P.S. Why does Phriction have the field text_metadata? And why does Conpherence have content_metadata? 🤔

it's the other way around.
When you save a Phriction document, the content is stored in the content property.
There is also a content_metadata property created with this action, which contains the attachedFilePHIDs.
This is the network logging from Firefox for a Phriction wiki document update:

For Conpherence, the content is stored in a property named text and the file PHIDs are stored in the corresponding text_metadata.
Maniphest tasks have description and description_metadata.

OK.

@avivey what do you think about a new silly commodity method PhabricatorTransactions::findOneByType(array, string) ?

Maybe you have a strong opinion

( https://we.phorge.it/D25709#19587 )

valerio.bozzolan mentioned this in Z1: Phorge.Jul 1 2024, 17:14

Updates after comments

Harbormaster completed remote builds in B1407: Diff 2197.Jul 1 2024, 17:19

Updates after comments

Harbormaster completed remote builds in B1408: Diff 2198.Jul 1 2024, 17:28

mturdus marked an inline comment as done.Jul 1 2024, 17:29

Please wait at least for another +1, or, a week, before landing this. Since others may have different opinions on PhabricatorTransactions::findOneByType().

Thanks, tested, finally I share a file and people see it also in Conpherence 🌈 awesome

This revision is now accepted and ready to land.Jul 1 2024, 17:51

In D25709#19636, @valerio.bozzolan wrote:

OK.

@avivey what do you think about a new silly commodity method PhabricatorTransactions::findOneByType(array, string) ?

I've seen worse 🤷🏻‍♂️