-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: file upload #922
feat: file upload #922
Conversation
src/commands/data/create/file.ts
Outdated
public static readonly flags = { | ||
'target-org': Flags.requiredOrg(), | ||
'api-version': Flags.orgApiVersion(), | ||
name: Flags.string({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about calling this flag "--new-name"? Just to make it clear that, well, the name is new :). Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the alternative interpretation that -new-name
clears up by eliminating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when i wrote my comment I was thinking that --name on a command is typically used to identify something that already exists; but I just looked at all our commands that have --name, and it's to give something a name, like in this instance. So ignore, --name is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mshanemc for referring me back to the demo video regarding the --name
flag. That was very helpful.
I see where @jshackell-sfdc was coming from with the --new-name
suggestion. I think she has a point. For me, it wasn't immediately apparent that setting --name
was optional and would overwrite the local filename.
Am I correct in understanding that a successful file upload is the combination of the following?
- Create a
ContentDocument
- Create a
ContentDocumentLink
If that's correct, is it possible that the Title
field of the ContentDocument metadata type is the thing that is getting set when the file name is "overridden" by the --name
flag?
If that's the case, maybe this flag should be --title
instead of --name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, funny -- I just realized that now, see my comment/screenshot below! I like --title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in understanding that a successful file upload is the combination of the following?
- Create a
ContentDocument
- Create a
ContentDocumentLink
Not exactly.
- If you're just uploading a file, that's a ContentVersion (which automatically creates its parent ContentDocument)
- the ContentDocumentLink is only used if you pass
--parent-id
that's what related the File to the Record
If that's the case, maybe this flag should be
--title
instead of--name
?
OK, title it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, the title also becomes the filename in the org (for example, if you open it or download it, etc).
Co-authored-by: Juliet Shackell <63259011+jshackell-sfdc@users.noreply.github.com>
command-snapshot.json
Outdated
"command": "data:create:file", | ||
"flagAliases": [], | ||
"flagChars": ["f", "i", "n", "o"], | ||
"flags": ["api-version", "file", "flags-dir", "json", "name", "parent-id", "target-org"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mshanemc - Is name
meant to be an API or Developer Name for the resulting file once uploaded to the org, or is it more like a label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the name of the file once it gets uploaded to the org. See the examples
where maybe the local filename sucks and you want a better name to show up on the Files tab or the File related list.
You can see the effect of that in yesterday's demo https://drive.google.com/file/d/1U4UPL-bnRKU6D1paUmqVBoIIKLvWX4MP/view around 10:00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mshanemc - I hadn't noticed this comment from @jshackell-sfdc while I was writing my other comment.
I think this is another good reason for changing the --name
flag to --title
.
@@ -115,13 +116,15 @@ | |||
"chalk": "^5.3.0", | |||
"change-case": "^5.4.4", | |||
"csv-parse": "^4.16.3", | |||
"csv-stringify": "^6.4.6" | |||
"csv-stringify": "^6.4.6", | |||
"form-data": "^4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3PP approved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used by jsforce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, yes.
src/api/file/fileToContentVersion.ts
Outdated
const result = await conn.query<ContentVersion>( | ||
`Select Id, ContentDocumentId, Title, FileExtension from ContentVersion where Id='${CV.id}'` | ||
); | ||
return result.records[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a record guaranteed to be returned from the query? I don't know the /sobjects/ContentVersion
API so maybe it can be assumed. But if it returns a non http 200 the code will continue and either fail during the query or throw a TypeError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the ID from one we just created in line 38.
http errors would throw from conn.query (before line 48). I can put some checks around it (or switch to singleRecordQuery, ex: CV saves but doesn't show up in the query for some reason)
What does this PR do?
adds a new command to upload files and optionally attach them to records
to replace a 3PP plugin command used by one of the commerce plugins
used
create
because of definition here https://github.com/salesforcecli/cli/wiki/Design-Guidelines-Common-ActionsWhat issues does this PR fix or reference?
part of forcedotcom/cli#2344
related to forcedotcom/cli#2346