🪟 🎉 Select dbt jobs with dropdown #19502

ambirdsall · 2022-11-16T20:22:23Z

What this does

Fetches all dbt Cloud jobs that the current workspace's dbt Cloud token authorizes us to access via a new cloud endpoint (resolves https://github.com/airbytehq/airbyte-cloud/issues/2875). The new data is used for two improvements to this feature:

the dbt Cloud transformations card's "Add transformation" button now opens a dropdown. The dropdown's options list is the dbt-Cloud-supplied list of jobs, with already-saved jobs filtered out to avoid duplicates.
display the dbt-supplied job names in the list of jobs to run for each sync. Since we don't persist those names for webhook operations, we match names to saved jobs by cross-referencing the account and job IDs.

What this does not do yet

Handle the case where a saved dbt Cloud job has been deleted in dbt Cloud but remains in the list of webhook operations :: I'd like to have a quick discussion about how to handle this case before implementing it
refactor the preexisting DbtCloudJob type to use the now-canonical acountId and jobId field names (instead of the current account and job) or types (number instead of string, which was chosen for convenience when working with the now-removed text inputs) :: I'm deferring this work until the backend fully supports structured dbt Cloud data (including for saved webhook operations).

How

I did an ad hoc pass to generate the cloud API client code and types with orval (using the same setup as Generate cloud api client from airbyte-cloud if present #19091); afterwards I manually moved all generated code relevant to the new dbt Cloud endpoint to airbyte-webapp/src/packages/cloud/lib/domain/dbtCloud/api.ts, deleted the rest, and defined a wrapper function.
Rewrote the "Add transformation" button to add jobs with pre-filled IDs via a dropdown instead of adding an empty job
Replaced the text inputs for account and job ID in the job list with read-only text
Removed the yup validations; since there's no longer any user-supplied values in a saved job definition, there's no longer any need to validate them as such.
replaced the generic "dbt Cloud transformation" title for items in the dbt Cloud jobs list with the dbt-Cloud-provided jobName (only if present, the old generic name still exists as a fallback)

Recommended reading order

a cursory glance at airbyte-webapp/src/packages/cloud/lib/domain/dbtCloud/api.ts; the main definition to take note of is DbtCloudJobInfo
airbyte-webapp/src/packages/cloud/services/dbtCloud.ts
airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

ambirdsall · 2022-11-16T22:06:45Z

airbyte-webapp/src/packages/cloud/services/dbtCloud.ts

 const dbtCloudDomain = "https://cloud.getdbt.com";
 const webhookConfigName = "dbt cloud";
 const executionBody = `{"cause": "airbyte"}`;
 const jobName = (t: DbtCloudJob) => `${t.account}/${t.job}`;

 const isDbtWebhookConfig = (webhookConfig: WebhookConfigRead) => !!webhookConfig.name?.includes("dbt");

-const toDbtCloudJob = (operation: OperationRead): DbtCloudJob => {


toDbtCloudJob is rewritten to handle either a saved webhook (OperationRead) or dbt-Cloud-supplied JSON job record (DbtCloudJobInfo) as input; existing logic is unchanged except for wrapping it in a type-narrowing conditional.

ambirdsall · 2022-11-16T22:13:31Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

@@ -47,17 +54,62 @@ export const DbtCloudTransformationsCard = ({ connection }: { connection: WebBac
  //        THEN show the jobs list and the "+ Add transformation" button

  const { hasDbtIntegration, saveJobs, dbtCloudJobs } = useDbtIntegration(connection);
+
+  return hasDbtIntegration ? <DbtJobsForm saveJobs={saveJobs} dbtCloudJobs={dbtCloudJobs} /> : <NoDbtIntegration />;


The top-level card component now delegates all content; its main role is sussing out whether the user's workspace has a dbt Cloud integration and rendering the appropriate UI.

The component used to contain a decent chunk of the "has dbt integration" UI; that got extracted to a helper component mostly unchanged (except, of course, for changing the "Add transformation" button to a dropdown.

Also includes a cheeky little test usage which probably should have had a name starting with an underscore (I did not commit the test code which added the new variable's contents to `(window as any).availableJobs`, on grounds that it was very ugly, but I did want to have the import and call of the wrapper function hit the git history here).

Selecting job from dropdown adds it to saved jobs, but the new endpoint is unconditionally called even though it will always throw an error for users with no dbt Cloud integration configured.

Well, I suppose throwing a runtime error for every connection which could support dbt Cloud jobs but is part of a workspace with no integration set up, but that doesn't exactly seem ideal. Instead, this pulls all logic out of the top-level card except for pulling the dbt Cloud integration information; then it delegates the rest to either of two fairly self-contained components, `NoDbtIntegration` or the new `DbtJobsForm`. The immediate benefit is that I have a nice component boundary in which I can unconditionally run dbt-Cloud-only logic to fetch available jobs.

Since the values are now supplied by dbt Cloud via API, the user no longer has to manually input anything; and this sudden lack of user input rather obviates the need to validate user input.

josephkmh

Looks good! Just left a few non-blocking comments. Did not (yet) test locally.

Also a general thought: there are 5 components in DbtCloudTransformationsCard.tsx. I tend to aggressively break out components into separate files, unless they are really small and trivial. Mostly this helps me use Cmd + P to quickly open components 😄 but I also find it easier to scope styles, tests, etc. when components are in separate files. Do you have any criteria by which you would choose to put a component in a new file?

This is a pretty subjective code style question, so maybe something we should discuss in the frontend chapter. Not bringing it up because I feel super strongly one way or another, but I would be interested in having some sort of convention.

josephkmh · 2022-11-17T10:14:46Z

...pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.module.scss

+  background-color: colors.$grey-50;
+  flex-grow: 2;
+  padding: variables.$spacing-sm;
+  border-radius: variables.$border-radius-sm;


Styling of the icon / border radius looks a bit off? (icon too big, text too close to icon, border radius). Maybe I'm looking at an old Figma design.

Agreed, it could use a few small tweaks. The icon size at least is as designed, but the text is definitely a bit too close and now that it holds the job's display name instead of a generic "hey this is a dbt cloud job" title, it's probably a bit too small also. I'm going to merge this as-is for now, though, so Sophia can test it out while rewriting the documentation to reflect the updated UI.

josephkmh · 2022-11-17T10:15:13Z

...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx

+};
+
+const NoDbtIntegration = () => {
+  const { workspaceId } = useCurrentWorkspace();


nit: we also have useCurrentWorkspaceId() btw

josephkmh · 2022-11-17T10:24:50Z

airbyte-webapp/src/packages/cloud/services/dbtCloud.ts

+  const results = useQuery(
+    ["dbtCloud", dbtConfigId, "list"],
+    () => webBackendGetAvailableDbtJobsForWorkspace({ workspaceId, dbtConfigId }, requestOptions),
+    {
+      suspense: true,
+    }
+  );
+
+  // casting type to remove `| undefined`, since `suspense: true` will ensure the value
+  // is, in fact, available
+  return (results.data as WorkspaceGetDbtJobsResponse).availableDbtJobs;


You can use useSuspenseQuery() wrapper to avoid the | undefined and { suspense: true } option. I don't know why useSuspenseQuery() currently lives in services/connector/useSuspenseQuery, but that's where you can find it!

timroes · 2022-11-17T19:33:57Z

Please don't forget to update the PR title (and thus squash commit msg) to bring it in our standard format, before merging.

ambirdsall · 2022-11-17T19:45:24Z

Also a general thought: there are 5 components in DbtCloudTransformationsCard.tsx. I tend to aggressively break out components into separate files, unless they are really small and trivial. Mostly this helps me use Cmd + P to quickly open components smile but I also find it easier to scope styles, tests, etc. when components are in separate files. Do you have any criteria by which you would choose to put a component in a new file?

@josephkmh I intend (in a follow-up) to break out the DbtJobsList + JobsListItem components into another file. Those two are so inherently coupled that I think it makes sense to bundle them; and with those out, the DbtCloudTransformationsTab.tsx file would contain a pretty simple DbtCloudTransformationsCard component, a completely trivial NoDbtIntegration component, and the DbtJobsForm with the bulk of the transformation card structure. To this point I've deferred actually doing that primarily for time reasons (and in part to give myself a simpler workflow to edit prop names and structure while the implementation was in more flux).

* Add wrapper for cloud dbt endpoint Also includes a cheeky little test usage which probably should have had a name starting with an underscore (I did not commit the test code which added the new variable's contents to `(window as any).availableJobs`, on grounds that it was very ugly, but I did want to have the import and call of the wrapper function hit the git history here). * Add dbt Cloud jobs via dropdown (WIP: breaks if no integration) Selecting job from dropdown adds it to saved jobs, but the new endpoint is unconditionally called even though it will always throw an error for users with no dbt Cloud integration configured. * Refactor to stop errors in non-dbt-integrated workspaces Well, I suppose throwing a runtime error for every connection which could support dbt Cloud jobs but is part of a workspace with no integration set up, but that doesn't exactly seem ideal. Instead, this pulls all logic out of the top-level card except for pulling the dbt Cloud integration information; then it delegates the rest to either of two fairly self-contained components, `NoDbtIntegration` or the new `DbtJobsForm`. The immediate benefit is that I have a nice component boundary in which I can unconditionally run dbt-Cloud-only logic to fetch available jobs. * Filter already-selected jobs out of dropdown * Use dbt's jobNames and read-only {account,job}Id in job list * Remove obsolete yup validations for dbt Cloud jobs Since the values are now supplied by dbt Cloud via API, the user no longer has to manually input anything; and this sudden lack of user input rather obviates the need to validate user input. * Add button loading state when saving dbt Cloud jobs

ambirdsall requested a review from a team as a code owner November 16, 2022 20:22

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 16, 2022

ambirdsall marked this pull request as draft November 16, 2022 22:20

ambirdsall commented Nov 16, 2022

View reviewed changes

ambirdsall added 5 commits November 16, 2022 14:32

Add dbt Cloud jobs via dropdown (WIP: breaks if no integration)

141c210

Selecting job from dropdown adds it to saved jobs, but the new endpoint is unconditionally called even though it will always throw an error for users with no dbt Cloud integration configured.

Filter already-selected jobs out of dropdown

9022889

Use dbt's jobNames and read-only {account,job}Id in job list

72b601d

ambirdsall force-pushed the alex/select-dbt-jobs-with-dropdown branch from 5e2af7f to 8714b6a Compare November 16, 2022 22:32

ambirdsall marked this pull request as ready for review November 16, 2022 22:36

ambirdsall requested a review from josephkmh November 16, 2022 22:36

timroes self-requested a review November 16, 2022 23:29

Remove obsolete yup validations for dbt Cloud jobs

b9ee03c

Since the values are now supplied by dbt Cloud via API, the user no longer has to manually input anything; and this sudden lack of user input rather obviates the need to validate user input.

ambirdsall force-pushed the alex/select-dbt-jobs-with-dropdown branch from 8714b6a to b9ee03c Compare November 17, 2022 08:06

ambirdsall added 2 commits November 17, 2022 00:25

Add button loading state when saving dbt Cloud jobs

ca26f77

Merge branch 'master' into alex/select-dbt-jobs-with-dropdown

ed9b205

josephkmh approved these changes Nov 17, 2022

View reviewed changes

ambirdsall changed the title ~~Alex/select dbt jobs with dropdown~~ 🪟 🎉 Select dbt jobs with dropdown Nov 17, 2022

ambirdsall merged commit 1dbad96 into master Nov 17, 2022

ambirdsall deleted the alex/select-dbt-jobs-with-dropdown branch November 17, 2022 19:47

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎉 Select dbt jobs with dropdown #19502

🪟 🎉 Select dbt jobs with dropdown #19502

ambirdsall commented Nov 16, 2022 •

edited

Loading

ambirdsall Nov 16, 2022

ambirdsall Nov 16, 2022

josephkmh left a comment •

edited

Loading

josephkmh Nov 17, 2022

ambirdsall Nov 17, 2022

josephkmh Nov 17, 2022

josephkmh Nov 17, 2022

timroes commented Nov 17, 2022

ambirdsall commented Nov 17, 2022

🪟 🎉 Select dbt jobs with dropdown #19502

🪟 🎉 Select dbt jobs with dropdown #19502

Conversation

ambirdsall commented Nov 16, 2022 • edited Loading

What this does

What this does not do yet

How

Recommended reading order

ambirdsall Nov 16, 2022

Choose a reason for hiding this comment

ambirdsall Nov 16, 2022

Choose a reason for hiding this comment

josephkmh left a comment • edited Loading

Choose a reason for hiding this comment

josephkmh Nov 17, 2022

Choose a reason for hiding this comment

ambirdsall Nov 17, 2022

Choose a reason for hiding this comment

josephkmh Nov 17, 2022

Choose a reason for hiding this comment

josephkmh Nov 17, 2022

Choose a reason for hiding this comment

timroes commented Nov 17, 2022

ambirdsall commented Nov 17, 2022

ambirdsall commented Nov 16, 2022 •

edited

Loading

josephkmh left a comment •

edited

Loading