Feature/godfisk 586/redirect module handle url with slash in the end #21

grom8255 · 2018-05-11T09:52:57Z

fixes #13
The essence of the change is in the following tests:

CustomRedirect spec
  CustomRedirectCollection spec
    describe CustomRedirectCollection with exact match
      Given the redirect rule is http://mysite/test => http://mysite, exactMatch=True
        when url is 'http://mysite/test' then it's redirected to 'http://mysite' (29ms)
        (CHANGE) when url is 'http://mysite/test/' then it's redirected to 'http://mysite' (0ms)
        when url is 'http://mysite/testme' then it does not redirect (1ms)
        when url is 'http://mysite/testme' then it does not redirect (0ms)
        and includeQueryString=true
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite?query=my-query' (1ms)
        and includeQueryString=false
          when url is 'http://mysite/test' then it's redirected to 'http://mysite' (1ms)
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite' (0ms)
    describe CustomRedirectCollection exact match false and append match to new url false
      Given the redirect rule is http://mysite/test => http://mysite, exactNatch=False, appendMatchToNewUrl=False
        when url is 'http://mysite/test' then it's redirected to 'http://mysite' (0ms)
        (CHANGE) when url is 'http://mysite/test/' then it's redirected to 'http://mysite' (0ms)
        when url is 'http://mysite/testme' then it's redirected to 'http://mysite' (0ms)
        when url is 'http://mysite/test/me' then it's redirected to 'http://mysite' (0ms)
        and includeQueryString=true
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite?query=my-query' (0ms)
        and includeQueryString=false
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite' (0ms)
    describe CustomRedirectCollection exact match false and append match to new url true
      Given the redirect rule is http://mysite/test => http://mysite, exactNatch=False, appendMatchToNewUrl=True
        when url is 'http://mysite/test' then it's redirected to 'http://mysite' (0ms)
        (CHANGE)when url is 'http://mysite/test/' then it's redirected to 'http://mysite' (0ms)**
        when url is 'http://mysite/testme' then it's redirected to 'http://mysiteme' (0ms)
        when url is 'http://mysite/test/me' then it's redirected to 'http://mysite/me' (0ms)
        and includeQueryString=true
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite?query=my-query' (0ms)
          when url is 'http://mysite/testme?query=my-query' then it's redirected to 'http://mysiteme?query=my-query' (0ms)
        and includeQueryString=false
          when url is 'http://mysite/test?query=my-query' then it's redirected to 'http://mysite' (0ms)
          when url is 'http://mysite/testme?query=my-query' then it's redirected to 'http://mysiteme' (0ms)
    describe CustomRedirectCollection query string merge
      Given the redirect rule is http://mysite/test => http://mysite?a=1&b=2, exactMatch=True, appendMatchToNewUrl=True
        when url is 'http://mysite/test?a=0&x=0' then it's redirected to 'http://mysite?a=1&b=2&x=0' (0ms)
        when url is 'http://mysite/test/?x=0' then it's redirected to 'http://mysite?a=1&b=2&x=0' (0ms)
    (CHANGE) describe CustomRedirectCollection protocol invariant rule
      Given the redirect rule is //mysite/test => //mysite, exactMatch=True, appendMatchToNewUrl=True
        when url is 'http://mysite/test' then it's redirected to '//mysite' (0ms)
        when url is 'https://mysite/test' then it's redirected to '//mysite' (0ms)
      Given the redirect rule is //mysite/test => https://mysite, exactMatch=True, appendMatchToNewUrl=True
        when url is 'http://mysite/test' then it's redirected to 'https://mysite' (0ms)
        when url is 'https://mysite/test' then it's redirected to 'https://mysite' (0ms)
    describe CustomRedirectCollection wildcard append works with rule for relative url
      Given the redirect rule is /test => http://mysite
        when url is 'http://mysite/test' then it's redirected to 'http://mysite' (0ms)
        when url is 'http://mysite/testme' then it's redirected to 'http://mysiteme' (0ms)
        when url is 'http://mysite/test/me' then it's redirected to 'http://mysite/me' (0ms)
        when url is 'http://mysite/test/me?a=0&b=1' then it's redirected to 'http://mysite/me?a=0&b=1' (0ms)

…lash_in_the_end' into develop

VladimirLevchuk · 2018-05-16T13:10:54Z

src/Creuna.Episerver.RedirectHandler.TestApp/Creuna.Episerver.RedirectHandler.TestApp.csproj

@@ -462,9 +447,9 @@
        <WebProjectProperties>
          <UseIIS>True</UseIIS>
          <AutoAssignPort>True</AutoAssignPort>
-          <DevelopmentServerPort>0</DevelopmentServerPort>
+          <DevelopmentServerPort>51181</DevelopmentServerPort>


@Roman982 please move the settings outside the project (to .user file)

VladimirLevchuk · 2018-05-16T13:11:53Z

...p/modules/_protected/Creuna.Episerver.RedirectHandler/Core/CustomRedirects/CustomRedirect.cs

@@ -23,6 +25,14 @@ public CustomRedirect(string oldUrl, string newUrl, bool appendMatchToNewUrl, bo

        }

+        public CustomRedirect(CustomRedirect redirect)


state and notFoundErrorCount are missed in the copy ctor

Romanets · 2018-05-18T07:43:23Z

...p/modules/_protected/Creuna.Episerver.RedirectHandler/Core/CustomRedirects/CustomRedirect.cs

