Skip to content

Fixes #39246 - Add sorting and searching to the new Leapp preupgrade report#171

Merged
Lukshio merged 13 commits into
theforeman:masterfrom
kmalyjur:job-inv-leapp-filter
Jul 3, 2026
Merged

Fixes #39246 - Add sorting and searching to the new Leapp preupgrade report#171
Lukshio merged 13 commits into
theforeman:masterfrom
kmalyjur:job-inv-leapp-filter

Conversation

@kmalyjur

@kmalyjur kmalyjur commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

I’d appreciate an extra pair of eyes on the backend to confirm it's good, because I'm not sure about some parts.

EDIT: Dependent on theforeman/foreman#11060

Before:
image

After:
image

Comment thread app/controllers/api/v2/preupgrade_reports_controller.rb Outdated
Comment thread app/controllers/preupgrade_report_entries_controller.rb Outdated
Comment thread app/controllers/preupgrade_report_entries_controller.rb Outdated
@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch from 839459c to 9c13592 Compare April 29, 2026 14:58

@Lukshio Lukshio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The old sorting was more user friendly :/

Anyway there are some issues

I didn't find fix type in new filter.
Image

Comment thread app/views/api/v2/preupgrade_report_entries/index.json.rabl Outdated
Comment thread webpack/components/PreupgradeReportsTable/index.js Outdated
Comment thread webpack/components/PreupgradeReportsTable/index.js Outdated
@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch 3 times, most recently from cf0a4d1 to 05b5b8a Compare April 30, 2026 14:34
@kmalyjur

kmalyjur commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

DON'T REVIEW NOW, IT'S WORK IN PROGRESS.
Thank you for the reviews. I made quite a lot of changes to the backend, and it currently doesn't work.

To-do: add test/functional/api/v2/preupgrade_report_entries_controller_test.rb

The old sorting was more user friendly :/

@Lukshio do you mean searching? In my opinion, the new version is more consistent with the rest of the tables.

@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch from 05b5b8a to 9c4945f Compare April 30, 2026 15:09
@Lukshio

Lukshio commented May 3, 2026

Copy link
Copy Markdown
Contributor

DON'T REVIEW NOW, IT'S WORK IN PROGRESS.

Thank you for the reviews. I made quite a lot of changes to the backend, and it currently doesn't work.

To-do: add test/functional/api/v2/preupgrade_report_entries_controller_test.rb

The old sorting was more user friendly :/

@Lukshio do you mean searching? In my opinion, the new version is more consistent with the rest of the tables.

@kmalyjur Yes, I meant search, anyway I didn't find way how to search for "Fix type"

@kmalyjur kmalyjur marked this pull request as draft May 4, 2026 08:33
@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch 4 times, most recently from f93e38d to d5da017 Compare May 5, 2026 13:01
@kmalyjur kmalyjur marked this pull request as ready for review May 5, 2026 13:02
@kmalyjur kmalyjur requested review from Lukshio and adamruzicka May 5, 2026 13:02
@kmalyjur

kmalyjur commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@Lukshio @adamruzicka it's ready for a re-review now. (sorry for all the pushes, I had broken rubycop)

@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch from d5da017 to c22140f Compare May 7, 2026 10:48
@MariaAga

MariaAga commented May 7, 2026

Copy link
Copy Markdown
Member

when changing pages, the toolbar items disappear for a second
image

@Lukshio Lukshio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing filters causes buttons to disapear.
Image

Expadning some rows with long title causes columns to change its width
Image

I would prefer to have the column names in one row as with expanded items.
Image

Comment thread app/controllers/preupgrade_report_entries_controller.rb Outdated
Comment thread webpack/components/PreupgradeReportsTable/PreupgradeReportsTableHelpers.js Outdated
@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch from c22140f to 4ce7bf4 Compare May 7, 2026 17:39
@kmalyjur

kmalyjur commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

I fixed all the issues except for this one:

I would prefer to have the column names in one row as with expanded items.

@Lukshio I didn't see it before, is it okay now?

@kmalyjur kmalyjur requested a review from Lukshio May 7, 2026 17:41
@kmalyjur

Copy link
Copy Markdown
Contributor Author

Added a custom layout for the preupgrade entries index endpoint to include fixable_count in the response. This
allows the frontend to show the correct count of fixable entries (those with fix_type = command) in the bulk
selection dropdown without affecting other API responses.

@kmalyjur kmalyjur force-pushed the job-inv-leapp-filter branch from e45ccd2 to 4c7cf07 Compare June 25, 2026 09:50
@kmalyjur

Copy link
Copy Markdown
Contributor Author

Snaps has to be replaced by RTL (PreupgradeReports/components/snapshots/NoReports.test.js.snap)

I replaced this one test, however, I don't think it's necessary to replace the rest (13) of them, because they are going to be deleted when deleting the legacy leapp table. @Lukshio what do you think?

Rebased.

@Lukshio

Lukshio commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

I replaced this one test, however, I don't think it's necessary to replace the rest (13) of them, because they are going to be deleted when deleting the legacy leapp table. @Lukshio what do you think?

Sure, leave it as it is.

@adamruzicka adamruzicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of the custom layout. Normally I'd argue that those things will eventually deviate from the original they're supposed to be extending, but this one already does. Additionally I'd be worried that if the original changes, noone will remember to update this.

If this is strictly about showing the count when you select all, could that count be retrieved on demand? Once the user clicks that checkbox, it would make a call to the index endpoint and explicitly ask for fixable issues (maybe with per_page=0 since we just need the count)?

protected

def model_of_controller
resource_scope

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Judging by the method name, shouldn't this be "just" PreupgradeReportEntry?

@kmalyjur

kmalyjur commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

It is now fetching the accurate count of fixable entries only when the user clicks "Select All". The commit 71e7469 using a custom layout was reverted.
This PR is now dependent on theforeman/foreman#11060.

@Lukshio

Lukshio commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@kmalyjur Can you please provide screenshots/video of the issue with 0 select? I am probably not seeing/understanding the issue correctly

@kmalyjur

kmalyjur commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@Lukshio Without the foreman core commit, the fixableCount is initially null, SelectAllCheckbox would use its default value of 0. This makes the "Select all" dropdown option disabled (isDisabled={totalCount === 0 || areAllRowsSelected}) even though fixable items exist - it just hasn't fetched the count yet.

Screen.Recording.2026-07-01.at.15.22.40.mov

Comment thread webpack/components/PreupgradeReportsTable/index.js Outdated
() => rawSelectedIds.filter(id => validFixableIds.includes(id)),
// eslint-disable-next-line react-hooks/exhaustive-deps
[rawSelectedIds.join(','), validFixableIds]
const selectAll = useCallback(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its causing number glitching between total and subtotal.

Screencast_20260701_190714.mp4

Comment thread webpack/components/PreupgradeReportsTable/PreupgradeReportsTableHelpers.js Outdated

@Lukshio Lukshio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Core PR merged, changes looks good, packit tested

@Lukshio Lukshio merged commit 146d843 into theforeman:master Jul 3, 2026
16 checks passed
@Lukshio

Lukshio commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Thanks @kmalyjur @adamruzicka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaks-robottelo Can be set via comment: /label breaks-robottelo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants