Skip to content
This repository has been archived by the owner. It is now read-only.

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

Closed
icinga-migration opened this issue May 30, 2013 · 10 comments
Labels
Milestone

Comments

@icinga-migration
Copy link
Member

@icinga-migration icinga-migration commented May 30, 2013

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

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 1, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 1, 2013

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

  • Status changed from New to Feedback
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 1, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 1, 2013

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

well me neither. thanks for finding and fixing.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 3, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 3, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 3, 2013

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

  • File deleted page_refresh.patch
@icinga-migration

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 3, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Jun 3, 2013

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

This comment has been minimized.

Copy link
Member Author

@icinga-migration icinga-migration commented Dec 8, 2014

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
1 participant
You can’t perform that action at this time.