Fixes #38891 - Refactor @subtotal assignment for API hosts action#1015
Fixes #38891 - Refactor @subtotal assignment for API hosts action#1015adamruzicka merged 1 commit intotheforeman:masterfrom
Conversation
ofedoren
force-pushed
the
bug-38891-sizes-typo
branch
from
03131c5 to
e882bdb
Compare
| @hosts = @hosts.select { |host| @host_statuses[host.id] == 'N/A' } if params[:awaiting] | ||
| @subtotal = @hosts.size |
There was a problem hiding this comment.
Sadly this breaks pagination
> ::Host.search_for('name ~ test-00*').paginate(page: 1, per_page: 5).size
5
> ::Host.search_for('name ~ test-00*').paginate(page: 1, per_page: 5).total_entries
10
There was a problem hiding this comment.
That example makes me think that the variable names are... misleading?
#size is more logical for @subtotal whereas #total_entries is more logical for @total...
There was a problem hiding this comment.
That example makes me think that the variable names are... misleading?
Pretty much, yes. Iirc the foreman logic for this is that total is the total number of the records without searching and subtotal is the total number of records matched by the search without pagination.
There was a problem hiding this comment.
That example makes me think that the variable names are... misleading?
Pretty much, yes. Iirc the foreman logic for this is that
totalis the total number of the records without searching andsubtotalis the total number of records matched by the search without pagination.
Yes, subtotal is sum of filtered results
ofedoren
force-pushed
the
bug-38891-sizes-typo
branch
from
e882bdb to
4d17572
Compare
ofedoren
force-pushed
the
bug-38891-sizes-typo
branch
from
4d17572 to
e5070f4
Compare
| def hosts | ||
| set_hosts_and_template_invocations | ||
| set_statuses_and_smart_proxies | ||
| @total = @job_invocation.targeting.hosts.size |
There was a problem hiding this comment.
This is removed since it's essentially this: https://github.com/theforeman/foreman_remote_execution/blob/master/app/controllers/api/v2/job_invocations_controller.rb#L304, although not sure 'cause of .where later when params[:search] is present.
| @subtotal = @hosts.respond_to?(:total_entries) ? @hosts.total_entries : @hosts.sizes | ||
| end | ||
| @hosts = @hosts.select { |host| @host_statuses[host.id] == 'N/A' } if params[:awaiting] | ||
| @subtotal = @hosts.try(:count).to_i |
There was a problem hiding this comment.
This is just manually doing the same https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/base_controller.rb#L81 does.
|
Thanks, @adamruzicka, updated. I thought about making it use https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/base_controller.rb#L75-L85, so we're compliant with the pattern, but it would require even nastier changes :/ |
ofedoren
force-pushed
the
bug-38891-sizes-typo
branch
from
e5070f4 to
96fbedc
Compare
|
Thanks, @adamruzicka, updated. Welp, I've never thought I'll open a can of worms by this patch :/ The page sometimes feels even more bugged. Before this patch: |
ofedoren
force-pushed
the
bug-38891-sizes-typo
branch
from
96fbedc to
38a2241
Compare
3 participants
I think there was a typo in
sizemethod, but looking at the condition and used methods, I think it can be unified a bit.@Lukshio, could you please check if I didn't messed up your patch?