-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Alex/mvp UI for dbt cloud integration #18095
Conversation
0865bf0
to
72b719a
Compare
72b719a
to
42f2a3a
Compare
fbba192
to
65ea077
Compare
To get the styling to work, I needed to edit `LabeledInput` to accept a `className` prop, so I could give it contextually-specific styling.
This feature isn't added to either OSS or cloud builds; it will be dynamically toggled for specific targeted accounts via LaunchDarkly's `featureService.overwrites` key.
This still uses some hardcoded conditions instead of anything resembling actual data
There's some key data missing and it's not currently wired up
Transformations | ||
<Button | ||
variant="secondary" | ||
disabled={!!values.jobs.find((job: DbtCloudJob) => !job.operationId)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently has a bug: the app doesn't add operationIds
after submitting, so you can only ever add one job at a time without refreshing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left mostly non-blocking comments that you should feel free to ignore, but the core logic looks sound to me 👍 Most of my comments are also things I think we should discuss as a FE team anyway, so not really specific to this PR.
I did not test locally beyond seeing the dbt cloud integration settings page.
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [_fullUrl, account, job] = matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// eslint-disable-next-line @typescript-eslint/no-unused-vars | |
const [_fullUrl, account, job] = matches; | |
const [, account, job] = matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ We don't assign unused variables any name, but instead skip them. This is happening in a couple of places around this PR, so we should try to remove all of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I didn't realize you can do that when destructuring; I like that much better than using throwaway variables and manually shushing the linter. Will do for all instances.
@use "../../../../../scss/colors"; | ||
@use "../../../../../scss/variables" as vars; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@use "../../../../../scss/colors"; | |
@use "../../../../../scss/variables" as vars; | |
@use "scss/colors"; | |
@use "scss/variables" as vars; |
import classNames from "classnames"; | ||
import { Field, Form, Formik, FieldArray, FieldProps } from "formik"; | ||
import { Link } from "react-router-dom"; | ||
import { array, object, number } from "yup"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I've noticed we tend to alias the import import * as yup from "yup"
- would adjust that here for consistency
})} | ||
> | ||
<div> | ||
<button className={styles.arrow} tabIndex={0} onClick={toggleOpen}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessibility improvement: would be good to add aria-expanded
, aria-controls
and aria-label
to the button.
UX idea: add <label>{children}</label>
so you can click on "Advanced options" as well as just the arrow button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might simply want to use the https://headlessui.com/react/disclosure component from headless UI, which we use in other places for collapsible elements, which handles all the accessibility. I know that the platform workflow team has also thought about creating a component for that now. @edmundito @dizel852 do you know if this is already started work on, since we should try to coordinate do not do that effort duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep using button
here it also won't require tabIndex
, since that's the default value of the button
element anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX idea: add {children} so you can click on "Advanced options" as well as just the arrow button.
Or children should just be part of the button
completely. That one got me as well. Since I feel usually there is more content inside the collapsible than in the button, I'd suggest we make the drawer
property just to be the children
instead and add a buttonLabel
property instead that renders inside the button, so we don't need to put the full content of the collapsiple into a property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that could also work, only drawback I see is that you might want icons/other JSX in the buttonLabel
slot. IMO Tim's idea to use headlessui is probably the quickest and easiest. It also has a more verbose but composable API:
<Disclosure>
<Disclosure.Button className="py-2">
Is team pricing available?
</Disclosure.Button>
<Disclosure.Panel className="text-gray-500">
Yes! You can purchase a license that you can share with your entire
team.
</Disclosure.Panel>
</Disclosure>
<Card | ||
title={ | ||
<span className={styles.jobListTitle}> | ||
Transformations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's already on your radar, but these components are all still using hard-coded strings instead of <FormattedMessage>
. This one I would fix before merging, lest we forget.
)} | ||
</Field> | ||
</div> | ||
<button type="button" className={styles.jobListItemDelete} onClick={removeJob}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add an aria-label
here unless FontAwesomeIcon
has some text label (I don't think it does?)
const dbtSettingsPath = `/${RoutePaths.Workspaces}/${workspaceId}/${RoutePaths.Settings}/dbt-cloud`; | ||
return ( | ||
<div className={classNames(className, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using <Text>
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should :) It will also solve some default browser margin issues on p
.
{hasDbtIntegration ? ( | ||
<DbtJobsList jobs={values.jobs} remove={remove} isValid={isValid} /> | ||
) : ( | ||
<NoDbtIntegration className={styles.jobListContainer} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should <Formik>
even render if !hasDbtIntegration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, no, since there's no need for it in that case. It's a detail I didn't have time to clean up: the Formik
component was inside the DbtJobsList
component until I wired up the "add new integration" button and realized I needed it to have access to the push
function from FieldArray
's render props.
@use "../../../../../scss/colors"; | ||
@use "../../../../../scss/variables" as vars; | ||
|
||
$item-spacing: 25px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we can use the spacing variables in scss/variables
65ea077
to
b0e9079
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sending in between review states, because of the urgency)
const dbtSettingsPath = `/${RoutePaths.Workspaces}/${workspaceId}/${RoutePaths.Settings}/dbt-cloud`; | ||
return ( | ||
<div className={classNames(className, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More hardcoded strings that still need to be i18n'ed before merging.
})} | ||
> | ||
<div> | ||
<button className={styles.arrow} tabIndex={0} onClick={toggleOpen}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep using button
here it also won't require tabIndex
, since that's the default value of the button
element anyway.
airbyte-webapp/src/components/ui/CollapsablePanel/CollapsablePanel.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/ui/CollapsablePanel/CollapsablePanel.tsx
Outdated
Show resolved
Hide resolved
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [_fullUrl, account, job] = matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ We don't assign unused variables any name, but instead skip them. This is happening in a couple of places around this PR, so we should try to remove all of those.
airbyte-webapp/src/components/ui/CollapsablePanel/CollapsablePanel.tsx
Outdated
Show resolved
Hide resolved
const { operationId } = operation; | ||
const { executionUrl } = operation.operatorConfiguration.webhook || {}; | ||
|
||
const matches = (executionUrl || "").match(/\/accounts\/([^/]+)\/jobs\/([^]+)\//); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, does it mean the job actually only stores the full execution URL, while we're entering in the UI individual separate fields?
I think the backend should store the fields we want the user to actually enter, and just build the executionUrl when doing the request. Parsing this client side sounds like a fragile method, and if we don't store this data now, we might have a problem addressing this in a backwards compatible way easily, since we already lost the individual field information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout! We did discuss this a bit.
The backend is actually unaware of dbt accounts/jobs, or dbt cloud at all - it's a "webhook operation", and it just holds the plain URL. dbt Cloud is a totally frontend concept at the moment.
With that said, I do think a better approach would be what you're describing, and the way I'd do it would be to accept the URL "template" that includes named params; plus a map of the params. e.g., "https://cloud.getdbt.com/api/v2/accounts/{accountId}/jobs/{jobId}/run/", {"accountId": "x", "jobId": "y"}
.
I think for existing configs we would represent this like "https://cloud.getdbt.com/api/v2/accounts/x/jobs/y/run/", {}
. Since dbt Cloud is going to be the only "webhook operation" integration in the UI for the foreseeable future, once we have the template, params
representation we can run a targeted migration since we know what these look like, and then we'd be able to kill this bit of messy FE code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davinchia any thoughts here? I do agree we should make the change in the backend ~asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% agree; I think there should be cloud endpoints which are specific to dbt Cloud, return the easy-to-use DbtCloudJob
objects, and, down the line, support UI niceties like selecting dbt jobs from a dropdown (which has to go through the backend for CORS reasons) and the fact that these are backed by a generic webhook record remaining an implementation detail hidden from the frontend code. (In fact, it's one of the primary reasons I tried to keep all the API interactions contained to this single file.) It came up a bit in earlier discussions and it's something I'm going to advocate for in the immediate clean-up planning/spec work. Thoughts, @mfsiega-airbyte?
For what it's worth, this seems like a reasonable PR sequencing of that change if we do choose to ship the MVP with these API contracts still in place:
- first an "append-only" PR to
airbyte-cloud
adding new dbt-Cloud-specific APIs (which wouldn't yet be used); - then full-stack PR to
airbyte
to define and generate those new APIs and replace all the webhook-oriented code in this file with them.
That said, in terms of this PR, I didn't have a working instance of this more minimal backend implementation to develop and experiment with until this weekend, so I had little choice but to run with what was available to get something working for the demo.
const connectionService = useWebConnectionService(); | ||
|
||
// TODO extract shared isDbtWebhookConfig predicate | ||
const hasDbtIntegration = !isEmpty(workspace.webhookConfigs?.filter((config) => /dbt/.test(config.name || ""))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 This looks like it could just be an .includes
instead of a regexp (same the line below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got paranoid about locking in our ability to edit the webhook name if we decide to tweak the naming convention, but I think you're right that it would be cleaner just checking for the actual value, since it's not user-supplied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether we meant the same thing, so just for clarification, I suggested just replacing the regexp by an includes not the whole filter statement, i.e.:
const hasDbtIntegration = !isEmpty(workspace.webhookConfigs?.filter((config) => /dbt/.test(config.name || ""))); | |
const hasDbtIntegration = !isEmpty(workspace.webhookConfigs?.filter((config) => config.name?.includes("dbt"))); |
...ionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/packages/cloud/views/settings/integrations/DbtCloudSettingsView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Demoed it in pairprogramming and looked over the code. Seems to be good to merge for the demo, but will require several cleanup work afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left several notes for cleanup. We try to not overdo it on Octavia illustrations in the product, to get to a more professional looking design. Maybe you could sync with Nico shortly about the empty state to get some ideas and input there.
<div className={classNames(styles.jobListContainer, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> | ||
{jobs.length ? ( | ||
jobs.map((_j, i) => <JobsListItem key={i} jobIndex={i} removeJob={() => remove(i)} />) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 Unused variable should simply be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bindings in an argument list can't be skipped like they can when destructuring, though: it's a syntax error.
throw new Error(`Cannot extract dbt cloud job params from executionUrl ${executionUrl}`); | ||
} else { | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [_fullUrl, account, job] = matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 Other unused variable, that should be skipped in destructuring.
) : ( | ||
<> | ||
<img src="/images/octavia/worker.png" alt="An octopus wearing a hard hat, tools at the ready" /> | ||
No transformations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18n needed
)} | ||
<div className={styles.jobListButtonGroup}> | ||
<Button className={styles.jobListButton} type="reset" variant="secondary"> | ||
Cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18n needed
Cancel | ||
</Button> | ||
<Button className={styles.jobListButton} type="submit" variant="primary" disabled={!dirty || !isValid}> | ||
Save changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18n needed
const dbtSettingsPath = `/${RoutePaths.Workspaces}/${workspaceId}/${RoutePaths.Settings}/dbt-cloud`; | ||
return ( | ||
<div className={classNames(className, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should :) It will also solve some default browser margin issues on p
.
const dbtSettingsPath = `/${RoutePaths.Workspaces}/${workspaceId}/${RoutePaths.Settings}/dbt-cloud`; | ||
return ( | ||
<div className={classNames(className, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this sentence in the integration not setup state, since it feels a bit weird telling, that the following transformations will run, while we know there are no transformations, and the user isn't setup yet. I'd maybe phrase it more like:
"To run dbt Cloud transformations when syncing data, setup your dbt Cloud account in the settings."
isValid: boolean; | ||
dirty: boolean; | ||
}) => ( | ||
<div className={classNames(styles.jobListContainer, styles.emptyListContent)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit weird, that we're attaching the emptyListContent
classname here, no matter whether this list is empty or not.
dirty: boolean; | ||
}) => ( | ||
<div className={classNames(styles.jobListContainer, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be <Text>
(as Joey mentioned on an occasion below).
dirty: boolean; | ||
}) => ( | ||
<div className={classNames(styles.jobListContainer, styles.emptyListContent)}> | ||
<p className={styles.contextExplanation}>After an Airbyte sync job has completed, the following jobs will run</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in case there are no jobs setup yet, we should not show this message, since we know there are no "following jobs". Maybe the empty state could be simply the image with the label below stating something along the lines of:
"No Transformations. You can add transformations to run when sync jobs have completed."
return ( | ||
<Card className={styles.jobListItem}> | ||
<div className={styles.jobListItemIntegrationName}> | ||
<img src="/images/external/dbt-bit_tm.png" alt="dbt logo" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dbt offers their logos also in SVG format. We should exchange this via their SVG version.
* master: (304 commits) Bump helm chart version reference to 0.40.27 (#18152) Bump helm chart version reference to 0.40.26 (#18094) Update deployment.yaml (#18151) Publishes Postgres, MySQL, MSSQL source with changes from #18041 (#18086) Fix minor DBT Cloud Errors. (#18147) Sentry Integration : Stop reporting all non system-error error types. (#18133) Docs: Fix backoff stategy docs (#18143) 🐛 Destination GCS: Fix error logs to log 'Gcs' rather than 'AWS' (#17901) Add openAPI spec for Connector Builder Server (#17535) Alex/mvp UI for dbt cloud integration (#18095) increased timeout for sat tests (#18128) Bmoric/remove dep connector worker (#17977) `recordsRead` should be a long (#18123) doc_update_oath_issue_gsc (#17967) 🎉 Source Zendesk Chat: engagements data fix infinity looping + gradlew format (#18121) 🐛 Source Zendesk Chat: engagements data fix infinity looping (#17745) Custom APM Tracing (#17947) 11679 BigQuery-Denormalized Destination: improve code coverage (#17827) increased timeout for sat tests (#18114) docs: clarify language (#18090) ...
* Add lightly-styled ui for dbt cloud settings * Add CollapsablePanel component * Add CollapsablePanel around url input, MVP styling To get the styling to work, I needed to edit `LabeledInput` to accept a `className` prop, so I could give it contextually-specific styling. * Add new feature flag for dbt cloud integration This feature isn't added to either OSS or cloud builds; it will be dynamically toggled for specific targeted accounts via LaunchDarkly's `featureService.overwrites` key. * Put settings page dbt cloud ui behind feature flag * Add feature-flagged CloudTransformationsCard * Extract (and rename) DbtCloudTransformationsCard * Extract EmptyTransformationList component * List transformations if any, "no integration" UI This still uses some hardcoded conditions instead of anything resembling actual data * Initial UI for cloud transform jobs * Use formik-backed inputs for job list data fields * Improve job list management with FieldArray et al * WIP: build payload to save job data as operations There's some key data missing and it's not currently wired up * Start pulling dbt cloud business logic to its own module * Renaming pass (s/transformation/job/g) * Move more logic into dbt service module * Renaming pass (s/project/account/) * Improve useDbtIntegration hook * Add skeleton of updateWorkspace fn * Connect pages to actual backend (no new jobs tho) * Add hacky initial add new job implementation * Put the whole dbt cloud card inside FieldArray This dramatically simplifies adding to the list of jobs. * Fix button placement, loss of focus on input Never use the input prop in your component key, kids. * re-extract DbtJobsList component * Add input labels for dbt cloud job list * Validate dbt cloud jobs so bad data doesn't crash the party * Fix typo * Improve dirty form tracking for dbt jobs list * Remove unused input, add loading state to dbt cloud settings view * Handle no integration, dirty states in dbt jobs list Co-authored-by: Alex Birdsall <[email protected]>
What
Prototype/MVP UI for dbt cloud integration.
How
Describe the solution
Recommended reading order
packages/cloud/services/dbtCloud.ts
: manipulating backend data and parsing the generic webhook data types into something more convenient for the UIpages/ConnectionPage/pages/ConnectionItemPage/ConnectionTransformationTab.tsx
: the conditions when the new UI appearspages/ConnectionPage/pages/ConnectionItemPage/ConnectionTransformationTab/DbtCloudTransformationsCard.tsx
: the complicated bits of the new UIpackages/cloud/views/settings/integrations/DbtCloudSettingsView.tsx
: the settings UI for saving auth tokens🚨 User Impact 🚨
Should be none; all new UI is behind feature flags and off by default
Corners cut
Tests
It sure would have been nice to have time to write some!