🪟 🎉 remove sub-1 hour frequency options #15708

lmossman · 2022-08-16T23:57:39Z

What

Resolves https://github.com/airbytehq/airbyte-cloud/issues/1272

This PR removes the sub-1 hour frequency options from the replication frequency dropdown, to encourage users to use frequencies that are in line with our stated limits and system expectations.

OSS users can still configure connections to run at any custom frequency through the API.

How

Removes the sub-1 hour frequency options from FrequencyConfig.json.

This PR also adds logic to include the connection's currently configured frequency in the dropdown, so as not to confuse users if they have a connection with a frequency that is not one of the default options in the dropdown.

Example screenshot of new default options:

Example screenshot of a connection with some custom frequency already set:

🚨 User Impact 🚨

Users will no longer see sub-1 hour frequencies in the replication frequency dropdown UI. OSS users can still configure any frequency through the API.

krishnaglick · 2022-08-17T16:01:33Z

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

+      if (!additionalFreqAlreadyPresent) {
+        frequencies.unshift(additionalFrequency);
+      }
+    }


This might be a good use-case for a set
But we'd need to sort on return.

I think given that we know our frequency config is rather short (I mean we'll never talk about more than 100 entries here), converting this into a set (and handling the sorting ourselves) sounds like way more effort (even in terms of performance), than just iterating through the (rather small array) once.

timroes · 2022-08-17T16:05:18Z

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

+          frequency?.timeUnit === additionalFrequency.timeUnit && frequency?.units === additionalFrequency.units
+      );
+      if (!additionalFreqAlreadyPresent) {
+        frequencies.unshift(additionalFrequency);


I'd actually suggest we add this to the end. I feel it's a bit weird, that this sits above the "Manual" option eventually. Also given that if they have a custom frequency, it might not be a "recommended" once, I feel putting it at the end might be preferred.

That makes sense, I agree with the fact that it's not "recommended" so we shouldn't put it at the front. Would it make any sense to try to put it in sorted order with the other entries? My guess is that would be slightly more complicated than it is worth, and like you said since it is not recommended it might just make more sense to put it at the end anyway

I have updated this to push it onto the end instead. Here's what it looks like now in the UI:

…ency to dropdown

timroes · 2022-08-17T19:05:08Z

@lmossman I just noticed we still have https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/src/config/utils.ts#L5 where we're reading from that file. This method seems to only be used to actually get a "human readable" version of the schedule to attach to segment events. Maybe to make this properly we work we can actually change this slightly, so that when we can't find the appropriate element inside the JSON file, we're simply concatenating the schedule.units and schedule.timeUnit together so it will return also something for those custom schedules. At that point we should make this method directly return the string and not an object from which we (currently) in all places anyway just read the .type property off.

…dule values

lmossman · 2022-08-17T20:57:04Z

I pushed a refactor of that util method to just use the values in the passed-in ConnectionSchedule, as we discussed over slack: 3e0bedd

timroes · 2022-08-18T12:06:56Z

airbyte-webapp/src/config/frequencyConfig.ts

@@ -0,0 +1,35 @@
+import { ConnectionSchedule } from "core/request/AirbyteClient";
+
+const frequencyConfig: Array<ConnectionSchedule | null> = [


We usually try to have named exports (the default exports) are left overs we're trying to eliminate. So since we're anyway touching this, could we please turn this into a:

export const frequencyConfig ...

instead of the default export at the bottom.

Ah yeah, this was mentioned in the style guide too - my bad! I've pushed this change

timroes

Tested locally with uneven numbers in a connection. Everything works and shows up as expected. One minor nitpick about exports left, once that's solved LGTM

* remove sub-1-hour frequency options, and add current connection frequency to dropdown * fix e2e test * add additional frequency to end of options instead of beginning * use better jest expect methods * refactor getFrequencyConfig util function to just use connection schedule values * move frequencyConfig to a TS file and remove unnecessary type field * use named export Co-authored-by: Tim Roes <tim@airbyte.io>

st0302 · 2022-08-28T14:30:13Z

who can tell me, how to add a new option , eg:15 minutes

st0302 · 2022-08-29T06:55:41Z

What

Resolves https://github.com/airbytehq/airbyte-cloud/issues/1272

This PR removes the sub-1 hour frequency options from the replication frequency dropdown, to encourage users to use frequencies that are in line with our stated limits and system expectations.

OSS users can still configure connections to run at any custom frequency through the API.

How

Removes the sub-1 hour frequency options from FrequencyConfig.json.

This PR also adds logic to include the connection's currently configured frequency in the dropdown, so as not to confuse users if they have a connection with a frequency that is not one of the default options in the dropdown.

Example screenshot of new default options:

Example screenshot of a connection with some custom frequency already set:

Recommended reading order

FrequencyConfig.json

formConfig.tsx

ConnectionForm.tsx

formConfig.test.ts

🚨 User Impact 🚨

Users will no longer see sub-1 hour frequencies in the replication frequency dropdown UI. OSS users can still configure any frequency through the API.

How to add the option "Every 17 minutes" in version 0.40.1 , thx

lmossman · 2022-08-30T18:36:21Z

@st0302 in order to use a custom frequency for a connection, you will need to use the Airbyte API to create or update a connection, setting the schedule and scheduleData.basicSchedule fields to the value you want to use. We are currently working on deprecating the schedule field in the API, after which you will just need to set scheduleData to the desired value.

lmossman requested a review from a team as a code owner August 16, 2022 23:57

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

krishnaglick reviewed Aug 17, 2022

View reviewed changes

timroes reviewed Aug 17, 2022

View reviewed changes

remove sub-1-hour frequency options, and add current connection frequ…

0da519b

…ency to dropdown

lmossman force-pushed the lmossman/remove-sub-1-hour-frequencies branch from b7b52a9 to 0da519b Compare August 17, 2022 16:39

lmossman added 3 commits August 17, 2022 10:26

fix e2e test

a8d2d82

add additional frequency to end of options instead of beginning

abd9ffb

use better jest expect methods

8a7516d

lmossman requested a review from timroes August 17, 2022 17:49

refactor getFrequencyConfig util function to just use connection sche…

3e0bedd

…dule values

lmossman and others added 2 commits August 17, 2022 14:45

move frequencyConfig to a TS file and remove unnecessary type field

cc43595

Merge branch 'master' into lmossman/remove-sub-1-hour-frequencies

1844228

timroes reviewed Aug 18, 2022

View reviewed changes

timroes approved these changes Aug 18, 2022

View reviewed changes

use named export

7b63572

lmossman merged commit 81eac50 into master Aug 18, 2022

lmossman deleted the lmossman/remove-sub-1-hour-frequencies branch August 18, 2022 16:49

octavia-squidington-iii mentioned this pull request Aug 19, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1-alpha #15775

Closed

evantahler mentioned this pull request Aug 22, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1 #15857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎉 remove sub-1 hour frequency options #15708

🪟 🎉 remove sub-1 hour frequency options #15708

lmossman commented Aug 16, 2022 •

edited

Loading

krishnaglick Aug 17, 2022

timroes Aug 17, 2022

timroes Aug 17, 2022

lmossman Aug 17, 2022

lmossman Aug 17, 2022

timroes commented Aug 17, 2022

lmossman commented Aug 17, 2022 •

edited

Loading

timroes Aug 18, 2022

lmossman Aug 18, 2022

timroes left a comment

st0302 commented Aug 28, 2022

st0302 commented Aug 29, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

lmossman commented Aug 30, 2022 •

edited

Loading

		@@ -0,0 +1,35 @@
		import { ConnectionSchedule } from "core/request/AirbyteClient";

		const frequencyConfig: Array<ConnectionSchedule \| null> = [

🪟 🎉 remove sub-1 hour frequency options #15708

🪟 🎉 remove sub-1 hour frequency options #15708

Conversation

lmossman commented Aug 16, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

krishnaglick Aug 17, 2022

Choose a reason for hiding this comment

timroes Aug 17, 2022

Choose a reason for hiding this comment

timroes Aug 17, 2022

Choose a reason for hiding this comment

lmossman Aug 17, 2022

Choose a reason for hiding this comment

lmossman Aug 17, 2022

Choose a reason for hiding this comment

timroes commented Aug 17, 2022

lmossman commented Aug 17, 2022 • edited Loading

timroes Aug 18, 2022

Choose a reason for hiding this comment

lmossman Aug 18, 2022

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

st0302 commented Aug 28, 2022

st0302 commented Aug 29, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

lmossman commented Aug 30, 2022 • edited Loading

lmossman commented Aug 16, 2022 •

edited

Loading

lmossman commented Aug 17, 2022 •

edited

Loading

lmossman commented Aug 30, 2022 •

edited

Loading