Update query parameters and add documentation #11
Conversation
brandoncharnesky
requested review from
explunit and
lynnfaraday
and removed request for
explunit
github-actions
bot
added
the
cc/publicinfo/mdh
dotenv.sample
Outdated
| RKS_SERVICE_ACCOUNT= | ||
| # The Project GUID | ||
| # | ||
| # Found in MyDataHelpsDesigner via Projects -> About Settings -> Project ID |
There was a problem hiding this comment.
I like the general note at the top, but these specific instructions may change, and nobody's going to remember to update them here. I think it's better to just point them at the readme.
We could improve the readme though if you think it's unclear. Maybe link each line item in the bullet points to the specific sections rather than having the note underneath.
There was a problem hiding this comment.
Removed all of this in favor of one line to remind about the readme
dotenv.sample
Outdated
| # | ||
| # Found in MyDataHelpsDesigner via Projects -> About Settings -> Project ID | ||
| # | ||
| RKS_PROJECT_ID="361e1234-8c15-78ae-1234-3b70d33023d6" |
There was a problem hiding this comment.
I think it's better not to have concrete examples here since someone might think they're real. If you want to make it something generic like "YOUR_PROJECT_ID" or "YOUR_ACCOUNT_NAME" that's fine, or just leave them blank.
quickstart.js
Outdated
| @@ -54,14 +54,15 @@ async function getServiceAccessToken() { | |||
|
|
|||
| async function getFromApi(serviceAccessToken, resourceUrl, queryParams = {}, raiseError = true) { | |||
|
|
|||
| const url = `${baseUrl}${resourceUrl}` | |||
| const url = new URL(`${baseUrl}${resourceUrl}`); | |||
There was a problem hiding this comment.
Thanks for fixing this. I think there's a slightly more straightforward way to do this:
async function getFromApi(serviceAccessToken, resourceUrl, queryParams = null, raiseError = true) {
const queryString = queryParams ? `?${new URLSearchParams(queryParams)}` : '';
const url = `${baseUrl}${resourceUrl}${queryString}`;
quickstart.js
Outdated
| @@ -104,6 +105,7 @@ function logResponse(response) { | |||
| async function quickstart() { | |||
| var url; | |||
| var response; | |||
| var queryParams = {}; | |||
There was a problem hiding this comment.
The existing example was to illustrate getting all participants, so we don't want to add the query param to that one.
I think now that you've fixed it, it'll be pretty obvious how to do a call with a query param and we don't need a specific example. But if you want to include one, you should add a new example at the end, after the existing ones. (maybe a participant search by name or something)
There was a problem hiding this comment.
I think the queryParams var is no longer used now and can also be removed.
|
@brandoncharnesky Please tick off the security checklist boxes in the PR description if they're completed. |
|
Thank you for the feedback! |
quickstart.js
Outdated
| const url = `${baseUrl}${resourceUrl}` | ||
| const queryString = queryParams ? `?${new URLSearchParams(queryParams)}` : ''; | ||
| const url = `${baseUrl}${resourceUrl}${queryString}`; | ||
| console.log(url); |
There was a problem hiding this comment.
We should either remove this log or add some clarity text if you feel it's important for debugging. Like: "Calling ${url}"
Overview
Add a query parameter example
Update the way query parameters are handled in getFromApi
Add documentation directly to the dotenv.sample
Security
REMINDER: All file contents are public.
Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including: