Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Canonical and None mapping redirects #3396

Merged

Conversation

daguiler
Copy link
Contributor

@daguiler daguiler commented Dec 10, 2019

Fixes #3395
Fixes #3017
Fixes #2841

Summary

This pull request fixes wrong redirections that occur on Canonical and None URL mappings.
I have tested this as much as I could. I created following spreadsheet to track my tests and make sure this change does not introduce new regressions:
https://docs.google.com/spreadsheets/d/1c9DyWfLAG5qrAnPDZsvAg5VBJkSRWBciWtPxHARxWUA/edit?usp=sharing
Above spreadsheet contains:

Tab Description
DNN943 A set of test URLs along with the expected and the actual responses when executed on DNN 9.4.3. Red colors on the right show cases where the response is not correct, as per my understanding. In such cases the Severity column shows 1 (minor issue) or 3 (big issue).
Patched Same set of test URLs but with responses after applying this fix. Green colors in Severity column show how each test case improved with respect to original.
Bindings IIS bindings I used in the test environment
Portals How I set up the portals in the test environment. Main portal is localized, child portal is not localized
Tabs Which pages I created in the test environment
Aliases How aliases were configured.

I used following Powershell script to automate requesting each test URL:

$domain = "platform-9-4-3.dnndev.me"
@(
	"en-$domain"
	"en.$domain"
	"fr-$domain"
	"fr.$domain"
	"$domain"
	"$domain/en-us"
	"$domain/fr-fr"
	"www-$domain"
	"www-$domain/en-us"
	"www-$domain/fr-fr"
	"www.$domain"
	"www.$domain/en-us"
	"www.$domain/fr-fr"
    "$domain/child"
    "www.$domain/child"
    "www-$domain/child"
) | % { @( $_, "$_/" ) | % {
    $response = Invoke-WebRequest "http://$_" -MaximumRedirection 0 -ErrorAction Ignore -UseBasicParsing
    $output = $_, $response.StatusCode
    If ($response.StatusCode -eq 301) {
        $output += $response.Headers['Location'], $response.Headers['X-Redirect-Reason']
    }
    $output -join "`t"
}}

This PR is currently targetting 9.4.4 instead of develop, but I know it needs to be re-targeted. I did this on purpose to test if it's easier to re-target from 9.4.4 than from develop.

@valadas
Copy link
Contributor

valadas commented Dec 10, 2019

If I am not mistaken, this would Fix #3395, #3017 and #2841 Correct?

@valadas valadas changed the base branch from release/9.4.4 to develop December 10, 2019 00:56
@valadas valadas changed the base branch from develop to release/9.4.4 December 10, 2019 00:57
@daguiler
Copy link
Contributor Author

I'm not sure. Maybe we can add that specific scenario to the test sheet and check it out?

@valadas
Copy link
Contributor

valadas commented Dec 10, 2019

Yeah, let me know if you test it before me, the changes look good to me but I did not review yet because I would like to make sure how many issues this closes :) If you have time before me to test those scenarios, please report back...

@daguiler
Copy link
Contributor Author

If I'm reading 2841 case correctly, it looks pretty much like my child site case, where there are several competing aliases, none of them having a culture code. The key difference is that in my case I have the primary alias in the first record of the PortalAlias table, whereas in 2841 the problem begun when the second row was selected as primary. So I changed the primary alias to be the one in the second row (www-* in my case):
dnn-2841-alias
I also set the mapping mode to Redirect (I think that was also the case in 2841)
With all this in place, I was not able to reproduce the issue. Every time I requested any of the 3 variants, I got correctly redirected to the selected primary.
Maybe I'm not testing 2841 correctly.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this looks awesome to me and should close 3 issues :) Awesome job!

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome, and checking your mappings and using your script this appears to be correct.

However, this should be targeting /develop

@valadas valadas changed the base branch from release/9.4.4 to develop December 10, 2019 05:24
@valadas valadas changed the base branch from develop to future/10 December 10, 2019 05:24
@valadas valadas changed the base branch from future/10 to develop December 10, 2019 05:24
@valadas
Copy link
Contributor

valadas commented Dec 10, 2019

Yeah, it was a test also to retarget, it worked without any conflict. Just need to let it rebuild and we are good to merge...

@valadas valadas added this to the 9.5.0 milestone Dec 10, 2019
@valadas valadas merged commit 46c17ff into dnnsoftware:develop Dec 10, 2019
@daguiler daguiler deleted the fix-canonical-mapping-redirect branch December 10, 2019 08:56
@daguiler
Copy link
Contributor Author

Thanks guys.
About 2841, there was a misunderstanding. My comment above should have been more precise. What I tried to say is that I was not able to reproduce 2841, even before applying this fix. So we can't really say this fixes 2841.

@daguiler
Copy link
Contributor Author

Maybe @hismightiness can chime in?

@WillStrohl
Copy link
Contributor

#2841 only occurs under very specific conditions and it's not consistently reproducible, as noted in the Issue.

@valadas
Copy link
Contributor

valadas commented Dec 10, 2019

Ok, do we leave #2841 closed until someone reports it back with maybe some steps to make it consistently happen?

@WillStrohl
Copy link
Contributor

Ok, do we leave #2841 closed until someone reports it back with maybe some steps to make it consistently happen?

I'd vote yes. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants