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(bulk-import): updated bulk import ui to show the correct import jobs count #2315

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

debsmita1
Copy link
Member

@debsmita1 debsmita1 commented Oct 7, 2024

Resolves:

Note: Used @tanstack/react-query for querying the import jobs

Screenshots:

When the number of rows listed is less than the number of rows selected
(https://issues.redhat.com/browse/RHIDP-3868)
Screenshot 2024-10-16 at 1 45 48 PM

@debsmita1 debsmita1 requested a review from a team as a code owner October 7, 2024 19:29
@debsmita1 debsmita1 changed the title fix(bulk-import): Updated bulk import ui to show the correct import jobs count fix(bulk-import): updated bulk import ui to show the correct import jobs count Oct 7, 2024
@debsmita1 debsmita1 changed the title fix(bulk-import): updated bulk import ui to show the correct import jobs count [WIP] fix(bulk-import): updated bulk import ui to show the correct import jobs count Oct 8, 2024
@debsmita1 debsmita1 changed the title [WIP] fix(bulk-import): updated bulk import ui to show the correct import jobs count fix(bulk-import): updated bulk import ui to show the correct import jobs count Oct 8, 2024
@debsmita1 debsmita1 requested a review from rm3l October 8, 2024 13:21
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

@debsmita1 I just tested this PR, and can confirm both https://issues.redhat.com/browse/RHIDP-4067 and https://issues.redhat.com/browse/RHIDP-3868 are fixed.

However, I noticed what I think could be a regression: when trying to import new repos, the "Create pull request" button remains disabled regardless of the repos selected. See the screencast attached:

review-pr-2315-2024-10-09_11.07.04.mp4

Do you confirm this behavior on your end?

@rm3l
Copy link
Member

rm3l commented Oct 9, 2024

@debsmita1 I just tested this PR, and can confirm both issues.redhat.com/browse/RHIDP-4067 and issues.redhat.com/browse/RHIDP-3868 are fixed.

However, I noticed what I think could be a regression: when trying to import new repos, the "Create pull request" button remains disabled regardless of the repos selected. See the screencast attached:

review-pr-2315-2024-10-09_11.07.04.mp4
Do you confirm this behavior on your end?

I can also confirm this issue does not occur on the main branch.

@debsmita1 debsmita1 force-pushed the update-bi-imports branch 2 times, most recently from 354ca95 to c73b5d9 Compare October 9, 2024 19:20
@debsmita1 debsmita1 requested a review from rm3l October 10, 2024 07:05
@debsmita1
Copy link
Member Author

@debsmita1 I just tested this PR, and can confirm both https://issues.redhat.com/browse/RHIDP-4067 and https://issues.redhat.com/browse/RHIDP-3868 are fixed.

However, I noticed what I think could be a regression: when trying to import new repos, the "Create pull request" button remains disabled regardless of the repos selected. See the screencast attached:

review-pr-2315-2024-10-09_11.07.04.mp4
Do you confirm this behavior on your end?

Fixed it. PTAL

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

@debsmita1 I just tested this PR, and can confirm both issues.redhat.com/browse/RHIDP-4067 and issues.redhat.com/browse/RHIDP-3868 are fixed.
However, I noticed what I think could be a regression: when trying to import new repos, the "Create pull request" button remains disabled regardless of the repos selected. See the screencast attached:
review-pr-2315-2024-10-09_11.07.04.mp4
Do you confirm this behavior on your end?

Fixed it. PTAL

It works better now - thanks. LGTM, but I'll let frontend experts take a look as well.

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Some small comments and questions:

@debsmita1 debsmita1 force-pushed the update-bi-imports branch 2 times, most recently from facd635 to b92f16c Compare October 16, 2024 11:16
Copy link

changeset-bot bot commented Oct 16, 2024

🦋 Changeset detected

Latest commit: 5fd57b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@janus-idp/backstage-plugin-bulk-import Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@debsmita1 debsmita1 requested a review from a team as a code owner October 16, 2024 11:31
Copy link

sonarcloud bot commented Oct 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

<RepositoriesList />
</FormControl>
</Formik>
<QueryClientProvider client={queryClientRef.current as QueryClient}>

Choose a reason for hiding this comment

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

as always trigger me ;)

Suggested change
<QueryClientProvider client={queryClientRef.current as QueryClient}>
<QueryClientProvider client={queryClientRef.current!}>

Comment on lines +124 to +126
values?.repositories?.[data.id]?.catalogInfoYaml?.prTemplate
?.pullRequestUrl ||
values?.repositories[data.id]?.repoUrl ||
values?.repositories?.[data.id]?.repoUrl ||

Choose a reason for hiding this comment

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

Anyway, the code generates a link that isn't clickable in some cases.

With the recommended check we can ensure that we don't create a button that doesn't open a valid link.

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Tiny nits

const classes = useStyles();

return (
<TableRow hover key={data.id}>

Choose a reason for hiding this comment

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

nit, but I guess the key here isn't needed.

Comment on lines +54 to +57
<Link to={data?.repoUrl || ''}>
{urlHelper(data?.repoUrl || '')}
<OpenInNewIcon style={{ verticalAlign: 'sub', paddingTop: '7px' }} />
</Link>

Choose a reason for hiding this comment

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

As mentioned in other places. We should avoid rendering broken links.

Comment on lines +60 to +63
<Link to={data?.organizationUrl || ''}>
{urlHelper(data?.organizationUrl || '')}
<OpenInNewIcon style={{ verticalAlign: 'sub', paddingTop: '7px' }} />
</Link>

Choose a reason for hiding this comment

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

As mentioned in other places. We should avoid rendering broken links.

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Please take care of the left overs as part of your other bulk import PR.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 20, 2024
@christoph-jerolimov
Copy link
Member

/override SonarCloud Code Analysis

Copy link

openshift-ci bot commented Oct 20, 2024

@jerolimov: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Analysis
  • Code
  • SonarCloud

Only the following failed contexts/checkruns were expected:

  • Build with Node.js 20
  • Conventional Commits
  • SonarCloud Code Analysis
  • Test with Node.js 20
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override SonarCloud Code Analysis

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@christoph-jerolimov
Copy link
Member

/override "SonarCloud Code Analysis"

Copy link

openshift-ci bot commented Oct 20, 2024

@jerolimov: Overrode contexts on behalf of jerolimov: SonarCloud Code Analysis

In response to this:

/override "SonarCloud Code Analysis"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 7a28963 into janus-idp:main Oct 20, 2024
9 of 10 checks passed
04kash pushed a commit to 04kash/backstage-plugins that referenced this pull request Oct 23, 2024
…obs count (janus-idp#2315)

[Bulk import]: fix added repositories count
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants