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

Fixes #37587 - cast page and perPage to Number #10301

Closed
wants to merge 1 commit into from

Conversation

jeremylenz
Copy link
Contributor

New All Hosts page - On the bulk packages wizard, you get this error when you choose anything except 'Upgrade all' and then advance to the next wizard step (the packages table):

Warning: Failed prop type: Invalid prop `perPage` of type `string` supplied to `Pagination`, expected `number`.
    in Pagination (created by TableIndexPage)
    in TableIndexPage (created by BulkPackagesTable)
    in BulkPackagesTable (created by BulkPackagesUpgradeTable)

page and perPage being strings also cause the Pagination component to do crazy things like go from page 1 > 2 > 21 when you press the next page button.

This casts them to Numbers in TableIndexPage. I also added some .to_is in the backend - I'm not sure they made a difference, but think they should stay so that things are more consistent.

@github-actions github-actions bot added the UI label Aug 29, 2024
@MariaAga MariaAga self-assigned this Aug 30, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

My biggest issue with Ruby's .to_i is that it silently ignores anything that doesn't fit. For example, 'asd'.to_i becomes 0. That can lead to per_page being 0 which IMHO is never valid.

I'd like to see some tests for this and a reproducer.

@@ -102,8 +102,8 @@ def metadata_page
end

def metadata_per_page
@per_page ||= Setting[:entries_per_page] if params[:per_page].blank?
@per_page ||= params[:per_page] == 'all' ? metadata_total : params[:per_page].to_i
@per_page ||= Setting[:entries_per_page].to_i if params[:per_page].blank?
Copy link
Member

Choose a reason for hiding this comment

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

Why is this setting ever not a number? From the model I'd expect it to always be an integer:

setting('entries_per_page',
type: :integer,
description: N_("Number of records shown per page in Foreman"),
default: 20,
full_name: N_('Entries per page'))

We already know that if you modify the default to a string via the API it becomes an integer:

test "should parse string values to integers" do
put :update, params: { :id => 'entries_per_page', :setting => { :value => "100" } }
assert_response :success
assert_equal 100, Setting['entries_per_page']
end

If it's not, something is wrong and we need to figure out why. IMHO we shouldn't do this.

@per_page ||= Setting[:entries_per_page] if params[:per_page].blank?
@per_page ||= params[:per_page] == 'all' ? metadata_total : params[:per_page].to_i
@per_page ||= Setting[:entries_per_page].to_i if params[:per_page].blank?
@per_page ||= params[:per_page] == 'all' ? metadata_total.to_i : params[:per_page].to_i
Copy link
Member

Choose a reason for hiding this comment

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

Same here: if this isn't a number then we also print the total as a non-number in our JSON which is wrong.

@jeremylenz
Copy link
Contributor Author

I have a hunch the problem is in https://github.com/theforeman/foreman/blob/develop/app/views/api/v2/layouts/index_layout.json.erb

but I didn't want to touch that, because I'm sure there are .. reasons that's the way it is 🤔

@jeremylenz
Copy link
Contributor Author

all I know is it always comes back from the API as a string.

@ekohl
Copy link
Member

ekohl commented Aug 30, 2024

Could you come up with an integration test that reproduces it? Or at least the API request & response?

@jeremylenz
Copy link
Contributor Author

Ok, I've looked into it some more and found something interesting. The page and per_page always come back as strings, but only (a) when you've sent them as URL query params, and (b) only from Katello endpoints. This led me to Katello/katello#11124, which seems to fix the issue without any Foreman changes (I tested on develop). @ekohl Let me know if you still want to keep any changes here; otherwise I think we can close this in favor of the Katello one.

@ekohl
Copy link
Member

ekohl commented Aug 30, 2024

I'd prefer to keep it simple and avoid redundant conversions.

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

Successfully merging this pull request may close these issues.

3 participants