Page MenuHomePhorge
Differential D25052

Add and use new RemarkupMetadata class
ClosedPublic
Actions

Authored by Dylsss on Sep 18 2022, 16:59.
Tags
Referenced Files
F305333: D25052.id272.diff
Sat, Jun 3, 17:55
F305332: D25052.id270.diff
Sat, Jun 3, 17:55
F305331: D25052.id269.diff
Sat, Jun 3, 17:55
F305330: D25052.id.diff
Sat, Jun 3, 17:55
F305329: D25052.diff
Sat, Jun 3, 17:54
Unknown Object (File)
Fri, Jun 2, 16:14
Unknown Object (File)
Wed, May 31, 06:00
Unknown Object (File)
Sat, May 6, 18:24
View All 100 Files
Subscribers
avivey
Cigaryno
Matthew
MCPCN
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
avivey
Matthew
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15106: "Upload file" in remarkup text fields should attach by default
Commits
rP90f9da643d16: Add and use new RemarkupMetadata class
Summary

Add a new RemarkupMetadata class and use with upload button as well as drag and drop pathways.

With this change, files are now attached automatically to the object. This this could solve
several issues, for example when a person uploads a file but only that author is able to see it.

Ref T15106

Test Plan
  • Drag and drop file, upload file with button. Check that both files are attached with "attachedFilePHIDs" values.
    • drop file in a Task description (now works)
    • drop file in a Task comment (now works)
    • drop file in an edited Task comment (still not supported)
  • This was already tested in Wikimedia Foundation: https://phabricator.wikimedia.org/D1203

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningwebroot/rsrc/js/core/RemarkupMetadata.js:1JAVELIN5`javelinsymbols` Not In Path
Unit
Tests Passed
Build Status
Buildable 87
Build 87: arc lint + arc unit

Event Timeline

Dylsss created this revision.Sep 18 2022, 16:59
Owners added a reviewer: O1: Blessed Committers.Sep 18 2022, 16:59
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptSep 18 2022, 16:59
Dylsss requested review of this revision.Sep 18 2022, 16:59
Harbormaster completed remote builds in B85: Diff 269.Sep 18 2022, 16:59
Dylsss updated this revision to Diff 270.Sep 18 2022, 17:02

ran celerity map again

Harbormaster completed remote builds in B86: Diff 270.Sep 18 2022, 17:02
Dylsss added a task: T15106: "Upload file" in remarkup text fields should attach by default .Sep 18 2022, 17:06
Dylsss updated this revision to Diff 272.Sep 19 2022, 01:58

Differentiate between metadata of different text areas using node ID. e.g., new comment text areas and text areas in the edit comment dialog.

Harbormaster completed remote builds in B87: Diff 272.Sep 19 2022, 01:58
avivey added a subscriber: avivey.Sep 27 2022, 14:23

Bear with me here, because Javeline (like all js frameworks) is confusing to me.

I'd expect some PHP code to actually attach the files from attachedFilePHIDs to the relevant object, but I don't see it in this diff. Is that already happening somehow?

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
260–264

is metadata gets automatically inserted into something? I don't see it being manually saved.

Dylsss added a comment.Oct 13 2022, 14:11
In D25052#1750, @avivey wrote:

I'd expect some PHP code to actually attach the files from attachedFilePHIDs to the relevant object, but I don't see it in this diff. Is that already happening somehow?

It's done here: https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;5aa159a83013fca3a72f5c6d2c493c117fbbf83b$2296

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
260–264

It gets written to the HTML element in the setMetadata method.

Ekubischta mentioned this in Q20: After uploading an image to a task, rights are not inherited. Image in status Restricted (Answer 15).Oct 26 2022, 21:35
avivey accepted this revision.Oct 28 2022, 10:19
This revision is now accepted and ready to land.Oct 28 2022, 10:19
Cigaryno added a project: Remarkup.Oct 31 2022, 13:59
Matthew requested changes to this revision.Dec 8 2022, 22:37

Please rebase and resubmit, there is a merge conflict.

matthew@Tower phorge % arc patch D25052
Created and checked out branch arcpatch-D25052.
Checking patch webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js...
Checking patch webroot/rsrc/js/core/behavior-drag-and-drop-textarea.js...
Checking patch webroot/rsrc/js/core/RemarkupMetadata.js...
Checking patch src/view/form/control/PhabricatorRemarkupControl.php...
Checking patch resources/celerity/map.php...
Applied patch webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js cleanly.
Applied patch webroot/rsrc/js/core/behavior-drag-and-drop-textarea.js cleanly.
Applied patch webroot/rsrc/js/core/RemarkupMetadata.js cleanly.
Applied patch src/view/form/control/PhabricatorRemarkupControl.php cleanly.
Applied patch resources/celerity/map.php cleanly.
 COMMITTED  Successfully committed patch.
