Merge lp:~camptocamp/openerp-web/6.1-lp1055349-url-widget into lp:openerp-web/6.1

Proposed by Guewen Baconnier @ Camptocamp
Status: Needs review
Proposed branch: lp:~camptocamp/openerp-web/6.1-lp1055349-url-widget
Merge into: lp:openerp-web/6.1
Diff against target: 17 lines (+6/-1)
1 file modified
addons/web/static/src/js/view_form.js (+6/-1)
To merge this branch: bzr merge lp:~camptocamp/openerp-web/6.1-lp1055349-url-widget
Reviewer Review Type Date Requested Status
Xavier (Open ERP) (community) Approve
Review via email: mp+125940@code.launchpad.net

Description of the change

To post a comment you must log in.
2461. By Guewen Baconnier @ Camptocamp <email address hidden> on 2012-09-24

[FIX] need a coffee.

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

Although I believe the code could use RegExp#test(String) (https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/RegExp/test) rather than RegExp#exec(String), as the result matches are not used, the match is simply used as a boolean.

#test makes purpose clearer, and may be slightly more efficient.

And I don't know if you're shooting for full correctness (this is already an improvement over the current state of affairs) but there are a few potentjial issues:

* In the current code, with its existing semantics, everything after the `:` is redundant, the matchgroup isn't needed and it doesn't filter out anything more than not putting it at all.
* Because the match is not anchored to the start of the string (by `^`) and the colon can be in a fragment, the url `foo.com/#a:3` would pass the test though it should not
* The prepending will fail if the URL is of the form `//foo.com` (same scheme, different domain) (note: this means the match should probably remain a match)

review: Approve

Unmerged revisions

2461. By Guewen Baconnier @ Camptocamp <email address hidden> on 2012-09-24

[FIX] need a coffee.

2460. By Guewen Baconnier @ Camptocamp <email address hidden> on 2012-09-24

[FIX] prepend http:// before the url when we open it from the form url widget

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'addons/web/static/src/js/view_form.js'
2--- addons/web/static/src/js/view_form.js 2012-09-03 13:17:44 +0000
3+++ addons/web/static/src/js/view_form.js 2012-09-24 08:33:19 +0000
4@@ -1582,7 +1582,12 @@
5 if (!this.value) {
6 this.do_warn(_t("Resource error"), _t("This resource is empty"));
7 } else {
8- window.open(this.value);
9+ var value = this.value;
10+ var s = /(\w+):(.+)/.exec(value);
11+ if (!s) {
12+ value = "http://" + value;
13+ }
14+ window.open(value);
15 }
16 }
17 });