[dev.icinga.com #4233] URL construction issue in icinga_reload_scroll_position() javascript

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/4233

Created by ksuehring on 2013-05-30 09:38:55 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2013-06-03 20:43:42 +00:00)
Target Version: 1.9.2
Last Update: 2014-12-08 09:15:27 +00:00 (in Redmine)

Icinga Version: 1.9.1
OS Version: Ubuntu 12.04.2 LTS

---

in html/js/page_refresh.js the function icinga_reload_scroll_position() tries to replace the "scroll=" part of the URL. The new scroll parameter is always added to the end of the URL. So the "?" operator might be moved behind other parameters.

The attached patch should solve the problem by changing the regex to replace the scroll position in place.

Attachments

• page_refresh.patch2 ksuehring - 2013-06-01 13:52:52 +00:00

Changesets

2013-06-03 19:46:57 +00:00 by (unknown) 68b6717

classic ui: fix URL construction issue in icinga_reload_scroll_position() javascript

fixes #4233

2013-06-17 20:00:42 +00:00 by ricardo 37bb277

classic-ui: javascript cleanup and common.lib added #4324

* common functions got moved to common_functions.js
* showlog and notifications got also "blind" effect for time picker
* fixes bug 4233 "scroll" as well.

fixes: #4324
fixes: #4233

[icinga-migration]

Updated by ricardo on 2013-06-01 12:59:50 +00:00

• Priority changed from High to Normal

Hi,

but what if there isn't a scroll option. Then it doesn't get set to a new value.

[icinga-migration]

Updated by ricardo on 2013-06-01 13:00:03 +00:00

• Status changed from New to Feedback

[icinga-migration]

Updated by ksuehring on 2013-06-01 13:53:17 +00:00

• File added page_refresh.patch2

Good point. I'm attaching a patch that resolves that. Could probably be done more elegant, but I'm not a Javascript wizard ;-)

[icinga-migration]

Updated by mfriedrich on 2013-06-01 18:36:55 +00:00

well me neither. thanks for finding and fixing.

[icinga-migration]

Updated by mfriedrich on 2013-06-03 19:20:52 +00:00

i've assembled the 2 patches into a git patch - does not work for me.

/icinga/cgi-bin/status.cgi?host=all&type=detail&servicestatustypes=29

does not get any 'scroll' item initially added to the url string.

diff --git a/html/js/page_refresh.js b/html/js/page_refresh.js
index e0243d4..e12d0a3 100644
--- a/html/js/page_refresh.js
+++ b/html/js/page_refresh.js
@@ -48,11 +48,14 @@ function icinga_reload_scroll_position() {
        else {
                /* remove previous scroll leftovers */
                url = window.location.href;
-               url_c = url.replace(/[\?&]scroll=(\d+)/,'');
+               url_c = url.replace(/([\?&])scroll=(\d+)/,'$1scroll=' + scroll_pos);
+
+               if (url_c.indexOf('?') === -1) {
+                       url_c = urlc.replace(/&/,'?');
+               }

                /* create new querystring and reload */
-               q = url_c.indexOf('?') === -1 ? '?' : '&';
-               window.location.href = url_c + q + 'scroll=' + scroll_pos;
+               window.location.href = url_c;
        }
 }

the issue is - the original code intended to remove all occurences of 'scroll' within the string, pushing a new entry after cleaning. the new code does not take that into account, and tries to replace existing 'scroll' occurences, but fails when there is none initially.

so the patch requires rework, or the issue can be solved differently.

[icinga-migration]

Updated by ksuehring on 2013-06-03 19:25:43 +00:00

You can ignore the first patch. The one line of the second patch alone fixes the problem.

After the scroll position is removed in the original code, it will just replace the first "&" with a "?" (if any) and the original code will add the scroll position to the end with the proper "&" or "?".

[icinga-migration]

Updated by mfriedrich on 2013-06-03 19:42:27 +00:00

• File deleted page_refresh.patch

[icinga-migration]

Updated by mfriedrich on 2013-06-03 19:48:12 +00:00

• Status changed from Feedback to Assigned
• Assigned to set to mfriedrich
• Target Version set to 1.9.2

ah ok, thanks for clarification. yes, that one works for me too. thanks for the fix, will be in 1.9.2 then.

[icinga-migration]

Updated by Anonymous on 2013-06-03 20:43:42 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 0 to 100

Applied in changeset icinga-core:68b6717b2a8b10012140edcb46d4b2515beb44e3.

[icinga-migration]

Updated by mfriedrich on 2014-12-08 09:15:27 +00:00

• Project changed from 19 to Core, Classic UI, IDOUtils
• Category changed from 52 to Classic UI
• OS Version set to Ubuntu 12.04.2 LTS