Skip to content
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

feat: improve BBMRI negotiator error handling and do some cleanup #215

Merged

Conversation

timakro
Copy link
Contributor

@timakro timakro commented Feb 11, 2025

Important: I couldn't fully test this because I'm not whitelisted to send requests to the BBMRI Negotiator.

I set out to improve the error handling and ended up rewriting the entire file. Improvements include:

  • HTTP status codes other than 201 are now always logged to the developer console including the response body. This would have saved us some debugging when working on [Bug]: Negotiator Service generates wrong json, when all collections are selected #207.
  • Use errorChannel instead of alert() to inform the user of errors
  • Simplified how lens options are accessed
  • Fixed the name of negotiateOptions (previously incorrectly negotiatorOptions) so that types are correctly inferred
  • (Hopefully) improved the documentation

I've decided to use the same user-facing error message for all types of errors: "Fehler beim Anfragen der Daten und Proben". My take is that describing in more detail what went wrong (e.g. network error or unexpected HTTP status code) wouldn't really provide any benefit to the user who couldn't take action anyways. So this potentially fixes #210 even though it doesn't do exactly what is described there because I don't think it's a good idea. Do you agree in general with this approach or do you prefer more specific user-facing errors?

…options

BREAKING CHANGE: If you have "negotiateOptions" in your lens options, you must add a key "authorizationHeader" under "negotiateOptions" that contains the value of the HTTP Authorization header (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization), e.g. "Basic <base64string>"
…iate-button> component

BREAKING CHANGE: If you are using <lens-negotiate-button> in your application change the value of the "type" prop to "Negotiator" if it was "bbmri" before or to "ProjectManager" if it was "ccp" before.
@timakro timakro marked this pull request as draft February 14, 2025 11:51
@timakro timakro marked this pull request as ready for review February 14, 2025 12:13
Copy link
Collaborator

@patrickskowronekdkfz patrickskowronekdkfz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we should start documenting the API's

@patrickskowronekdkfz patrickskowronekdkfz merged commit d1fba27 into develop Feb 14, 2025
6 checks passed
@patrickskowronekdkfz patrickskowronekdkfz deleted the feat/improve-bbmri-negotiator-error-handling branch February 14, 2025 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Use error channel and cover all http statues for negotiatorservice
2 participants