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

UI Tasks view: fix per page settings #1181

Closed
wants to merge 1 commit into from
Closed

UI Tasks view: fix per page settings #1181

wants to merge 1 commit into from

Conversation

mzazrivec
Copy link
Contributor

Configuration -> Tasks -> per page settings and page navigation
wouldn't work correctly:

  • the view would always default to 50 items per page
  • incorrect numbers shown next to the navigation icons

Fixes #818

https://bugzilla.redhat.com/show_bug.cgi?id=1171150

@@ -23,7 +23,7 @@ def index
def change_tab
@tabform = "tasks_" + params[:tab]
jobs
render :action=>"jobs"
render :action => "jobs" unless request.xml_http_request?
Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec why do we need unless check here, don't we only call change_tab when tab is changed in UI, i don't see any other callers of this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also call change_tab() when navigating through the tasks list (those next, previous, first, last buttons).

Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec (next,previous..) buttons should not send up action as change_tab, they should send up a transaction to "index" method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec ignore my previous comment, discussing with @AparnaKarve, @dclarizio to see if this should be the correct behavior for paging controls on the screen with multiple tabs and list views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paging controls -- as far as I can tell -- are being all rendered from
layouts/_pagingcontrols.html from which we call update_paging_url_parms()
to construct the navigation urls.

update_paging_url_parms() simply takes the relate path from request:

"#{request.path_info}?#{updated_query_string.to_query}"

So what's being called when you hit the navigation buttons depends on the request
that rendered the list view that you're clicking on.

If you initially land on the 'Tasks' page, you're being presented with the 'My VM Analysis Tasks'
tab -- request is by /miq_proxy/index. Paging requests then go to /miq_proxy/index.

If you change the tab to something else ('All Other Tasks' for example), the request changes
to /miq_proxy/change_tab and all subsequent paging request then go to change_tab.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec your analysis above is correct, the question here is should we leave it as it is currently working or do we need to figure out a way to change the path so paging controls send up a transaction to index method. @AparnaKarve @dclarizio thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec I think a fix might be needed in update_paging_url_parms.
There are essentially two parts to this method - the path and the url params.
The path is what needs to be addressed. Let me get back to you on what could be done to fix the path.

@Fryguy Fryguy added the bug label Dec 8, 2014
@h-kataria
Copy link
Contributor

@mzazrivec when i go to Tasks screen first time, i selected 5 on Per Page pull down nothing happens. It starts to work after i change tab once.

@mzazrivec
Copy link
Contributor Author

@h-kataria This is because in this particular case, the per page pull down makes a post request
to /miq_proxy/index -> routing error.

This is fixable in the same way I fixed it for /miq_proxy/change_tab: add the route and
modify the index method -- render unless request.xml_http_request? to avoid double
render error.

Configuration -> Tasks -> per page settings and page navigation
wouldn't work correctly:
* the view would always default to 50 items per page
* incorrect numbers shown next to the navigation icons

Fixes #818

https://bugzilla.redhat.com/show_bug.cgi?id=1171150
@miq-bot
Copy link
Member

miq-bot commented Dec 10, 2014

Checked commit mzazrivec@7cd0e66 with rubocop 0.27.1
3 files checked, 0 offenses detected
Everything looks good. 🍪

@dclarizio
Copy link

Closed per @mzazrivec comment in #1231.

@dclarizio dclarizio closed this Dec 17, 2014
@mzazrivec mzazrivec deleted the fix_task_paging branch January 9, 2015 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure -> Tasks "Per Page" pulldown doesn't do anything
6 participants