🪟 🐛 Fix direct job linking to work with pagination #16517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/platform
lmossman
commented
Sep 9, 2022
| @@ -48,7 +48,7 @@ export const JobItem: React.FC<JobItemProps> = ({ job }) => { | |||
| const didSucceed = didJobSucceed(job); | |||
|
|
|||
| const onExpand = () => { | |||
| setIsOpen(!isOpen); | |||
| setIsOpen((prevIsOpen) => !prevIsOpen); | |||
There was a problem hiding this comment.
Just updating this to use the best practice of using the arrow function for a change to a state value that depends on itself
lmossman
force-pushed
the
lmossman/add-including-job-to-list-jobs-api
branch
from
c57a400 to
ed71403
Compare
edmundito
reviewed
Sep 12, 2022
airbyte-webapp/src/locales/en.json
Outdated
| @@ -361,6 +361,8 @@ | |||
| "connection.cancelSync": "Cancel Sync", | |||
| "connection.cancelReset": "Cancel Reset", | |||
| "connection.canceling": "Canceling...", | |||
| "connection.linkedJobNotFound": "The linked job could not be found", | |||
There was a problem hiding this comment.
Could also be "Job not found"
Comment on lines
+188
to
+196
| ) : linkedJobNotFound ? ( | ||
| <EmptyResource | ||
| text={<FormattedMessage id="connection.linkedJobNotFound" />} | ||
| description={ | ||
| <Link to={pathname}> | ||
| <FormattedMessage id="connection.returnToSyncHistory" /> | ||
| </Link> | ||
| } | ||
| /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
lmossman
force-pushed
the
lmossman/fix-direct-job-linking
branch
from
4878e2c to
ac7e103
Compare
Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>
Base automatically changed from
lmossman/add-including-job-to-list-jobs-api
to
master
September 12, 2022 23:32
edmundito
approved these changes
Sep 14, 2022
letiescanciano
added a commit
that referenced
this pull request
Sep 15, 2022
* master: (200 commits) 🪟 🧹 Display returned error messages on replication view (#16280) 🎉 Source mixpanel: Use "Retry-After" header for backoff (#16770) 🐛 Source google ads: mark custom query fields required (#15858) 🪟 🔧Remove useRouter hook (#16598) CDK: improve TypeTransformer to convert simple types to array of simple types (#16636) CDK: TypeTransformer - warning message more informative (#16695) Source MySQL: Add Python SAT to detect backwards breaking changes (#16445) remove eager (#16756) bump com.networknt:json-schema-validator to latest version (#16619) Remove Cloud from Kafka docs (#16753) Normalization Summaries table and read/write methods (#16655) comment out flaky test suite while it is being investigated (#16752) Update ConfigRepository to read protocol version (#16670) Use LOG4J2 to wrap connectors logs to JSON format (#15668) Update connector catalog (#16749) 🪟 🎨 Remove feedback modal from UI (#16548) Add missing env var for Kube overlays (#16747) Prepare for React v18 upgrade (#16694) 🪟 🐛 Fix direct job linking to work with pagination (#16517) Fix formatting (#16743) ...
robbinhan
pushed a commit
to robbinhan/airbyte
that referenced
this pull request
Sep 29, 2022
* start implementation of new persistence method * add includingJobId and totalJobCount to job list request * format * update local openapi as well * refactor queries into JOOQ and return empty list if target job cannot be found * fix descriptions and undo changes from other branch * switch including job to starting job * fix job history handler tests * rewrite jobs subqueries in jooq * fix multiple config type querying * remove unnecessary casts * switch back to 'including' and return multiple of page size necessary to include job * undo webapp changes * fix test description * format * save progress * use list job api changes to retrieve linked job if linked * change job not found wording * Update airbyte-webapp/src/services/job/JobService.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>
jhammarstedt
pushed a commit
to jhammarstedt/airbyte
that referenced
this pull request
Oct 31, 2022
* start implementation of new persistence method * add includingJobId and totalJobCount to job list request * format * update local openapi as well * refactor queries into JOOQ and return empty list if target job cannot be found * fix descriptions and undo changes from other branch * switch including job to starting job * fix job history handler tests * rewrite jobs subqueries in jooq * fix multiple config type querying * remove unnecessary casts * switch back to 'including' and return multiple of page size necessary to include job * undo webapp changes * fix test description * format * save progress * use list job api changes to retrieve linked job if linked * change job not found wording * Update airbyte-webapp/src/services/job/JobService.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Resolves #16369
See above linked issue for context.
How
This PR solves the problem by utilizing the list job API improvements made in #16415 to directly fetch the desired list of jobs when a job ID is linked to in the URL. This means that if someone links to a job outside of the first page of jobs, this view will immediately load the necessary number of pages required to include the linked job, as demonstrated in this screen recording:
Screen.Recording.2022-09-08.at.5.50.06.PM.mov
This PR also adds a fallback view for the edge case where someone puts an invalid job ID into the URL, with a link back to the normal sync history:
Screen.Recording.2022-09-08.at.5.52.42.PM.mov
Lastly, this PR also contains a small fix to utilize the new
totalJobCountproperty in the list jobs response, to decide if there are more jobs to load and therefore whether the Load more button should be shown.Recommended reading order
StatusView.tsx- main changes to the sync history viewJobService.tsx- adding theincludingJobIdto the react-query keys, so that jobs are properly fetched when this is changed🚨 User Impact 🚨
Direct job linking should now work as expected again.