Skip to content

Fixes #38591 - Api calls refactor#988

Merged
MariaAga merged 1 commit intotheforeman:masterfrom
Lukshio:38591-api_refactor
Sep 19, 2025
Merged

Fixes #38591 - Api calls refactor#988
MariaAga merged 1 commit intotheforeman:masterfrom
Lukshio:38591-api_refactor

Conversation

@Lukshio
Copy link
Copy Markdown
Contributor

@Lukshio Lukshio commented Jul 22, 2025

Remove useApi duplicates, unify calls

@Lukshio Lukshio force-pushed the 38591-api_refactor branch 3 times, most recently from 5e5a4e9 to c0053b6 Compare July 22, 2025 14:26
@Lukshio Lukshio force-pushed the 38591-api_refactor branch from c0053b6 to 22571d0 Compare July 22, 2025 14:49
@MariaAga
Copy link
Copy Markdown
Member

This makes it so if a users selects all hosts, and opens in a new tab, only the first page is opened, instead of the first 100

@adamruzicka adamruzicka requested a review from Copilot July 23, 2025 14:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors API calls in the JobInvocationDetail component to remove duplicated useApi hooks and unify API call handling. The changes replace multiple useAPI hooks with a single dispatch-based approach using Redux APIActions, and reorganize the code structure for better maintainability.

Key changes:

  • Replaced multiple useAPI hooks with unified Redux dispatch approach using APIActions.get
  • Restructured component state management and API response handling
  • Added new constant JOB_INVOCATION_PAGE for API key management

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
JobInvocationHostTable.js Major refactor replacing useAPI hooks with Redux dispatch, restructuring state management and API call logic
JobInvocationConstants.js Added new constant JOB_INVOCATION_PAGE for API key management

@adamruzicka
Copy link
Copy Markdown
Contributor

Needs a rebase

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 22571d0 to 02696f9 Compare July 23, 2025 18:04
@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Jul 23, 2025

This makes it so if a users selects all hosts, and opens in a new tab, only the first page is opened, instead of the first 100

Can you please retest it now? Opening in new tabs has its own limit

if (selectedIds.length <= DIRECT_OPEN_HOST_LIMIT) {
selectedIds.forEach(id => openLink(templateInvocationPageUrl(id, jobID)));
return;
}

@MariaAga
Copy link
Copy Markdown
Member

since nothing calls for the first 100 ( per_page = MAX_HOSTS_API_SIZE) it still only opens the first page instead of the first 100

@MariaAga
Copy link
Copy Markdown
Member

also it auto sets per page to 20 instead of the user per_page

@MariaAga
Copy link
Copy Markdown
Member

changing per_page adds brackets around the search, and then every page change adds more brackets, searching by clicking on the chart doesnt work anymore

@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Jul 29, 2025

changing per_page adds brackets around the search, and then every page change adds more brackets, searching by clicking on the chart doesnt work anymore

Fixed

also it auto sets per page to 20 instead of the user per_page

Fixed

since nothing calls for the first 100 ( per_page = MAX_HOSTS_API_SIZE) it still only opens the first page instead of the first 100

I couldn't replicate this issue

@MariaAga
Copy link
Copy Markdown
Member

to replicate - have a job run on 21+ hosts, open the job details page, make sure per page is less than the amount of hosts (usually set to 20) click on the open all tab, the number shown in the modal is the per page number, and not all hosts number

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from e77f3c5 to ee2a4a9 Compare July 31, 2025 08:59
@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Jul 31, 2025

to replicate - have a job run on 21+ hosts, open the job details page, make sure per page is less than the amount of hosts (usually set to 20) click on the open all tab, the number shown in the modal is the per page number, and not all hosts number

Added old MAX_HOSTS const to load only first 100

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from ee2a4a9 to 8209cd4 Compare July 31, 2025 09:11
@adamruzicka
Copy link
Copy Markdown
Contributor