matthew@Tower phorge % arc land
 STRATEGY  Merging with "squash" strategy, the default strategy.
 SOURCE  Landing the current branch, "arcpatch-D25052".
 ONTO REMOTE  Landing onto remote "origin", the default remote under Git.
 ONTO TARGET  Landing onto target "master", the default target under Git.
 INTO REMOTE  Will merge into remote "origin" by default, because this is the remote the change is landing onto.
 INTO TARGET  Will merge into target "master" by default, because this is the "onto" target.
 FETCH  Fetching "master" from remote "origin"...

  $   git fetch --no-tags --quiet -- origin master


 INTO COMMIT  Preparing merge into "master" from remote "origin", at commit "dc558b5538cd".
 LANDING  These changes will land:

  *   D25052 Add and use new RemarkupMetadata class
          27935e60780f  Add and use new RemarkupMetadata class

 >>>  Land these changes? [y/N/?] y

 <!> NOT REVISION AUTHOR 
You are landing revisions which you ("@Matthew") are not the author of:

  *   D25052 Add and use new RemarkupMetadata class
        Author: @Dylsss

  ?   Use "Commandeer" in the web interface to become the author of a
      revision.

 >>>  Land revisions you are not the author of? [y/N/?] y
 <!> BUILD FAILURES 
1 revision(s) have build failures:

  *   D25052 Add and use new RemarkupMetadata class
  *     B87 Buildable "B87"
 ://      https://we.phorge.it/B87
  *       Build 87 arc lint + arc unit


 >>>  Land 1 revision(s) anyway, despite failed builds? [y/N/?] y
 MERGING  27935e60780f Add and use new RemarkupMetadata class
 MERGE  Attempting to rebase changes.
 MERGE  Attempting to reduce and rebase changes.

 <!> MERGE CONFLICT 
Local commit "27935e60780f" ("arcpatch-D25052") does not merge cleanly into
"dc558b5538cd". Rebase or merge local changes so they can merge cleanly.

 LOAD STATE  Restoring local state (to ref "arcpatch-D25052" at commit "27935e60780f").
 USAGE EXCEPTION  Encountered a merge conflict.
This revision now requires changes to proceed.Dec 8 2022, 22:37
Dylsss updated this revision to Diff 320.EditedFeb 21 2023, 00:23

Rebased on master, ran bin/celerity map to resolve merge conflict.

Harbormaster completed remote builds in B116: Diff 320.Feb 21 2023, 00:23
valerio.bozzolan added a comment.Mar 9 2023, 06:49

For some historical context, this current patch comes from another one from the Phabricator of Wikimedia Foundation:

https://phabricator.wikimedia.org/D1203

webroot/rsrc/js/core/RemarkupMetadata.js
13–18

Thanks. In short, this is the proposed change. Taking this:

if (JX.RemarkupMetadata._metadataValue == null) {
  JX.RemarkupMetadata._metadataValue = [];
  JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue;
} else if (!JX.RemarkupMetadata._metadataValue.hasOwnProperty(metadataID)) {
  JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue;
}

And simplifying it to this:

if (JX.RemarkupMetadata._metadataValue == null) {
  JX.RemarkupMetadata._metadataValue = {};
}
if (!JX.RemarkupMetadata._metadataValue.hasOwnProperty(metadataID)) {
  JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue;
}

Reasons:

  1. I suggest to have myVar = {} instead of myVar = [] (so, explicitly declaring an object, instead of an array-object) since using a JavaScript array-object for an "associative map" is a little understood hack, and the risk here is that a person with experience in PHP misunderstand what is going on in JavaScript, and he/she can goes crazy when he/she finds out that things like .length are completely broken on the array, since we are not really working with an array, we are really not adding elements to an array, etc. Here we need an "associative array", and since in JavaScript an array is really an object, and since the syntax myVar['foo']=value just does myVar.foo=value on an object, and since hasOwnProperty() is just inherited from the generical object class, I suggest to just declare it as any normal object. I'm sincerely sorry if JavaScript is so weird. If you have a doubt, think about JSON: an "associative array" is just an object {}, while arrays are only used to represent a stack of consecutive elements.
  2. I also suggest to avoid the base case optimization, essentially because it does not justify the loss of readability. It also avoids the introduction of future typos since we don't repeat the same instruction twice.
valerio.bozzolan mentioned this in Q39: New behaviour when we add picture in Phriction page (Answer 43).Mar 16 2023, 15:57
valerio.bozzolan added a comment.Mar 24 2023, 23:07
This comment was removed by valerio.bozzolan.
valerio.bozzolan mentioned this in Q49: File uploads - default permissions (Answer 60).Apr 20 2023, 13:42
valerio.bozzolan edited the test plan for this revision. (Show Details)Apr 20 2023, 13:47
valerio.bozzolan added inline comments.
webroot/rsrc/js/core/RemarkupMetadata.js
13–18

Hi @Dylsss do you agree with the inline comments here?

You can also say "Yeah I don't care" ihih and I will amend that for you, no problem

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 20 2023, 13:50
MCPCN added a subscriber: MCPCN.Apr 20 2023, 15:21

