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

🪟 🐛 Fix direct job linking to work with pagination #16517

Merged
merged 24 commits into from
Sep 14, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
8eb89b8
start implementation of new persistence method
lmossman Sep 7, 2022
88a99d8
add includingJobId and totalJobCount to job list request
lmossman Sep 7, 2022
9002461
format
lmossman Sep 7, 2022
80c213e
update local openapi as well
lmossman Sep 7, 2022
aeaffad
refactor queries into JOOQ and return empty list if target job cannot…
lmossman Sep 8, 2022
df71109
fix descriptions and undo changes from other branch
lmossman Sep 8, 2022
d68fc5a
switch including job to starting job
lmossman Sep 8, 2022
ac750d9
fix job history handler tests
lmossman Sep 8, 2022
ef2d6dc
rewrite jobs subqueries in jooq
lmossman Sep 8, 2022
6419233
fix multiple config type querying
lmossman Sep 8, 2022
84b8dbe
remove unnecessary casts
lmossman Sep 8, 2022
5d53ab7
switch back to 'including' and return multiple of page size necessary…
lmossman Sep 8, 2022
5a5adea
undo webapp changes
lmossman Sep 8, 2022
ed71403
fix test description
lmossman Sep 8, 2022
c991275
format
lmossman Sep 9, 2022
09d8b92
Merge branch 'master' into lmossman/add-including-job-to-list-jobs-api
lmossman Sep 12, 2022
be27e62
save progress
lmossman Sep 8, 2022
ac7e103
use list job api changes to retrieve linked job if linked
lmossman Sep 9, 2022
97958e1
change job not found wording
lmossman Sep 12, 2022
93c4d73
Update airbyte-webapp/src/services/job/JobService.tsx
lmossman Sep 12, 2022
a787b05
Merge branch 'master' into lmossman/fix-direct-job-linking
lmossman Sep 12, 2022
cdf4a59
merge with master
lmossman Sep 14, 2022
9264180
Merge branch 'master' into lmossman/fix-direct-job-linking
lmossman Sep 14, 2022
85e31da
Merge branch 'master' into lmossman/fix-direct-job-linking
lmossman Sep 14, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion airbyte-webapp/src/components/JobItem/JobItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const JobItem: React.FC<JobItemProps> = ({ job }) => {
const didSucceed = didJobSucceed(job);

const onExpand = () => {
setIsOpen(!isOpen);
setIsOpen((prevIsOpen) => !prevIsOpen);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updating this to use the best practice of using the arrow function for a change to a state value that depends on itself

};

const onDetailsToggled = useCallback(() => {
Expand Down
2 changes: 2 additions & 0 deletions airbyte-webapp/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@
"connection.cancelSync": "Cancel Sync",
"connection.cancelReset": "Cancel Reset",
"connection.canceling": "Canceling...",
"connection.linkedJobNotFound": "Job not found",
"connection.returnToSyncHistory": "Return to Sync History",

"form.frequency": "Replication frequency*",
"form.frequency.placeholder": "Select a frequency",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import { faXmark } from "@fortawesome/free-solid-svg-icons";
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
import React, { useEffect, useState } from "react";
import { FormattedMessage } from "react-intl";
import { Link, useLocation } from "react-router-dom";

import { Button, LoadingButton } from "components";
import { Card } from "components/base/Card";
import { Tooltip } from "components/base/Tooltip";
import EmptyResource from "components/EmptyResourceBlock";
import { RotateIcon } from "components/icons/RotateIcon";
import { useAttemptLink } from "components/JobItem/attemptLinkUtils";

import { getFrequencyType } from "config/utils";
import { Action, Namespace } from "core/analytics";
Expand Down Expand Up @@ -52,15 +54,23 @@ const StatusView: React.FC<StatusViewProps> = ({ connection }) => {
const [activeJob, setActiveJob] = useState<ActiveJob>();
const [jobPageSize, setJobPageSize] = useState(JOB_PAGE_SIZE_INCREMENT);
const analyticsService = useAnalyticsService();
const { jobs, isPreviousData: isJobPageLoading } = useListJobs({
const { jobId: linkedJobId } = useAttemptLink();
const { pathname } = useLocation();
const {
jobs,
totalJobCount,
isPreviousData: isJobPageLoading,
} = useListJobs({
configId: connection.connectionId,
configTypes: ["sync", "reset_connection"],
includingJobId: linkedJobId ? Number(linkedJobId) : undefined,
pagination: {
pageSize: jobPageSize,
},
});

const moreJobPagesAvailable = jobs.length === jobPageSize;
const linkedJobNotFound = linkedJobId && jobs.length === 0;
const moreJobPagesAvailable = !linkedJobNotFound && jobPageSize < totalJobCount;

useEffect(() => {
const jobRunningOrPending = getJobRunningOrPending(jobs);
Expand All @@ -74,6 +84,9 @@ const StatusView: React.FC<StatusViewProps> = ({ connection }) => {
// We need to disable button when job is canceled but the job list still has a running job
} as ActiveJob)
);

// necessary because request to listJobs may return a result larger than the current page size if a linkedJobId is passed in
setJobPageSize((prevJobPageSize) => Math.max(prevJobPageSize, jobs.length));
}, [jobs]);

const { openConfirmationModal, closeConfirmationModal } = useConfirmationModalService();
Expand Down Expand Up @@ -171,7 +184,20 @@ const StatusView: React.FC<StatusViewProps> = ({ connection }) => {
</div>
}
>
{jobs.length ? <JobsList jobs={jobs} /> : <EmptyResource text={<FormattedMessage id="sources.noSync" />} />}
{jobs.length ? (
<JobsList jobs={jobs} />
) : linkedJobNotFound ? (
<EmptyResource
text={<FormattedMessage id="connection.linkedJobNotFound" />}
description={
<Link to={pathname}>
<FormattedMessage id="connection.returnToSyncHistory" />
</Link>
}
/>
Comment on lines +189 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead of showing this, what if it showed an error notification at the bottom of the screen but still showed the jobs list? I suppose that in this case, the API would need to still return the jobs as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was how I had originally implemented the backend API, but the backend team pushed back on that a bit saying it made the endpoint somewhat confusing to return results even if the specific job ID could not be found (it was basically behaving in two different ways depending on that), which I kind of agreed with.

I don't expect users to run into this situation very often, since it would require them to manually edit the job ID in the URL, so hopefully having this separate view with a single click to get back to the main job view is not too cumbersome

) : (
<EmptyResource text={<FormattedMessage id="sources.noSync" />} />
)}
</Card>

{(moreJobPagesAvailable || isJobPageLoading) && (
Expand Down
24 changes: 15 additions & 9 deletions airbyte-webapp/src/services/job/JobService.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,16 @@ import {
JobDebugInfoRead,
JobInfoRead,
JobListRequestBody,
JobWithAttemptsRead,
JobReadList,
Pagination,
} from "../../core/request/AirbyteClient";
import { useSuspenseQuery } from "../connector/useSuspenseQuery";

export const jobsKeys = {
all: ["jobs"] as const,
lists: () => [...jobsKeys.all, "list"] as const,
list: (filters: string, pagination?: Pagination) => [...jobsKeys.lists(), { filters, pagination }] as const,
list: (filters: string, includingJobId?: number, pagination?: Pagination) =>
[...jobsKeys.lists(), { filters, includingJobId, pagination }] as const,
detail: (jobId: number) => [...jobsKeys.all, "details", jobId] as const,
getDebugInfo: (jobId: number) => [...jobsKeys.all, "getDebugInfo", jobId] as const,
cancel: (jobId: string) => [...jobsKeys.all, "cancel", jobId] as const,
Expand All @@ -31,13 +32,18 @@ function useGetJobService() {

export const useListJobs = (listParams: JobListRequestBody) => {
const service = useGetJobService();
const result = useQuery(jobsKeys.list(listParams.configId, listParams.pagination), () => service.list(listParams), {
refetchInterval: 2500, // every 2,5 seconds,
keepPreviousData: true,
suspense: true,
});
// cast to JobWithAttemptsRead[] because (suspense: true) means we will never get undefined
return { jobs: result.data?.jobs as JobWithAttemptsRead[], isPreviousData: result.isPreviousData };
const result = useQuery(
jobsKeys.list(listParams.configId, listParams.includingJobId, listParams.pagination),
() => service.list(listParams),
{
refetchInterval: 2500, // every 2,5 seconds,
keepPreviousData: true,
suspense: true,
}
);
// cast to JobReadList because (suspense: true) means we will never get undefined
const jobReadList = result.data as JobReadList;
return { jobs: jobReadList.jobs, totalJobCount: jobReadList.totalJobCount, isPreviousData: result.isPreviousData };
};

export const useGetJob = (id: number, enabled = true) => {
Expand Down