It seems we're polling periodically, without waiting for the previous poll to finish? Could we do something about that?

image

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 8209cd4 to f8459cf Compare July 31, 2025 13:16
@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Jul 31, 2025

After discussion with @MariaAga new behavior is:

  • Open in new tabs in applied to current page if not selected, same for failed open
  • All api calls are reduced to 1 which handles ids for selection, failed, etc
  • MAX_HOSTS is no longer in use with API, but still with max opened tabs

@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Aug 4, 2025

The open in new tab button - should open the page if nothing is selected, should not have the text "The maximum is 100.«", and should have a matching text (in the modal and tooltip) if its opening the selected in the page or the just the page. open all failed - should say open all failed runs from this page, the modal also shouldnt say "the maximum is 100.«", also ,it doesnt open any tabs when clicked

Unable to reproduce not opening page if nothing selected, other fixed

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 54ea3ed to 51dd9b9 Compare August 4, 2025 10:49
@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Aug 4, 2025

The open in new tab button - should open the page if nothing is selected, should not have the text "The maximum is 100.«", and should have a matching text (in the modal and tooltip) if its opening the selected in the page or the just the page. open all failed - should say open all failed runs from this page, the modal also shouldnt say "the maximum is 100.«", also ,it doesnt open any tabs when clicked

Unable to reproduce not opening page if nothing selected, other fixed

Ad. refactored opening new tabs, now it should be working

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 51dd9b9 to c4013a6 Compare August 4, 2025 11:25
@Lukshio Lukshio force-pushed the 38591-api_refactor branch from c4013a6 to ef0bc78 Compare August 6, 2025 13:49
@MariaAga
Copy link
Copy Markdown
Member

MariaAga commented Aug 6, 2025

clicking the chart seems to add conditions instead of replacing them
(job_invocation.result = failed) AND ((job_invocation.result = success) AND ((job_invocation.result = failed) AND ((job_invocation.result = cancelled) AND ((job_invocation.result = pending) AND ((job_invocation.result = success) AND ((job_invocation.result = failed)))))))

@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Aug 7, 2025

clicking the chart seems to add conditions instead of replacing them (job_invocation.result = failed) AND ((job_invocation.result = success) AND ((job_invocation.result = failed) AND ((job_invocation.result = cancelled) AND ((job_invocation.result = pending) AND ((job_invocation.result = success) AND ((job_invocation.result = failed)))))))

re fixed it, hopefully this will stop the circle of fixing something, and breaking something else :)

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from ef0bc78 to b9c61d5 Compare August 7, 2025 10:50
Copy link
Copy Markdown
Member

@MariaAga MariaAga left a comment

Choose a reason for hiding this comment

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

Seems to work as expected now! thats quite a lot of changes, @kmalyjur can you take a look as well to make sure I'm not missing anything?

Copy link
Copy Markdown
Contributor

@kmalyjur kmalyjur left a comment

Choose a reason for hiding this comment

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

Thank you, it works as it should, and the code is great except for these minor things. 🎆

Copy link
Copy Markdown
Contributor

@kmalyjur kmalyjur left a comment

Choose a reason for hiding this comment

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

Thank you for your edits! I'll be a bit annoying and leave one last comment, but after that, it should be ready to get merged. However, after yesterday's discussion on the planning, we'll need to wait a little before merging.

@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 0baa83c to 6a7e3e8 Compare August 13, 2025 14:14
@Lukshio Lukshio force-pushed the 38591-api_refactor branch from 6a7e3e8 to 3199f86 Compare August 13, 2025 14:29
@Lukshio
Copy link
Copy Markdown
Contributor Author

Lukshio commented Aug 13, 2025

Rebase

@MariaAga MariaAga merged commit 8dd7ea3 into theforeman:master Sep 19, 2025
25 checks passed
@MariaAga
Copy link
Copy Markdown
Member

Thanks everyone!

@Lukshio Lukshio deleted the 38591-api_refactor branch October 20, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants