🪟 Per-Stream state new flow #14634

timroes · 2022-07-12T17:29:08Z

What

Fixes #14560

Partial reset possible:

Full reset required:

TODOs

~~Unit tests~~
Proper typings for ModalService input/output
Finish and cleanup modal service
Proper warning modal
Finalize wording
Remove secondaryButton from ConfirmationModal again
Switch to new update API once merged

timroes · 2022-07-12T17:30:48Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

@@ -114,59 +99,62 @@ export const ReplicationView: React.FC<ReplicationViewProps> = ({ onAfterSaveSch
    if (activeUpdatingSchemaMode) {
      setActiveUpdatingSchemaMode(false);
    }
-
-    formikHelpers?.resetForm({ values });


ℹ️ The onSubmit function was never called with the formikHelpers parameters specified, so this should have never been called.

Just have to make sure that after saving, the confirmation modal to discard chagnes does not appear.

It seems to work. This should all be handled by the call inside the ConnectionForm: https://github.com/airbytehq/airbyte/blob/tim/per-stream-flow/airbyte-webapp/src/views/Connection/ConnectionForm/ConnectionForm.tsx#L167

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

timroes · 2022-07-13T09:45:24Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

    }
  };

-  const onEnterRefreshCatalogMode = async () => {
+  const onRefreshSourceSchema = async () => {
+    setSaved(false);


ℹ️ This (and the setSaved(false) call below) solves some issue that the "This connection was saved" reappears again after an error vanished, despite no new save has happened.

timroes · 2022-07-13T09:47:21Z

airbyte-webapp/src/views/Connection/ConnectionForm/ConnectionForm.tsx

-  const openResetDataModal = useCallback(() => {
-    openConfirmationModal({
-      title: "form.resetData",
-      text: "form.changedColumns",
-      submitButtonText: "form.reset",
-      cancelButtonText: "form.noNeed",
-      onSubmit: async () => {
-        await onReset?.();
-        closeConfirmationModal();
-      },
-    });
-  }, [closeConfirmationModal, onReset, openConfirmationModal]);


ℹ️ This was the old "should we reset or not" modal when the configuration inside the stream changed, but the stream wasn't manually refreshed.

Note: The warning shown here even was very much off, what actually happened. We showed "You have changed which column is used to detect new records for a stream. You may want to reset the data in your stream.", though we showed this also for other types of catalog changes (e.g. sync mode change).

timroes · 2022-07-13T09:49:36Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

-  const openResetDataModal = (values: ValuesProps) => {
-    openConfirmationModal({
-      title: "connection.updateSchema",
-      text: "connection.updateSchemaText",
-      submitButtonText: "connection.updateSchema",
-      submitButtonDataId: "refresh",
-      onSubmit: async () => {
-        await onSubmit(values);
-        closeConfirmationModal();
-      },
-    });
-  };


ℹ️ This was the old "we'll need to reset your data", when a "Source schema refresh" has happened before.

timroes · 2022-07-14T07:41:08Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

+  const requireFullReset = stateType === ConnectionStateType.legacy;
+  return (
+    <div className={styles.resetWarningModal}>
+      {/* TODO: This should use proper text stylings once we have them available. */}


ℹ️ This TODO is meant to stay inside, and hopefully we can build out the proper text styles soon, so use them here.

If there is an issue for that, it could be helpful to link it in this comment so that its easy to find all of the related TODOs in the code

timroes · 2022-07-14T17:06:33Z

airbyte-webapp/src/hooks/services/Modal/ModalService.tsx

+  const service: ModalServiceContextType = useMemo(
+    () => ({
+      openModal: (options) => {
+        resultSubjectRef.current = new Subject();


ℹ️ Why solving this with an observable here, despite only needing a one time resolved value. The problem is, that we can't control a Promise from the outside, so we'd need to instantiate a promise here and store a reference to it's resolve/reject method in a ref for later consumption. I had this first, and the whole component read way worse imho, thus I switched this over to use a simple subject and convert that later to a promise.

edmundito

Code looks good

edmundito · 2022-07-15T18:22:36Z

Code is good, but I ran into a situation while testing:

Add a new table to the source
Open the replication settings
Refresh the schema
Hit save without changing the stream configuration (new table is unchecked)

I didn't change anything about the streams, but I still got the dialog to reset the schema. This also happens if I modify a disabled stream.

edmundito · 2022-07-15T18:27:40Z

Another thing I noticed: After adding the new table and saving the schema, the stream disappears from the list. But when I reload the replication page, it appears again.

edmundito · 2022-07-15T18:53:12Z

Another thing about the modal, for me it makes more sense that it shows the "Save and Reset" as a button rather than a checkbox. so the user has three CTAs:

Cancel
Save
Save and Reset streams

timroes · 2022-07-15T19:15:04Z

Another thing about the modal, for me it makes more sense that it shows the "Save and Reset" as a button rather than a checkbox. so the user has three CTAs:

Cancel

Save

Save and Reset streams

The problem is we really want to discourage saving without resetting, thus this layout. Cc @andyjih happy to get your input on that question, checkbox vs three buttons?

timroes · 2022-07-15T19:17:59Z

Code is good, but I ran into a situation while testing:

Add a new table to the source

Open the replication settings

Refresh the schema

Hit save without changing the stream configuration (new table is unchecked)

I didn't change anything about the streams, but I still got the dialog to reset the schema. This also happens if I modify a disabled stream.

That is so far actually intended behavior, since the catalog changed (despite the stream not being enabled that changed).

@andyjih @lmossman I wonder if we should actually only compare enabled streams in the original and the catalog we're about to send? And only if there's a difference in the enabled ones show that modal?

lmossman · 2022-07-15T19:21:39Z

That is so far actually intended behavior, since the catalog changed (despite the stream not being enabled that changed).

@andyjih @lmossman I wonder if we should actually only compare enabled streams in the original and the catalog we're about to send? And only if there's a difference in the enabled ones show that modal?

@timroes I believe this is actually how it already works in the backend - we only compare the configured catalog that the frontend passes us with the configured catalog that is stored in the database, and those configured catalogs only contain streams that are enabled. So I think it will be a purely frontend change to bring it in line with this behavior, which we should do so that they are consistent

lmossman · 2022-07-17T18:53:50Z

@timroes you should rebase or merge master again, as on Friday I merged a fix for a bug I found in the updateNew endpoint: #14763

timroes · 2022-07-18T10:17:36Z

, and those configured catalogs only contain streams that are enabled.

Just for double checking: At the moment (before and with this PR) we're sending to the backend also all non-enabled streams, and have selected: false set on them. So with the change of only comparing active streams, we'd compare on a subset of what we're sending to the backend. This would still though be the desired behavior?

timroes · 2022-07-19T15:24:34Z

Added the change to only compare selected streams for showing the modal.

timroes · 2022-07-19T15:32:17Z

Another thing I noticed: After adding the new table and saving the schema, the stream disappears from the list. But when I reload the replication page, it appears again.

This is caused by some issue in the API. I filed #14839 to get this fixed.

timroes · 2022-07-19T15:43:14Z

@edmundito Could you give this another look? With Andy out, Charles and I would prefer merging the changes (once BE is fixed) for now as it is, and do a separate PR for cleaning up the modal (wording + design) once Andy will be available again.

airbyte-webapp/src/locales/en.json

edmundito

Code changes look good. Will approve pending @andyjih's comments on the wording for final approval.

Co-authored-by: Andy Jih <andyjih@users.noreply.github.com>

* [WIP] new per stream flow * Start work on ModalSerice * Continue work * Fix typo * Change wording * Add todos and remove dead code * Remove dead message * Adjust to new API * Adjust to new modal changes * Remove debug output * Fix e2e test * Add data-testids * Adjust for PR review * Switch to new ModalFooter/Body components * Add ModalService tests * Downgrade user-events again * Only compare selected streams * Update airbyte-webapp/src/locales/en.json Co-authored-by: Andy Jih <andyjih@users.noreply.github.com> * Update airbyte-webapp/src/locales/en.json Co-authored-by: Andy Jih <andyjih@users.noreply.github.com> * Remove redundant space Co-authored-by: Andy Jih <andyjih@users.noreply.github.com>

timroes added 2 commits July 12, 2022 15:41

[WIP] new per stream flow

78c9d6b

Start work on ModalSerice

efa5c98

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jul 12, 2022

timroes commented Jul 12, 2022

View reviewed changes

lmossman reviewed Jul 12, 2022

View reviewed changes

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx Outdated Show resolved Hide resolved

timroes added 4 commits July 13, 2022 10:38

Merge branch 'master' into tim/per-stream-flow

6ab269c

Continue work

654d502

Fix typo

b89cc19

Change wording

2657d88

timroes commented Jul 13, 2022

View reviewed changes

Add todos and remove dead code

6fce4f0

timroes commented Jul 13, 2022

View reviewed changes

timroes added 6 commits July 13, 2022 11:53

Remove dead message

5a9cbcc

Merge branch 'master' into tim/per-stream-flow

13be9e8

Merge branch 'master' into tim/per-stream-flow

7d1fbeb

Adjust to new API

9266bb5

Merge branch 'master' into tim/per-stream-flow

ee1c329

Adjust to new modal changes

871df8a

timroes commented Jul 14, 2022

View reviewed changes

Remove debug output

f0515e8

timroes changed the title ~~[WIP] new per stream flow~~ 🪟 Per-Stream state new flow Jul 14, 2022

Fix e2e test

8de44f9

timroes marked this pull request as ready for review July 14, 2022 08:20

timroes requested a review from a team as a code owner July 14, 2022 08:20

Add data-testids

e08d0e5

Amruta-Ranade added area/documentation Improvements or additions to documentation team/documentation labels Jul 14, 2022

timroes commented Jul 14, 2022

View reviewed changes

edmundito reviewed Jul 15, 2022

View reviewed changes

Merge branch 'master' into tim/per-stream-flow

7cadda6

timroes added 2 commits July 18, 2022 18:28

Downgrade user-events again

f736e2e

Merge branch 'master' into tim/per-stream-flow

4984735

timroes mentioned this pull request Jul 19, 2022

web_backend/connections/updateNew API does not return disabled streams #14839

Closed

Only compare selected streams

b92807b

andyjih suggested changes Jul 19, 2022

View reviewed changes

airbyte-webapp/src/locales/en.json Outdated Show resolved Hide resolved

airbyte-webapp/src/locales/en.json Outdated Show resolved Hide resolved

edmundito approved these changes Jul 19, 2022

View reviewed changes

timroes and others added 3 commits July 19, 2022 18:19

Update airbyte-webapp/src/locales/en.json

1f0b868

Co-authored-by: Andy Jih <andyjih@users.noreply.github.com>

Update airbyte-webapp/src/locales/en.json

5e759ca

Co-authored-by: Andy Jih <andyjih@users.noreply.github.com>

Remove redundant space

95a90b6

timroes requested a review from andyjih July 19, 2022 17:21

andyjih approved these changes Jul 19, 2022

View reviewed changes

timroes merged commit 87dcfd2 into master Jul 19, 2022

timroes deleted the tim/per-stream-flow branch July 19, 2022 19:01

evantahler mentioned this pull request Jul 21, 2022

Implement a logic to reset an individual stream #5317

Closed

octavia-squidington-iii mentioned this pull request Jul 23, 2022

Bump Airbyte version from 0.39.37-alpha to 0.39.38-alpha #14976

Merged

sophia-wiley mentioned this pull request Aug 5, 2022

Change stream configuration in Cloud #15338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 Per-Stream state new flow #14634

🪟 Per-Stream state new flow #14634

timroes commented Jul 12, 2022 •

edited

Loading

timroes Jul 12, 2022

edmundito Jul 14, 2022

timroes Jul 15, 2022

timroes Jul 13, 2022 •

edited

Loading

timroes Jul 13, 2022 •

edited

Loading

timroes Jul 13, 2022

timroes Jul 14, 2022

lmossman Jul 14, 2022

timroes Jul 14, 2022

edmundito left a comment

edmundito commented Jul 15, 2022 •

edited

Loading

edmundito commented Jul 15, 2022

edmundito commented Jul 15, 2022

timroes commented Jul 15, 2022

timroes commented Jul 15, 2022

lmossman commented Jul 15, 2022

lmossman commented Jul 17, 2022 •

edited

Loading

timroes commented Jul 18, 2022

timroes commented Jul 19, 2022

timroes commented Jul 19, 2022

timroes commented Jul 19, 2022

edmundito left a comment

🪟 Per-Stream state new flow #14634

🪟 Per-Stream state new flow #14634

Conversation

timroes commented Jul 12, 2022 • edited Loading

What

TODOs

timroes Jul 12, 2022

Choose a reason for hiding this comment

edmundito Jul 14, 2022

Choose a reason for hiding this comment

timroes Jul 15, 2022

Choose a reason for hiding this comment

timroes Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

timroes Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

timroes Jul 13, 2022

Choose a reason for hiding this comment

timroes Jul 14, 2022

Choose a reason for hiding this comment

lmossman Jul 14, 2022

Choose a reason for hiding this comment

timroes Jul 14, 2022

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

edmundito commented Jul 15, 2022 • edited Loading

edmundito commented Jul 15, 2022

edmundito commented Jul 15, 2022

timroes commented Jul 15, 2022

timroes commented Jul 15, 2022

lmossman commented Jul 15, 2022

lmossman commented Jul 17, 2022 • edited Loading

timroes commented Jul 18, 2022

timroes commented Jul 19, 2022

timroes commented Jul 19, 2022

timroes commented Jul 19, 2022

edmundito left a comment

Choose a reason for hiding this comment

timroes commented Jul 12, 2022 •

edited

Loading

timroes Jul 13, 2022 •

edited

Loading

timroes Jul 13, 2022 •

edited

Loading

edmundito commented Jul 15, 2022 •

edited

Loading

lmossman commented Jul 17, 2022 •

edited

Loading