Issue 29332588: Issue 3403 - Add web redirect for the uninstallation page (Closed)
|
Created:
Dec. 13, 2015, 5:22 p.m. by kzar Modified:
Dec. 15, 2015, 11:50 a.m. Reviewers:
Felix Dahlke CC:
mathias, Sebastian Noack Visibility:
Public. |
DescriptionIssue 3403 - Add web redirect for the uninstallation page
Patch Set 1 #
Total comments: 6
Patch Set 2 : Renamed uninstall-abp uninstalled, added comment #
Total comments: 5
MessagesTotal messages: 10
Patch Set 1 (This is the last blocker for getting the uninstallation page to be displayed in Chrome / Opera. Hoping we can get this pushed+provisioned before Wednesday as it will be another roadmap item completed for the quarter.) https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: rewrite ^ /$lang/$link$anchor redirect; This is required because we want to preserve query parameters and therefore can't have the trailing `?`.
https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:174: if ($arg_link ~ "uninstalled") Since you're not matching a regex, you can just use `=`. https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: rewrite ^ /$lang/$link$anchor redirect; On 2015/12/13 17:25:40, kzar wrote: > This is required because we want to preserve query parameters and therefore > can't have the trailing `?`. So you mean, this: https://adblockplus.org/redirect?link=uninstalled&foo=bar would redirect to: https://adblockplus.org/en/uninstall-abp?link=uninstalled&foo=bar ? Assuming this works, IMO it deserves a comment. Also note that we won't need $anchor here.
Patch Set 2 : Renamed uninstall-abp uninstalled, added comment https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:174: if ($arg_link ~ "uninstalled") On 2015/12/14 10:23:26, Felix Dahlke wrote: > Since you're not matching a regex, you can just use `=`. Done. https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: rewrite ^ /$lang/$link$anchor redirect; On 2015/12/14 10:23:26, Felix Dahlke wrote: > On 2015/12/13 17:25:40, kzar wrote: > > This is required because we want to preserve query parameters and therefore > > can't have the trailing `?`. > > So you mean, this: > > https://adblockplus.org/redirect?link=uninstalled&foo=bar > > would redirect to: > > https://adblockplus.org/en/uninstall-abp?link=uninstalled&foo=bar > > ? > > Assuming this works, IMO it deserves a comment. Also note that we won't need > $anchor here. Yep, although we've just agreed in IRC to rename uninstall-abp to uninstalled so I've updated it accordingly. (It does work, I've tested it each time.)
LGTM. Note that this is infrastructure, I've moved Sebastian to CC and added Matze. https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332589/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: rewrite ^ /$lang/$link$anchor redirect; On 2015/12/14 10:45:49, kzar wrote: > On 2015/12/14 10:23:26, Felix Dahlke wrote: > > On 2015/12/13 17:25:40, kzar wrote: > > > This is required because we want to preserve query parameters and therefore > > > can't have the trailing `?`. > > > > So you mean, this: > > > > https://adblockplus.org/redirect?link=uninstalled&foo=bar > > > > would redirect to: > > > > https://adblockplus.org/en/uninstall-abp?link=uninstalled&foo=bar > > > > ? > > > > Assuming this works, IMO it deserves a comment. Also note that we won't need > > $anchor here. > > Yep, although we've just agreed in IRC to rename uninstall-abp to uninstalled so > I've updated it accordingly. (It does work, I've tested it each time.) Awesome, I was about to suggest that, but didn't bother since the page was already live :P
https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation page we need to preserve the query parameters We probably should strip the "link" parameter. Any way to do that?
https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation page we need to preserve the query parameters On 2015/12/14 12:38:37, Sebastian Noack wrote: > We probably should strip the "link" parameter. Any way to do that? Sure it's redundant but it doesn't harm. I couldn't see an easy way to strip it but if you have a suggestion I'll give it a go.
https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation page we need to preserve the query parameters On 2015/12/14 12:38:37, Sebastian Noack wrote: > We probably should strip the "link" parameter. Any way to do that? I thought about this as well, but AFAIK there's no really simple way to do it. I see two options: 1. We capture all args after "link" in the rewrite, then add `?` to the target again and append the captured args after that. It's not gonna work if link is not the first parameter, but that should be fine. 2. We add the `?` back and list all the args we want explicitly using the `$arg_` variables. I'm not sure it's worth the trouble though.
https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation page we need to preserve the query parameters On 2015/12/14 12:49:10, Felix Dahlke wrote: > On 2015/12/14 12:38:37, Sebastian Noack wrote: > > We probably should strip the "link" parameter. Any way to do that? > > I thought about this as well, but AFAIK there's no really simple way to do it. I > see two options: > > 1. We capture all args after "link" in the rewrite, then add `?` to the target > again and append the captured args after that. It's not gonna work if link is > not the first parameter, but that should be fine. > > 2. We add the `?` back and list all the args we want explicitly using the > `$arg_` variables. > > I'm not sure it's worth the trouble though. My vote is to leave it as-is, it's simple and it works. It would be a shame to miss getting the uninstall page launched this quater because of a single redundant parameter.
https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... File modules/web/templates/adblockplus.org.conf.erb (right): https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation page we need to preserve the query parameters On 2015/12/14 12:59:12, kzar wrote: > On 2015/12/14 12:49:10, Felix Dahlke wrote: > > On 2015/12/14 12:38:37, Sebastian Noack wrote: > > > We probably should strip the "link" parameter. Any way to do that? > > > > I thought about this as well, but AFAIK there's no really simple way to do it. > I > > see two options: > > > > 1. We capture all args after "link" in the rewrite, then add `?` to the target > > again and append the captured args after that. It's not gonna work if link is > > not the first parameter, but that should be fine. > > > > 2. We add the `?` back and list all the args we want explicitly using the > > `$arg_` variables. > > > > I'm not sure it's worth the trouble though. > > My vote is to leave it as-is, it's simple and it works. It would be a shame to > miss getting the uninstall page launched this quater because of a single > redundant parameter. Well, this certainly isn't something that would delay the launch. We wouldn't have a release before Jan 5 anyway, there are still some issues with the uninstallation page itself that needs to be fixed first, and the possible solutions here aren't to complicated after all. But yeah, they aren't too great either, but IMO also not worse than not stripping the link parameter.
On 2015/12/14 13:17:26, Sebastian Noack wrote: > https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... > File modules/web/templates/adblockplus.org.conf.erb (right): > > https://codereview.adblockplus.org/29332588/diff/29332593/modules/web/templat... > modules/web/templates/adblockplus.org.conf.erb:249: # For the uninstallation > page we need to preserve the query parameters > On 2015/12/14 12:59:12, kzar wrote: > > On 2015/12/14 12:49:10, Felix Dahlke wrote: > > > On 2015/12/14 12:38:37, Sebastian Noack wrote: > > > > We probably should strip the "link" parameter. Any way to do that? > > > > > > I thought about this as well, but AFAIK there's no really simple way to do > it. > > I > > > see two options: > > > > > > 1. We capture all args after "link" in the rewrite, then add `?` to the > target > > > again and append the captured args after that. It's not gonna work if link > is > > > not the first parameter, but that should be fine. > > > > > > 2. We add the `?` back and list all the args we want explicitly using the > > > `$arg_` variables. > > > > > > I'm not sure it's worth the trouble though. > > > > My vote is to leave it as-is, it's simple and it works. It would be a shame to > > miss getting the uninstall page launched this quater because of a single > > redundant parameter. > > Well, this certainly isn't something that would delay the launch. We wouldn't > have a release before Jan 5 anyway, there are still some issues with the > uninstallation page itself that needs to be fixed first, and the possible > solutions here aren't to complicated after all. But yeah, they aren't too great > either, but IMO also not worse than not stripping the link parameter. I don't have a strong opinion, but I'd prefer to land this as it is and to file a follow-up issue for removing the link parameter from the URL. Once Matze is back, he might have a better idea than those I had. 
|
