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.
| * 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?"
| * @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
| * @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 🤔
| @@ -80,16 +129,18 @@ export function execute(...operations) { | |||
| * ], | |||
| * { extIdField: "vera__Result_UID__c" } | |||
| * ); | |||
| * @example <caption>Bulk update Account records using a lazy state reference</caption> | |||
There was a problem hiding this comment.
(this comment does not apply to this line - github only lets me comments on diffs)
In the top of the bulk docs, can we maybe link to salesforce or jsforce docs about what a bulk job is and how it works?
It's not our job to explain what a bulk job is. But for users who don't know, we should be able to provide a convenient link to help them skill up and get some context.
| * state.accounts = state.data.map((a) => ({ Id: a.id, Name: a.name })); | ||
| * return state; | ||
| * }); | ||
| * bulk("Account", "update", { failOnError: true }, $.accounts); | ||
| * @function | ||
| * @param {string} sObjectName - API name of the sObject. | ||
| * @param {string} operation - The bulk operation to be performed.Eg "insert" | "update" | "upsert" |
There was a problem hiding this comment.
Oh, this line isn't rendering fully in the docs page:
The | character is getting confused by the markdown. Can you maybe use a HTML escape code? Or forward slashes?
We need to be mindful of this 🤔
| * @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
| * @public | ||
| * @example | ||
| * query(state=> `SELECT Id FROM Patient__c WHERE Health_ID__c = '${state.data.field1}'`); | ||
| * @example <caption>Query more records if next records are available</caption> |
There was a problem hiding this comment.
This caption should be like Run a query and download all matching records
| /** | ||
| * @typedef {Object} QueryOptions | ||
| * @public | ||
| * @property {boolean} [autoFetch=false] - When true, automatically fetches next batch of records if available |
There was a problem hiding this comment.
Should we enable autofetch by default in this major update?
Do we have any feeling about how important/useful autofetching is?
| * @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 | ||
| * @param {QueryOptions} [options] - Optional configuration for the query operation | ||
| * @param {function} [callback] - Optional callback function to execute for each retrieved record |
There was a problem hiding this comment.
Oh, didn't we do a thing where we removed all callbacks from the API? We missed one!
| @@ -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[?(!@.meta.externalId)] | |||
| * @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>
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.