John conroy/fix protocols errors HMP-306 #3200
Conversation
john-conroy
changed the title
|
@NickAkhmetov am I missing any typescript bits in |
|
Great work, I like this approach! My only idea for potentially improving this further would be to also allow a custom error message function to be passed which would allow us to provide more in-depth error messages depending on the received response; e.g., it might be more informative to the user to display different error messages for 401/403/404s, or handle custom error messages inside response bodies when APIs don’t conform to standard status codes (protocols doesn’t throw 403s for unauthorized access, for example.) However, we can add that functionality at a later time, so it’s not a blocking issue whatsoever. |
context/app/static/js/components/detailPage/Protocol/Protocol.jsx
Outdated
Show resolved
Hide resolved
| import React from 'react'; | ||
| import EmailIconLink from 'js/shared-styles/Links/iconLinks/EmailIconLink'; | ||
|
|
||
| function HelpLink(props) { |
There was a problem hiding this comment.
am I missing any typescript bits in context/app/static/js/shared-styles/Links/HelpLink.tsx?
Technically in the future we'd want to define the types for the component props, but since these are being passed through to another component with unknown props, we can leave them as any for the time being and define them once we have a definition for the prop types of EmailIconLink.
I think you could define the prop type for this right now as Omit<typeof React.ComponentProps<EmailIconLink>, 'email'> but I can't be certain on the go :)
Fixes the handling of loading and errors for protocols.io requests in protocols section. Previously the non-200s were not being treated as errors and the resulting display matches what is shown in https://hms-dbmi.atlassian.net/browse/HMP-306. That being said, the requests were only failing locally due to our out of date
app.conffiles. Requests on PROD should work.I'm not sure if the mismatched key passed to
protocolsContextinProviderswould have caused any issues ( it does not seem like we're using this context anywhere?), but I fixed it to match the types defined inProctocolAPIContext. Should typescript have picked this up?I've also added
HelpLinkto be used throughout the application. I've filed https://hms-dbmi.atlassian.net/browse/HMP-307.200
Non-200