Page MenuHomePhorge
Differential D25475

Drag & Drop Task Cover Image: also attach
AcceptedPublic
Actions

Authored by valerio.bozzolan on Nov 20 2023, 09:06.
Tags
None
Referenced Files
F2166216: D25475.id1613.diff
Fri, Apr 26, 22:08
Unknown Object (File)
Fri, Apr 26, 12:33
Unknown Object (File)
Thu, Apr 25, 05:23
Unknown Object (File)
Thu, Apr 25, 02:35
Unknown Object (File)
Thu, Apr 25, 02:35
Unknown Object (File)
Thu, Apr 25, 02:34
Unknown Object (File)
Wed, Apr 24, 22:23
Unknown Object (File)
Wed, Apr 17, 18:15
View All 67 Files
Subscribers
avivey
Cigaryno
Matthew
speck
tobiaswiese
Tokens
"Mountain of Wealth" token, awarded by 20after4.

Details

Reviewers
avivey
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15703: Workboard Cover Image: the cropped "Transform" is orphan
T15163: The Task Cover Image is visible only to the uploader (invisible to Task participants)
Summary

After this change, the cover image is finally always visible to others (to people who can see the Task)
instead of being visible only to yourself.

Example conversation before this change:

  • Anna: (uploads a nice Cover Image on the Task, using drag & drop from the Workboard)
  • Bob: Thanks! But I cannot see that image. Please change visibility.
  • Anna: Ouch! How?
  • Bob: I don't know. Try Files > your File > Edit > Set Visibility > Something that includes "Bob".
  • Anna: Done. Do you see my image now? (Spoiler: it's a green pepper)
  • Bob: Yes and no. I mean, I can see the original file, but not from the Workboard.
  • Anna: Uh? Have you tried refreshing your cache?
  • Bob: Yes. Still invisible from Workboard.
  • Anna: Maybe you have to change Defaults Permissions of the "Files" Application, to have "All Users"
  • Bob: That is a bit invasive. Let's try. I changed the Files policy globally. Try re-uploading your image again.
  • Anna: Done. Do you see it now?
  • Bob: Yes! I can finally see it now, also from the Workboard.

Example conversation after this change:

  • Anna: (uploads a nice Cover Image on the Task, using drag & drop from the Workboard)

Done! So, after this change, everything works as expected. Every task participant can see that image,
and nobody starts a nonsense conversation, and nobody needs to do random experiments with
default application policies.

So, after this change:

  • the original file is attached to the Task
  • the cover image is not orphan anymore, but mentioned in the "Transforms" of the original file
  • the cover image so now inherits the original file policies

So, Task participants can finally see that cover image (transform), since it comes from a file,
and that file is attached to the Task, and since they can see that Task.

For a micro-optimization, this change refactors a bit PhabricatorFile.

You can still work in object-oriented:

$file->attachToObject($task_phid);

But now you can also work directly with PHIDs, if necessary:

PhabricatorFile::attachFileToObject($file_phid, $task_phid)

This patch somehow improves T15768.

Closes T15163
Closes T15703

Test Plan

Visit a Workboard with a Column and a Task. Then:

  1. Drag & Drop GNU.png from your computer to that Task
  2. Drag & Drop GNU.png again there
  3. Drag & Drop Linux.png to that Task
  4. Drag & Drop Linux.png again
  5. No nuclear implosion.
  6. In all cases Task participants can see your Cover Image.

Diff Detail

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

Event Timeline

valerio.bozzolan created this revision.Nov 20 2023, 09:06
Owners added a reviewer: O1: Blessed Committers.Nov 20 2023, 09:06
Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptNov 20 2023, 09:06
Harbormaster completed remote builds in B948: Diff 1531.Nov 20 2023, 09:06
valerio.bozzolan requested review of this revision.Nov 20 2023, 09:06
valerio.bozzolan planned changes to this revision.EditedNov 22 2023, 12:10

This partially fixes the problem:

  • the file is now visible to Task participants, but
  • the file file transform is still not visible as Cover Image for some interesting reasons
valerio.bozzolan mentioned this in T15674: Image Transforms should reflect policy of parent Image.Nov 22 2023, 13:04
valerio.bozzolan added a comment.Dec 29 2023, 07:33

So, as default, a transform is created. Should we attach that transform instead?

https://we.phorge.it/source/phorge/browse/master/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php;f6658bf391d3cd654cf25f47bd547bfc17f9457a$30-35

src/applications/project/controller/PhabricatorProjectCoverController.php
73

Probably, this could also be done inside an expandTransaction method in ManiphestTransactionEditor. But I'm not bold enough to generalize this. Thanks for tips.

valerio.bozzolan updated this revision to Diff 1613.Jan 8 2024, 08:07

fix missing Transform entry

Harbormaster completed remote builds in B1004: Diff 1613.Jan 8 2024, 08:07
valerio.bozzolan edited the summary of this revision. (Show Details)Jan 8 2024, 11:00
valerio.bozzolan added a task: T15703: Workboard Cover Image: the cropped "Transform" is orphan.
valerio.bozzolan added inline comments.Feb 28 2024, 12:46
src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php
44–52

🔶 I think that this connection should be executed as default by PhabricatorTransformedFile executeTransform() as default. But I'm not sure, and I also don't know how to do that in that way. Thanks for any counter-idea.

valerio.bozzolan updated this revision to Diff 1746.Mar 5 2024, 09:47

ready for review, flagged the thing as side-effect

Harbormaster completed remote builds in B1097: Diff 1746.Mar 5 2024, 09:47
valerio.bozzolan added inline comments.Mar 13 2024, 10:14
src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php
44–52

Looking at PhabricatorFileTestCase it seems normal to do this kind of things manually.

valerio.bozzolan edited the summary of this revision. (Show Details)Tue, Apr 2, 13:30
valerio.bozzolan updated this revision to Diff 1808.Tue, Apr 2, 13:30
valerio.bozzolan edited the summary of this revision. (Show Details)

beautified even more - all logic inside the Transaction itself

Harbormaster completed remote builds in B1141: Diff 1808.Tue, Apr 2, 13:30
valerio.bozzolan edited the summary of this revision. (Show Details)Tue, Apr 2, 13:31
20after4 awarded a token.Tue, Apr 2, 19:21
speck subscribed.Tue, Apr 2, 22:01

The change looks reasonable though I’m not familiar at this level of the database schema design

avivey mentioned this in T15768: Better tracking tools for Files.Wed, Apr 3, 06:41
avivey subscribed.Wed, Apr 3, 06:51

All of this should maybe go under "applyExternalEffects()", but I can't find any doc that explains what should go there. Looks like external runs after all the internal and the object being saved, and before final. 🤷‍♂️

valerio.bozzolan added a comment.EditedWed, Apr 3, 07:16

All of this should maybe go under "applyExternalEffects()"

Uhm. Let's process split this in two considerations:

  1. About the line new PhabricatorTransformedFile(): since that transform needs to be created as internal effect, it's probably OK to make it non-orphan as internal effect
  2. About the line $file->attachToObject($object->getPHID()): this may sound a good candidate for the applyExternalEffects() since it really changes something unrelated from the Task - maybe I can play with $object->getProperty('cover.filePHID')

I don't know because maybe then we should execute the same File query to check if the user has enough permissions to see that file 🤔 to just attach it to the file. (edited: or maybe not?)

Unrelated random overview:

  • PhabricatorProjectColumnStatusTransaction::applyExternalEffects() - updates a Trigger
  • PhabricatorUserApproveTransaction::applyExternalEffects() - sends email
  • PhabricatorUserUsernameTransaction::applyExternalEffects() - resets a cache
  • HarbormasterBuildMessageAbortTransaction::applyExternalEffects() - release artifacts
  • PhabricatorCalendarImportDeleteTransaction::applyExternalEffects() - destroy child objects
  • PhabricatorProjectColumnTriggerTransaction::applyExternalEffects() - index things

But look at PhabricatorFileDeleteTransaction that has applyInternalEffects() that creates an external Task runner 🤔

Anyway I'll try - thanks

valerio.bozzolan updated this revision to Diff 1814.Wed, Apr 3, 08:05

introduce a nice applyExternalEffect() to change external things (the orginal file)

For performance reasons, created a static method that does not require a File object just to receive its phid... maybe nice :D

Harbormaster completed remote builds in B1145: Diff 1814.Wed, Apr 3, 08:05
valerio.bozzolan updated this revision to Diff 1815.Wed, Apr 3, 08:09

add minor comment - ready again - tested

Harbormaster completed remote builds in B1146: Diff 1815.Wed, Apr 3, 08:09
valerio.bozzolan edited the summary of this revision. (Show Details)Wed, Apr 3, 08:13
valerio.bozzolan added a comment.Wed, Apr 17, 16:10

Thanks again avivey. I liked the idea of using applyExternalEffects(). I have not found a way to do it efficiently, without introducing another static method.

Thanks for any honest comment and tip.

avivey accepted this revision.Sat, Apr 27, 07:00

Pending that small inline, lgtm.

src/applications/files/storage/PhabricatorFile.php
1438

For extra safety, let's add a check for phid_get_type($file_phid) === PHID_TYPE_FILE here.

This revision is now accepted and ready to land.Sat, Apr 27, 07:00

Revision Contents
Changeset List

Diff 1815

View Options

src/applications/files/storage/PhabricatorFile.php

Loading...
View Options

src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php

Loading...
View Options

src/applications/project/controller/PhabricatorProjectCoverController.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0