Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes a bug attaching beacon URL parameters #37

Merged
merged 3 commits into from Apr 16, 2012

Conversation

Projects
None yet
3 participants
Contributor

edgarklein commented Mar 23, 2012

When attaching beacon URL parameters boomerang always starts attaching the parameters with "?". This is incorrect if the beacon URL already contains URL parameters. This fix checks whether the URL contains a "?" and if it does, it will attach the parameters prefixed with "&".

Contributor

bluesmoon commented Mar 29, 2012

agreed, this should be merged in.

@bluesmoon bluesmoon commented on the diff Mar 30, 2012

boomerang.js
@@ -373,8 +373,15 @@ boomr = {
return this;
}
+ // if there are already url parameters in the beacon url,
+ // change the first parameter prefix for the boomerang url parameters to &
+ var paramPrefix = '?';
@bluesmoon

bluesmoon Mar 30, 2012

Contributor

Based on boomerang's coding convention, variables should be declared at the start of the function

@bluesmoon bluesmoon commented on the diff Mar 30, 2012

boomerang.js
@@ -373,8 +373,15 @@ boomr = {
return this;
}
+ // if there are already url parameters in the beacon url,
+ // change the first parameter prefix for the boomerang url parameters to &
+ var paramPrefix = '?';
+ if(impl.beacon_url.indexOf('?') > -1)
@bluesmoon

bluesmoon Mar 30, 2012

Contributor

Also, preferred braces around all block statements.

Contributor

bluesmoon commented Mar 30, 2012

merged into bluesmoon/master with a few mods, and have requested an upstream pull.

Contributor

edgarklein commented Mar 30, 2012

I hope you merged all 3 pull requests because they are all related.

Contributor

bluesmoon commented Mar 30, 2012

yeah, that's the way git rolls ;)

@marcelduran marcelduran merged commit 8dc71c5 into yahoo:master Apr 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment