-
Notifications
You must be signed in to change notification settings - Fork 2
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
Nickakhmetov/HMP-152 protocols fix #3165
Conversation
NickAkhmetov
commented
Jul 11, 2023
•
edited
Loading
edited
- Added handling for multiple protocols links in one field
- Added logic for trimming/fixing the protocols links has been edited to handle all of the cases in protocols.io failure on some Donor pages #2925 except for the typo case where commas took the place of periods
- Added specs for the trimming logic
- Added client authentication to Protocols.io requests
- Targeted the v4 API protocol API instead of the legacy v3 API (takes care of HMP-254)
After taking some time to review the documentation for the protocols.io authorization, I can summarize it:
For a first pass, we can try just using the client auth key; we can build out OAuth support in a future iteration. |
3ac9107
to
aab15dd
Compare
Rebased following the squash-merge of the base branch. |
Refactored to avoid requiring multifetcher for this case, but still keeping introduced multifetcher for other cases
…rom the split string
Zhou added the client ID entries to the various environments; once I fix the specs here this should be all set |
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! I made a few comments that you can take or leave.
<SectionItem label="Error loading protocol info"> | ||
This protocol may be private or otherwise inaccessible. | ||
</SectionItem>; | ||
} | ||
if (loading || !data) { | ||
<SectionItem label="Loading protocol info">Please wait...</SectionItem>; |
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.
@tsliaw do you want to weigh in on the language used here?
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.
@NickAkhmetov Yes that language should be changed. Please change to "Protocols are loading. If protocols take a significant time to load, please contact [email protected] about this issue and mention the HuBMAP ID." And add the email icon after the email. The styling of that loader might need to change in the future since it doesn't match the rest of the portal, but this should be sufficient for now.
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.
export const multiFetcher = (...urls: string[]) => { | ||
const f = (url: string) => fetch(url).then((response) => response.json()); | ||
if (urls.length === 0) { | ||
return Promise.resolve([]); | ||
} | ||
return Promise.all(urls.map((url) => f(url))); | ||
}; | ||
|
||
export const multiFetcherWithToken = (token: string, ...urls: string[]) => { | ||
const f = (url: string) => | ||
fetch(url, { | ||
headers: { | ||
Authorization: `Bearer ${token}`, | ||
}, | ||
}).then((response) => response.json()); | ||
if (urls.length === 0) { | ||
return Promise.resolve([]); | ||
} |
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.
Could we refactor out the commonalities and enable the multiFetcher
to accept fetch options as a an arg with a default value of an empty obj?
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.
…on, add documentation for usage of fetcher functions
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.
Looks great!