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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃獰 馃帀 Display service token validation errors in the UI #19578

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

ambirdsall
Copy link
Contributor

@ambirdsall ambirdsall commented Nov 18, 2022

What

Shows error state if backend's token validation fails.
2022-11-17-23:12:23-screenshot

Two issues:

  1. I ran into a silly type error (default type for onError is unknown, which didn't let me access its argument's message property without a whole as any song-and-dance, including a linter magic comment 馃槩. There's a comment with a bit more detail. I figured out the issue; while I'm not sure why my valid call to useMutation wasn't typechecking, adding a MutationId argument does, which lets me clean up the call site in the settings component.
  2. the backend returns a 500 status (should be 400, or at least 4xx), so the frontend has to remove the error message's unsightly "Internal Server Error: " prefix in post (cc @mfsiega-airbyte)

@ambirdsall ambirdsall requested a review from a team as a code owner November 18, 2022 07:21
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 18, 2022
@timroes timroes self-requested a review November 18, 2022 07:45
onError: (e) => {
setHasValidationError(true);

setValidationMessage(e.message.replace("Internal Server Error: ", ""));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue for the status code cleanup.

@ambirdsall
Copy link
Contributor Author

I also added a success message; to keep the review workflow as simple as possible for this PR, I put that commit into a separate PR.

@ambirdsall ambirdsall force-pushed the alex/validate-dbt-cloud-service-tokens branch from 6a5c8a5 to b66a72d Compare November 18, 2022 19:52
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Reviewed it together over zoom, changes LGTM! Did not test locally

@ambirdsall ambirdsall force-pushed the alex/validate-dbt-cloud-service-tokens branch from ee53f98 to 7ca93f9 Compare November 18, 2022 22:34
@ambirdsall ambirdsall dismissed timroes鈥檚 stale review November 18, 2022 22:35

All requested changes were addressed; dismissing so waking Tim up is not a blocker for merging before holiday code freeze

Also removes the "Error" from the `{setV,v}alidationErrorMessage`
variables to pave the way for success feedback.
* Clean up type definitions

Also removes the "Error" from the `{setV,v}alidationErrorMessage`
variables to pave the way for success feedback.

* Show success message and clear form on successful dbt cloud token submission

* Always clear form error state and message in sync

* Disallow deleting jobs while an update is pending
@ambirdsall ambirdsall force-pushed the alex/validate-dbt-cloud-service-tokens branch from 0963cf2 to bab76fb Compare November 21, 2022 08:04
@davinchia davinchia merged commit 894d672 into master Nov 21, 2022
@davinchia davinchia deleted the alex/validate-dbt-cloud-service-tokens branch November 21, 2022 18:18
SofiiaZaitseva pushed a commit that referenced this pull request Nov 24, 2022
Note: the backend returns a 500 status (should be 400, or at least 4xx), so the frontend has to remove the error message's unsightly "Internal Server Error: " prefix in post.

In the future, we want to move this to a 400 error.
letiescanciano added a commit that referenced this pull request Nov 28, 2022
* master: (74 commits)
  Fix support icon in sidebar
  fix: add BuildPulse report for helm ac tests (#19785)
  fix: yaml syntax (#19775)
  馃獰 馃悰 Fix custom connection creation endpoint (#19702)
  Source facebook marketing: check "breakdowns" combinations (#19645)
  fix typo: notify instead of sync (#19737)
  find_non_rate_limited_PAT (#19736)
  Source Google Ads: fix schema for "campaigns" stream (#19700)
  馃帀 Source Asana: migrate to new SAT, added base HTTP errors handling (#19561)
  fix order not to randomly fail backward compatibility check (#19377)
  Bump helm chart version reference to 0.42.0 (#19706)
  fix: add extraEnv block (#19703)
  Bump Airbyte version from 0.40.21 to 0.40.22 (#19687)
  Bump helm chart version reference to 0.41.3 (#19685)
  Add connector builder server to airbyte proxy, kube overlays, and helm charts (#19554)
  dbt Cloud integration doc (#19619)
  馃獰 馃帀 Display service token validation errors in the UI (#19578)
  17644 Update Destination data type test to use the new data types (#19281)
  Docs: fix broken connector builder UI docs links (#19631)
  Bump Airbyte version from 0.40.20 to 0.40.21 (#19634)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants