salesforce update function docs
#821
Conversation
|
@josephjclark yes it will replace #799, I will close the PR once i am done with the changes |
There was a problem hiding this comment.
Hi @mtuchi, sorry for the delay!
I've done a detailed review of the docs and this PR and suggested a bunch of improvements (sorry it's a long list!). There's a bunch of older problems that we should take the opportunity to fix, and some open questions too.
packages/salesforce/src/Adaptor.js
Outdated
| * Options provided to the Salesforce bulk API request | ||
| * @typedef {Object} BulkOptions | ||
| * @public | ||
| * @property {string} extIdField - External id field. |
There was a problem hiding this comment.
Wait is this required? If it's required it probably shouldn't be on the options object.
Also (and I know this is a problem across the docs) we shouldn't mark options on an optional object as optional... because they're implicitly optional! And anyway who knows what the ugly brackets around the option name mean?"
There was a problem hiding this comment.
It's required only for upsert operation, I have update the docs to reflect that
| * @param {integer} [options.pollInterval=6000] - Polling interval in milliseconds. | ||
| * @param {integer} [options.pollTimeout=240000] - Polling timeout in milliseconds. | ||
| * @param {BulkOptions} options - Options passed to the bulk api. | ||
| * @state {SalesforceState} |
There was a problem hiding this comment.
What is written to state.data if you call bulk? Should we document it properly?
The generic Operation results really isn't very helpful
There was a problem hiding this comment.
I made some improvements
packages/salesforce/src/Adaptor.js
Outdated
| * @param {boolean} [options.failOnError=false] - Fail the operation on error. | ||
| * @param {integer} [options.pollInterval=6000] - Polling interval in milliseconds. | ||
| * @param {integer} [options.pollTimeout=240000] - Polling timeout in milliseconds. | ||
| * @param {BulkOptions} options - Options passed to the bulk api. |
There was a problem hiding this comment.
Ah this is a bit confusing. Some of these are OpenFn options, others are passed through to the jsforce API
This is a problem with the wording of options passed to the bulk API (because most options are NOT passed to the bulk API), and on the BulkOptions type.
Maybe we should use generic language like "Options to configure the request", then we should document our own options (as we do now), and THEN we should link to the jsforce options (ensuring we're linking to the right library version). Perhaps we say like "Options to configure the request. In addition to these, you can pass any of the options supported by the jsforce API [link]"
Then the pattern we establish here needs rolling out to other APIs
Checkout the docs for commcare.bulk - where we mostly link to commcare docs directly
There was a problem hiding this comment.
Unrelated: as this is a major version bump anyway, how do you feel about moving the operation option to the options object, and defaulting it to "upsert"?
I don't know how common it is to set this - but it feels like it should be an option rather than a required field.
Upsert is probably the most common case but I guess it could be dangerous if users aren't aware they're doing it 🤔
There was a problem hiding this comment.
Honestly i think bulk() needs to be improved. There are couple of operation that i have never seen being used. For example queryAll, query, hardDelete i don't know if they work at all.
But as for suggestion i think we should keep the function signature the same
packages/salesforce/src/Adaptor.js
Outdated
| * @param {object} options - Options passed to the bulk api. | ||
| * @param {boolean} [options.autoFetch=false] - Fetch next records if available. | ||
| * @param {function} callback - A callback to execute once the record is retrieved | ||
| * @param {(string|function)} qs - A SOQL query string or a function that returns a query string. Must be less than 4000 characters in WHERE clause |
There was a problem hiding this comment.
The idea of passing a function is (or should be) so normal and openfn-y that I don't want to document it explicitly here.
This is basically a very old documentation pattern and we should eradicate it where we can.
Also, as before, please rename qs to query
| @@ -548,6 +608,7 @@ export function query(qs, options = {}, callback = s => s) { | |||
| * @magic externalId - $.children[?(@.name=="{{args.sObject}}")].children[?(@.meta.externalId)].name | |||
| * @param {(object|object[])} records - Field attributes for the new object. | |||
| * @magic records - $.children[?(@.name=="{{args.sObject}}")].children[?([email protected])] | |||
| * @state {SalesforceState} | |||
| * @returns {Operation} | |||
| */ | |||
| export function upsert(sObjectName, externalId, records) { | |||
There was a problem hiding this comment.
The general documentation for this function isn't terribly good and is wierdly formatted. I think there's a missing full stop? But it's wierd that we say "externalID must be specified". It's not documented as optional or anything.
I'm also not a great fan of Array.<object>
There was a problem hiding this comment.
I have updated the docs for upsert and add a link to https://jsforce.github.io/document/#upsert
Also for records param. I have updated the type to be {(Object|Object[])} which is the accurate representation of the type
|
Hiya @josephjclark sorry for long overdue delay on this PR, I have finished working on your feedback, Please give this another round of review |
|
Thank you @mtuchi ! Sorry it's been an epic 😅 I'll review this tomorrow 👍 |
Summary
queryoption inrequestfunctionFixes #664
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
dev only changes don't need a changeset.