recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Drag & Drop Task Cover Image: also attach

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:

lang=php
$file->attachToObject($task_phid);

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

lang=php
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.

Reviewers: O1 Blessed Committers, avivey

Reviewed By: O1 Blessed Committers, avivey

Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno

Maniphest Tasks: T15163, T15703

Differential Revision: https://we.phorge.it/D25475

Valerio Bozzolan 3367fe00 f75c7ce7

+106 -4
+24 -4
src/applications/files/storage/PhabricatorFile.php
··· 1414 1414 1415 1415 /** 1416 1416 * Write the policy edge between this file and some object. 1417 + * This method is successful even if the file is already attached. 1417 1418 * 1418 1419 * @param phid Object PHID to attach to. 1419 1420 * @return this 1420 1421 */ 1421 1422 public function attachToObject($phid) { 1423 + self::attachFileToObject($this->getPHID(), $phid); 1424 + return $this; 1425 + } 1426 + 1427 + /** 1428 + * Write the policy edge between a file and some object. 1429 + * This method is successful even if the file is already attached. 1430 + * NOTE: Please avoid to use this static method directly. 1431 + * Instead, use PhabricatorFile#attachToObject(phid). 1432 + * 1433 + * @param phid File PHID to attach from. 1434 + * @param phid Object PHID to attach to. 1435 + * @return void 1436 + */ 1437 + public static function attachFileToObject($file_phid, $object_phid) { 1438 + 1439 + // It can be easy to confuse the two arguments. Be strict. 1440 + if (phid_get_type($file_phid) !== PhabricatorFileFilePHIDType::TYPECONST) { 1441 + throw new Exception(pht('The first argument must be a phid of a file.')); 1442 + } 1443 + 1422 1444 $attachment_table = new PhabricatorFileAttachment(); 1423 1445 $attachment_conn = $attachment_table->establishConnection('w'); 1424 1446 ··· 1432 1454 attacherPHID = VALUES(attacherPHID), 1433 1455 dateModified = VALUES(dateModified)', 1434 1456 $attachment_table, 1435 - $phid, 1436 - $this->getPHID(), 1457 + $object_phid, 1458 + $file_phid, 1437 1459 PhabricatorFileAttachment::MODE_ATTACH, 1438 1460 null, 1439 1461 PhabricatorTime::getNow(), 1440 1462 PhabricatorTime::getNow()); 1441 - 1442 - return $this; 1443 1463 } 1444 1464 1445 1465
+64
src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
··· 277 277 pht('Attached Thumbnail Visibility')); 278 278 } 279 279 280 + public function testFileVisibilityManually() { 281 + $author = $this->generateNewTestUser(); 282 + $viewer = $this->generateNewTestUser(); 283 + $author_and_viewer = array($author, $viewer); 284 + 285 + $engine = new PhabricatorTestStorageEngine(); 286 + $params = array( 287 + 'name' => 'test.dat', 288 + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, 289 + 'authorPHID' => $author->getPHID(), 290 + 'storageEngines' => array( 291 + $engine, 292 + ), 293 + ); 294 + 295 + $data = Filesystem::readRandomCharacters(64); 296 + $file = PhabricatorFile::newFromFileData($data, $params); 297 + 298 + // Create an object. 299 + $object = ManiphestTask::initializeNewTask($author) 300 + ->setTitle(pht('Test Task')) 301 + ->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy()) 302 + ->save(); 303 + 304 + // Test file's visibility before attachment. 305 + $this->assertEqual( 306 + array( 307 + true, 308 + false, 309 + ), 310 + $this->canViewFile($author_and_viewer, $file), 311 + pht('File Visibility Before Being Attached')); 312 + 313 + // Manually attach. 314 + $file->attachToObject($object->getPHID()); 315 + 316 + // Test the referenced file's visibility. 317 + $this->assertEqual( 318 + array( 319 + true, 320 + true, 321 + ), 322 + $this->canViewFile($author_and_viewer, $file), 323 + pht('File Visibility After Being Attached')); 324 + 325 + // Try again. This should not explode. 326 + $file->attachToObject($object->getPHID()); 327 + 328 + // Try again with this low-level. Again, this should not explode. 329 + PhabricatorFile::attachFileToObject($file->getPHID(), $object->getPHID()); 330 + 331 + // Try again but using the wrong low-level usage. 332 + $is_wrong_usage = false; 333 + try { 334 + PhabricatorFile::attachFileToObject($object->getPHID(), $file->getPHID()); 335 + } catch (Throwable $e) { 336 + $is_wrong_usage = true; 337 + } 338 + $this->assertEqual( 339 + true, 340 + $is_wrong_usage, 341 + pht('Check Attach Low-Level Validation')); 342 + } 343 + 280 344 private function canViewFile(array $users, PhabricatorFile $file) { 281 345 $results = array(); 282 346 foreach ($users as $user) {
+17
src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php
··· 27 27 return; 28 28 } 29 29 30 + // Generate an image transformation, usually smaller (orphan now). 30 31 $xform_key = PhabricatorFileThumbnailTransform::TRANSFORM_WORKCARD; 31 32 $xform = PhabricatorFileTransform::getTransformByKey($xform_key) 32 33 ->executeTransform($file); 33 34 35 + // Make that image transformation non-orphan. 36 + id(new PhabricatorTransformedFile()) 37 + ->setOriginalPHID($file_phid) 38 + ->setTransformedPHID($xform->getPHID()) 39 + ->setTransform($xform_key) 40 + ->save(); 41 + 34 42 $object->setProperty('cover.filePHID', $file->getPHID()); 35 43 $object->setProperty('cover.thumbnailPHID', $xform->getPHID()); 44 + } 45 + 46 + public function applyExternalEffects($object, $value) { 47 + // If the File has a Cover Image, attach that as side-effect. 48 + // Otherwise, the Cover Image may be invisible to participants. 49 + $file_phid = $object->getProperty('cover.filePHID'); 50 + if ($file_phid) { 51 + PhabricatorFile::attachFileToObject($file_phid, $object->getPHID()); 52 + } 36 53 } 37 54 38 55 public function getTitle() {
+1
src/applications/project/controller/PhabricatorProjectCoverController.php
··· 35 35 36 36 $xactions = array(); 37 37 38 + // Set the new Cover Image. 38 39 $xactions[] = id(new ManiphestTransaction()) 39 40 ->setTransactionType(ManiphestTaskCoverImageTransaction::TRANSACTIONTYPE) 40 41 ->setNewValue($file->getPHID());