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

🪟 🐛 Fix showing reset dialog after refresh source #19350

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Nov 11, 2022

What

Fixes a problem with the reset modal not showing after a user refreshed their source schema.

How

Since #16748 we updated the connection in ConnectionEditService, which is later when submitting used to compare against the form values to detect if there's a change in the catalog. Since it now always contained the updated (refreshed) connection, it would no longer diff from the form values just because of a refreshed catalog and thus the reset modal wouldn't show.

Sicne we now have the catalogDiff on that connection after refreshing I added logic to check if any of the transforms in this affects an enabled stream and if so show the reset modal again.

@timroes timroes added type/bug Something isn't working team/platform-move area/frontend Related to the Airbyte webapp ui/connection labels Nov 11, 2022
@timroes timroes requested a review from a team as a code owner November 11, 2022 12:32
@octavia-squidington-iv octavia-squidington-iv added the area/platform issues related to the platform label Nov 11, 2022
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

code looks good and tested locally, this solves the problem outlined

@timroes timroes changed the title Fix showing reset dialog after refresh source 🪟 🐛 Fix showing reset dialog after refresh source Nov 11, 2022
@josephkmh
Copy link
Contributor

Code LGTM - but looks like a place that would be useful to have a test to avoid another regression. WDYT?

@timroes
Copy link
Collaborator Author

timroes commented Nov 11, 2022

but looks like a place that would be useful to have a test to avoid another regression. WDYT?

I agree. We have that on the list of e2e tests we still want to add, since testing this in a unit test did require too mock way too much to actually still have realistic meaningful tests for it, since those detections (or the data delivered there) are build from several APIs and require to kind of run the whole connection form to be in a somewhat realistic state. cc @ambirdsall Would be great if we can keep track of adding some e2e tests for the reset dialogs, if we don't have this already tracked.

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 team/platform-move type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants