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

Branch

arcpatch-D25705

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1397
Build 1397: arc lint + arc unit

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 🤷🏻‍♂️