Page MenuHomePhorge

Fix missing file attachment in Phriction
ClosedPublic

Authored by mturdus on Jun 29 2024, 18:11.

Details

Summary

When you drag and drop a file in the Phriction editor and then save the
corresponding wiki document, the file was only visible by the author.

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

Refs T15106

Test Plan
  1. open the Phriction editor to edit an existing or create a new wiki document
  2. drag and drop file in it
  3. save wiki document
  4. verify file is attached to wiki document (other user can see file in wiki page)

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25705_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1405
Build 1405: arc lint + arc unit

Event Timeline

mturdus requested review of this revision.Jun 29 2024, 18:11

Wow ๐Ÿ˜ looks promising (untested from my side)

src/applications/phriction/controller/PhrictionEditController.php
123
125
141
src/applications/phriction/controller/PhrictionEditController.php
124
src/applications/phriction/controller/PhrictionEditController.php
147โ€“151

Also, better array() to follow project guidelines (eh... I personally love yours...)

mturdus added inline comments.
src/applications/phriction/controller/PhrictionEditController.php
124

If I change this to phutil_json_decode, the if statements below won't work correctly anymore as $content_metadata becomes an array instead of a stdClass

src/applications/phriction/controller/PhrictionEditController.php
124

Yep sorry, it becomes $content_metadata['attachedFilePHIDs'] ๐Ÿ‘

Fix file attachment in Conpherence

Summary:
when you drag and drop a file in a Conpherence chat, the file is only visible by the author.

Refs T15106

Test Plan:

  1. open the Conpherence chat and join a room
  2. drag and drop file in it
  3. send message
  4. verify file if file can be seen/downloaded by other user

(Do you need a little help to split the changes in two diffs?)

You may like also this:

$attached_phids = idx($metadata, 'attachedFilePHIDs', array());

Found here:

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

So just do isset($attached_phids) && is_array(...) etc.

Well, to be honest, feel free to generalize the description and commit message to include both changes, since they are the very same thing

mturdus retitled this revision from Fix file attachment in Phriction to Fix file attachment in Phriction and Conpherence.Jun 30 2024, 17:46
mturdus edited the summary of this revision. (Show Details)
mturdus edited the test plan for this revision. (Show Details)

(Do you need a little help to split the changes in two diffs?)

yes :-)

I ran arc diff and I thought it would create a 2nd diff revision.

OK try this to restore your desired situation (so, two diffs)

git stash save

git checkout -b just_phriction
git reset --hard origin/master
arc patch --nobranch --diff 2173
arc diff --update D25705

git checkout -b just_conpherence
git reset --hard origin/master
arc patch --nobranch --diff 2178
arc diff --create

Then you have this benefit that you can repeat as many times you want:

git checkout just_phriction
LOL DO CHANGES
arc diff

git checkout just_conpherence
LOL DO CHANGES
arc diff

Or, get fresh master again:

git checkout master

Ideally, the default assumes that people do not touch master but work in branches, each branch a different feature

valerio.bozzolan edited the test plan for this revision. (Show Details)

dedicating Conpherence its own diff as mturdus requested

we can remove an undeclared variable, and add a nonempty-str check

valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan retitled this revision from Fix file attachment in Phriction and Conpherence to Fix missing file attachment in Phriction.

Thanks my friend, this works perfectly

This revision is now accepted and ready to land.Jul 1 2024, 09:31

We can avoid phutil_nonempty_string() since PHP pitfalls "Check for non-empty strings" are under control here.

https://we.phorge.it/book/flavor/article/php_pitfalls/

Aaaaand... this was my last contribution here. Thanks again.

I

OK try this to restore your desired situation (so, two diffs)

git stash save

git checkout -b just_phriction
git reset --hard origin/master
arc patch --nobranch --diff 2173
arc diff --update D25705

git checkout -b just_conpherence
git reset --hard origin/master
arc patch --nobranch --diff 2178
arc diff --create

Then you have this benefit that you can repeat as many times you want:

git checkout just_phriction
LOL DO CHANGES
arc diff

git checkout just_conpherence
LOL DO CHANGES
arc diff

Or, get fresh master again:

git checkout master

Ideally, the default assumes that people do not touch master but work in branches, each branch a different feature

Thanks I will remember this for the future :-)

But since you have split these 2 up, I'm a bit confused now how I can land only the Phriction part.
I'm not that familiar with arc as you already might have noticed.

This is my current git status and show HEAD state:

gj@blackbird:/var/www/htdocs/phorge/phorge$ git status
On branch master
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean
gj@blackbird:/var/www/htdocs/phorge/phorge$ git show --name-only HEAD
commit 8e8372e2a79a226bd874858488c6738c784e87ee (HEAD -> master)
Author: Merula Turdus <merula.turdus@outlook.be>
Date:   Sat Jun 29 20:08:40 2024 +0200

    Fix file attachment in Phriction
    
    Summary:
    when you drag and drop a file in the Phriction editor and then save the
    corresponding wiki document, the file is only visible by the author.
    
    Refs T15106
    
    Test Plan:
    1) open the Phriction editor to edit an existing or create a new wiki document
    2) drag and drop file in it
    3) save wiki document
    4) verify file is attached to wiki document (other user can see file in wiki page)
    
    Reviewers: O1 Blessed Committers!
    
    Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
    
    Maniphest Tasks: T15106
    
    Differential Revision: https://we.phorge.it/D25705

src/applications/conpherence/controller/ConpherenceUpdateController.php
src/applications/phriction/controller/PhrictionEditController.php

Are these the correct steps:

git reset HEAD~1
git restore src/applications/conpherence/controller/ConpherenceUpdateController.php
arc diff --update D25705
arc land

If you would like to download the current version of D25705 to maybe land that, just:

arc patch D25705
arc land

Or, to just download the current version of D25705 to eventually add some local extra changes, just:

arc patch D25705

DO CHANGES
arc diff

DO CHANGES
arc diff

DO CHANGES
arc diff
This revision was automatically updated to reflect the committed changes.