@@ -23,6 +25,16 @@ public CustomRedirect(string oldUrl, string newUrl, bool appendMatchToNewUrl, bo

        }

+        public CustomRedirect(CustomRedirect redirect)


It looks strange that "copy" stor has differnt logic with Copy method, I think it would be better to have two differrent methods with corresponding names instead.

Romanets · 2018-05-18T07:45:12Z

...p/modules/_protected/Creuna.Episerver.RedirectHandler/Core/CustomRedirects/CustomRedirect.cs

-        public string OldUrl { get; set; }
+        public string OldUrl
+        {
+            get => UrlStandardizer.Standardize(_oldUrl);


Looks strange that UrlStandardizer.Standardize method used for both get and set.

Romanets · 2018-05-18T08:05:54Z

src/Creuna.Episerver.RedirectHandler.UnitTests/CustomRedirectCollectionTests.cs

+            [TestCase("http://www.website.com/test/x1", "/sjomatskolen/x1")]
+            [TestCase("http://www.website.com/test/x2/", "/sjomatskolen/x2")]
+            [TestCase("http://www.website.com/test/x3/?a=b", "/sjomatskolen/x3?a=b")]
+            public void the_original_redirect_newurl_remains_unchanged(string url, string expectedRedirect)


Expected redirect is not used

Romanets · 2018-05-18T08:18:26Z

src/Creuna.Episerver.RedirectHandler.UnitTests/CustomRedirectCollection_spec.cs

+
+            context[$"Given the redirect rule is http://mysite/test => http://mysite, exactMatch={exactMatch}"] = () =>
+            {
+                before = () => redirects = new CustomRedirectCollection { new CustomRedirect("http://mysite/test", "http://mysite", appendMatchToNewUrl, exactMatch, true) };


It would be nice to have the last parameter name here

Romanets · 2018-05-18T08:25:17Z

src/Creuna.Episerver.RedirectHandler.UnitTests/CustomRedirectCollection_spec.cs

+
+            context[$"Given the redirect rule is http://mysite/test => http://mysite, exactNatch={exactMatch}, appendMatchToNewUrl={appendMatchToNewUrl}"] = () =>
+            {
+                before = () => redirects = new CustomRedirectCollection { new CustomRedirect("http://mysite/test", "http://mysite", appendMatchToNewUrl, exactMatch, true) };


I would be nice to test case when new ULR has / at the end (http://mysite/)

Roman982 added 24 commits May 8, 2018 11:53

make test app available

069d26b

update EPiServer.CMS to v 10.1.0

b42d105

update Cupdate EPiServer.CMS to v 10.1.0

28e490c

install nspec

f125dc4

install FluentAssertions

348ffdf

add tests from Vladimir's repository

bd19342

tests reviewed; a couple of bugs fixed

6a6a341

add UrlStandardizer

16bc0b1

apply changes in CustomRedirect

aae7353

configure Url Standardizer

6aa73c5

temporary changes - not finished

c03c6c3

fixed all Vladimir's test except 'protocol invariant rule'

a8464db

all Vladimir's test are fixed

70040f0

fix several Joakim's tests

34e2a2f

all tests are fixed

637c446

update version in nuspec

be9e06e

update nuspec, readme

aac0ac4

remove not used code; undo local changes in config

11b213f

update assembly version

141e1bc

update version

34e22cc

Merge branch 'feature/GODFISK-586/Redirect_module_-_handle_URL_with_s…

d82f0ca

…lash_in_the_end' into develop

added red tests

787d710

add fix

c9968a3

update versions

6e48338

VladimirLevchuk reviewed May 16, 2018

View reviewed changes

Roman982 added 2 commits May 17, 2018 10:35

move settings to .user file

ae46d1e

update CustomRedirect constructor - set missed properties

7703e5e

Romanets approved these changes May 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/godfisk 586/redirect module handle url with slash in the end #21

Feature/godfisk 586/redirect module handle url with slash in the end #21

grom8255 commented May 11, 2018 •

edited by VladimirLevchuk

VladimirLevchuk May 16, 2018

VladimirLevchuk May 16, 2018

Romanets May 18, 2018

Romanets May 18, 2018

Romanets May 18, 2018

Romanets May 18, 2018

Romanets May 18, 2018

		@@ -23,6 +25,14 @@ public CustomRedirect(string oldUrl, string newUrl, bool appendMatchToNewUrl, bo

		}

		public CustomRedirect(CustomRedirect redirect)

		@@ -23,6 +25,16 @@ public CustomRedirect(string oldUrl, string newUrl, bool appendMatchToNewUrl, bo

		}

		public CustomRedirect(CustomRedirect redirect)

Feature/godfisk 586/redirect module handle url with slash in the end #21

Are you sure you want to change the base?

Feature/godfisk 586/redirect module handle url with slash in the end #21

Conversation

grom8255 commented May 11, 2018 • edited by VladimirLevchuk

VladimirLevchuk May 16, 2018

Choose a reason for hiding this comment

VladimirLevchuk May 16, 2018

Choose a reason for hiding this comment

Romanets May 18, 2018

Choose a reason for hiding this comment

Romanets May 18, 2018

Choose a reason for hiding this comment

Romanets May 18, 2018

Choose a reason for hiding this comment

Romanets May 18, 2018

Choose a reason for hiding this comment

Romanets May 18, 2018

Choose a reason for hiding this comment

grom8255 commented May 11, 2018 •

edited by VladimirLevchuk