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

🪟 🧹 Add cron string to segment calls, remove outdated schedule property use #17672

Merged
merged 6 commits into from
Oct 6, 2022

Conversation

teallarson
Copy link
Contributor

@teallarson teallarson commented Oct 6, 2022

What

This updates the code still using the connection.schedule property for an analytics call (and only for that analytics call). We also had a few different formats that we were sending frequency data.

Now, all frequency data is sent as either:

  • the frequency in a human-readable string for basic schedules
  • the cron string + time zone for cron schedules
  • the string "manual" for manual schedules

I did leave the dropdown analytics call as it was since that call seemed focused specifically on what a user chooses from the dropdown.

How

Updated the schedule util to produce human readable strings from the scheduleData object. Utilize this method in the analytics calls.

Recommended reading order

  1. util.tsx
  2. others

Note:

After this, we will be able to fully remove the schedule property from WebBackendConnectionRead

🚨 User Impact 🚨

None

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Oct 6, 2022
onStatusUpdating={onStatusUpdating}
disabled={!allowSync}
connection={connection}
frequencyType={getFrequencyType(connection.scheduleData?.basicSchedule)}
Copy link
Contributor Author

@teallarson teallarson Oct 6, 2022

Choose a reason for hiding this comment

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

This can be figured out directly in the child component from the connection object.
#15609 tracks converting the child component to use the service and remove the need to send connection as a prop at all.

@teallarson teallarson marked this pull request as ready for review October 6, 2022 16:20
@teallarson teallarson requested a review from a team as a code owner October 6, 2022 16:20
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Code LGTM. I assume Natalie was good w/ this change to analytics?

@teallarson
Copy link
Contributor Author

teallarson commented Oct 6, 2022

Yes @krishnaglick I ran it past @nataliekwong in Slack!

@teallarson teallarson merged commit ada1cbf into master Oct 6, 2022
@teallarson teallarson deleted the teal/remove-schedule-property-from-api branch October 6, 2022 18:39
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
…erty use (airbytehq#17672)

* update segment schedule frequency calls to include cron string, unify return value format

* remove unused frequencyType prop

* cleanup

* rename and move util
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.

3 participants