I'm the author of Q49 - just confirmed that this patch appears to work in our environment (latest release of Phabricator) - files uploaded to Maniphest tasks are accessible to others.

Using the upload button has different behaviour to drag&drop - files uploaded using the upload button need to be manually attached to the object which is a bit of a faff (not sure if there's a way to automatically do this?):

image.png (241×558 px, 24 KB)

Matthew accepted this revision.Apr 20 2023, 15:58
This revision is now accepted and ready to land.Apr 20 2023, 15:58
valerio.bozzolan added a comment.Apr 20 2023, 19:13
In D25052#4020, @MCPCN wrote:

I'm the author of Q49 - just confirmed that this patch appears to work in our environment (latest release of Phabricator) - files uploaded to Maniphest tasks are accessible to others.

Using the upload button has different behaviour to drag&drop - files uploaded using the upload button need to be manually attached to the object which is a bit of a faff (not sure if there's a way to automatically do this?):

image.png (241×558 px, 24 KB)

Thanks. Since it seems to me that you are not a bot (or you are a bot with good intentions! ihih) now you are marked as Trusted Contributors 🎉 This amazing power allows you to create new Tasks. If you want, feel free to describe such specific additional corner case, opening such Task under Remarkup (and please put myself in Subscribers since your report also affects me and it's interesting to me)

valerio.bozzolan added a comment.Apr 20 2023, 19:15

Amazing @Dylsss you have green light here. Feel free to land.

Although I would like to talk about those little inline comments from me :D Thank you for your opinion

valerio.bozzolan edited the test plan for this revision. (Show Details)Apr 20 2023, 20:00
valerio.bozzolan added a comment.Apr 20 2023, 20:03
WARNING: After this change, in my JavaScript console I can see this exception: Error: JX.DOM.scry(<yuck>, ...): first argument must be a DOM node. - I do not understand a shit about Javelin so I really don't know what does it mean
valerio.bozzolan added a comment.Apr 21 2023, 13:24
In D25052#4020, @MCPCN wrote:

I'm the author of Q49 - just confirmed that this patch appears to work in our environment (latest release of Phabricator) - files uploaded to Maniphest tasks are accessible to others.

Can I ask you if you see any error in your browser console, after that patch?

Dylsss added a comment.EditedApr 23 2023, 23:33
In D25052#4034, @valerio.bozzolan wrote:
WARNING: After this change, in my JavaScript console I can see this exception: Error: JX.DOM.scry(<yuck>, ...): first argument must be a DOM node. - I do not understand a shit about Javelin so I really don't know what does it mean

I reverted to a previous commit (36dba82224de1b64bf571ebae5e944ce8a02e7c2) and still get this error when uploading from the remarkup toolbar upload button. So I don't think it is related to this change.

Dylsss added a comment.Apr 24 2023, 00:05
In D25052#4102, @Dylsss wrote:

I reverted to a previous commit (36dba82224de1b64bf571ebae5e944ce8a02e7c2) and still get this error when uploading from the remarkup toolbar upload button. So I don't think it is related to this change.

D25076 fixed this issue, this issue shouldn't exist when this patch is merged, but I'll rebase against master anyway. I'll also apply the suggested changes.

Dylsss updated this revision to Diff 512.Apr 24 2023, 00:27

Rebased and applied suggested changes

Harbormaster completed remote builds in B264: Diff 512.Apr 24 2023, 00:27
Dylsss marked 3 inline comments as done.Apr 24 2023, 00:28
This revision was landed with ongoing or failed builds.Apr 24 2023, 01:53
Closed by commit rP90f9da643d16: Add and use new RemarkupMetadata class (authored by Dylsss). · Explain Why
This revision was automatically updated to reflect the committed changes.
Dylan F <git@dylanfarrar.com> added a commit: rP90f9da643d16: Add and use new RemarkupMetadata class.
Dylsss added a comment.Apr 24 2023, 01:57

(Ignored unrelated build failures, see fix: D25133)

valerio.bozzolan mentioned this in Next Up (since Week 18).Apr 25 2023, 13:46
valerio.bozzolan mentioned this in T15272: RemarkupMetadata.js - Error: Empty ID passed to JX.$()!.Apr 25 2023, 14:00

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)Packages
resources/
celerity/
100%
Loading...
O1: Blessed Committers
src/
view/
form/
control/
2 lines0%
Loading...
O1: Blessed Committers
webroot/
rsrc/
js/
core/
49 lines--O1: Blessed Committers
29 lines--O1: Blessed Committers
7 lines--O1: Blessed Committers

Diff 272

View Options

resources/celerity/map.php

Loading...
View Options

src/view/form/control/PhabricatorRemarkupControl.php

Loading...
View Options

webroot/rsrc/js/core/RemarkupMetadata.js

Loading...
View Options

webroot/rsrc/js/core/behavior-drag-and-drop-textarea.js

Loading...
View Options

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js

Loading...