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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/controllers/api/v2/base_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

foreman/app/registries/foreman/settings/general.rb

Lines 18 to 22 in 2ea2c46

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:

foreman/test/controllers/api/v2/settings_controller_test.rb

Lines 92 to 96 in 2ea2c46

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 ||= 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.

end

# For the purpose of ADDING/REMOVING associations in CHILD node on POST/PUT payload
Expand Down
6 changes: 4 additions & 2 deletions webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/TableIndexPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,16 @@ const TableIndexPage = ({
can_create: canCreate,
results,
total,
per_page: perPage,
page,
per_page: respPerPage,
page: respPage,
subtotal,
message: errorMessage,
},
status = STATUS.PENDING,
setAPIOptions,
} = response;
const page = Number(respPage);
const perPage = Number(respPerPage);

const memoDefaultSearchProps = useMemo(
() => getControllerSearchProps(controller),
Expand Down
Loading