Redirects with fragment #5519

juunas11 · 2016-11-08T21:16:17Z

Added overloads to the methods that return redirect results that allow specifying the fragment to add to the URL.

Added RedirectToAction, RedirectToActionPermanent, RedirectToRoute, and RedirectToRoutePermanent overloads that allow specifying the fragment
Added the Fragment property to RedirectToActionResult and RedirectToRouteResult
Modified RedirectToActionResultExecutor and RedirectToRouteResultExecutor to call the overload on UrlHelper that specifies the fragment. Left host and protocol to null as the other UrlHelper overloads do.

Addresses issue #3988

This is my first PR to MVC Core, I would be happy about any feedback :)

…to redirect to.

dnfclas · 2016-11-08T21:16:20Z

Hi @juunas11, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-11-08T21:23:10Z

@juunas11, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Eilon · 2016-11-18T22:56:04Z

@juunas11 thanks for the PR, we'll take a look!

@dougbu assigning to you to review and merge.

dougbu

Mostly small stuff and a bit more testing…

dougbu · 2016-11-21T02:56:42Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

            {
-                UrlHelper = Url,
+                UrlHelper = Url


Undo this. We like the trailing commas. Besides, let's not mix changing styles into this PR.

Did not know it was a style decision :) Thanks for pointing out, will do so in the future.

dougbu · 2016-11-21T02:59:12Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

            {
-                UrlHelper = Url,
+                UrlHelper = Url


Ditto here and the places below where you made this change.

dougbu · 2016-11-21T03:03:31Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

@@ -1082,7 +1219,7 @@ public virtual AcceptedAtRouteResult AcceptedAtRoute(object routeValues)
        /// <summary>
        /// Creates a <see cref="AcceptedAtRouteResult"/> object that produces an Accepted (202) response.
        /// </summary>
-        /// <param name="routeName">The name of the route to use for generating the URL.</param>       
+        /// <param name="routeName">The name of the route to use for generating the URL.</param>


I can undo this one if you want, left the whitespace deleter on auto :)

I was thanking you here. Removing trailing whitespace in a file you're changing is fine.

dougbu · 2016-11-21T03:12:06Z

src/Microsoft.AspNetCore.Mvc.Core/RedirectToActionResult.cs

+        {
+        }
+
+        public RedirectToActionResult(


Please add doc comments for this and the other new constructor overload. No need to do the same for the existing constructor overloads -- unless you're feeling industrious! 😺

dougbu · 2016-11-21T03:13:19Z

src/Microsoft.AspNetCore.Mvc.Core/RedirectToActionResult.cs

@@ -53,6 +73,11 @@ public class RedirectToActionResult : ActionResult, IKeepTempDataResult

        public bool Permanent { get; set; }


If you're feeling particularly industrious, feel free to add doc comments for this property as well 😺

dougbu · 2016-11-21T03:21:18Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RedirectToActionResultTest.cs

+        {
+            // Arrange
+            var expectedUrl = "/Home/SampleAction#test";
+            var expectedPermanentFlag = false;


nit: just expectedPermanent

dougbu · 2016-11-21T03:24:24Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RedirectToActionResultTest.cs

+            var httpResponse = new Mock<HttpResponse>();
+            httpContext
+                .Setup(o => o.Response)
+                .Returns(httpResponse.Object);


Should be no need to mock HttpContext or HttpResponse. Try using DefaultHttpContext and setting RequestServices instead.

This is actually the way it was before I touched it :) But I'll be glad to refactor it to less setup.

Guess the existing tests weren't the best model to follow. Don't change them but do avoid unnecessary mocks in tests you add.

dougbu · 2016-11-21T03:25:54Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RedirectToActionResultTest.cs

+            await result.ExecuteResultAsync(actionContext);
+
+            // Assert
+            // Verifying if Redirect was called with the specific Url and parameter flag.


nit: "the specific Url and parameter flag" -> "the expected URL and permanent parameters"

dougbu · 2016-11-21T03:26:28Z

test/WebSites/BasicWebSite/Controllers/HomeController.cs

@@ -35,11 +35,21 @@ public IActionResult RedirectToActionReturningTaskAction()
            return RedirectToAction("ActionReturningTask");
        }

+        public IActionResult RedirectToActionWithFragment()


This and the method below are not used. Probably should be. I'm not saying we need tests of all 8 new ControllerBase overloads but zero is too low. Suggest covering the two methods used here and the longest RedirectToActionPermanent() overload.

Thanks for all the nice comments, will update and get back to this when I can! 👍

dougbu · 2016-11-21T03:34:22Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RedirectToActionResultTest.cs

@@ -83,6 +83,41 @@ public class RedirectToActionResultTest
                "No route matches the supplied values.");
        }

+        [Fact]
+        public async Task RedirectToAction_Execute_WithFragment_PassesCorrectValuesToRedirect()


Add a similar test in RedirectToRouteResultTest, perhaps covering the overload accepting permanent: true instead.

@dougbu

… back per @dougbu's comment.

…ctToActionResult.cs.

…ionResult.cs.

…oRouteResult.

…tToRouteResult.

…ult.

…ToActionResult XML comment.

dougbu

Added a few more comments. Also looks like a few more tests are still needed.

dougbu · 2016-11-28T20:13:42Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

+
+        /// <summary>
+        /// Redirects to the specified route with <see cref="RedirectToRouteResult.Permanent"/> set to true
+        /// using the specified <paramref name="routeName"/>, and <paramref name="fragment"/>.


Remove the comma here.

dougbu · 2016-11-28T20:17:20Z

src/Microsoft.AspNetCore.Mvc.Core/RedirectToActionResult.cs

@@ -51,8 +108,16 @@ public class RedirectToActionResult : ActionResult, IKeepTempDataResult
        /// </summary>
        public RouteValueDictionary RouteValues { get; set; }

+        /// <summary>
+        /// Gets or sets whether the redirect is permanent.


"Gets or sets an indication that …"

dougbu · 2016-11-28T20:18:26Z

src/Microsoft.AspNetCore.Mvc.Core/RedirectToRouteResult.cs

@@ -48,8 +104,16 @@ public RedirectToRouteResult(object routeValues)
        /// </summary>
        public RouteValueDictionary RouteValues { get; set; }

+        /// <summary>
+        /// Gets or sets whether the redirect is permanent.


"Gets or sets an indication that …"

dougbu · 2016-11-28T20:21:26Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RedirectToActionResultTest.cs

+            var httpResponse = new Mock<HttpResponse>();
+            httpContext
+                .Setup(o => o.Response)
+                .Returns(httpResponse.Object);


Guess the existing tests weren't the best model to follow. Don't change them but do avoid unnecessary mocks in tests you add.

dougbu · 2016-12-06T04:53:33Z

@juunas11 this PR is very close. Any chance you can wrap up the few remaining comments soon?

juunas11 · 2016-12-06T14:41:15Z

Yes, I'll try to get everything done in the following one or two days.

…oActionResult, and RedirectToRouteResult

… overloads of RedirectToAction and RedirectToRoutePermanent

juunas11 · 2016-12-06T16:10:20Z

I think I've done everything you requested, but please tell me if there is still something.

dougbu

Revert changes to one file and I'll get this in.

Thanks very much @juunas11

dougbu · 2016-12-06T17:30:04Z

test/WebSites/BasicWebSite/Controllers/HomeController.cs

@@ -35,11 +35,21 @@ public IActionResult RedirectToActionReturningTaskAction()
            return RedirectToAction("ActionReturningTask");
        }

+        public IActionResult RedirectToActionWithFragment()


Please revert the changes to this test controller. None of the functional tests uses the two new methods and I've decided coverage is fine with the new unit tests.

dougbu · 2016-12-06T18:32:21Z

1dd1d49

Joonas Westlin added 5 commits November 8, 2016 22:12

Added implementation of overloads where you can specify the fragment …

57716f2

…to redirect to.

Added unit test for RedirectToActionResult with fragment

39857ee

Added XML comments

e7e8108

Added implementation for including the fragment with permanen redirects

b46d6b4

Added RedirectToRoute overloads where you can specify a fragment.

f8b11e8

dnfclas added the cla-required label Nov 8, 2016

dnfclas added cla-signed and removed cla-required labels Nov 8, 2016

Eilon assigned dougbu Nov 18, 2016

dougbu suggested changes Nov 21, 2016

View reviewed changes

Joonas Westlin added 12 commits November 28, 2016 21:36

Added missing dots in XML comments in ControllerBase.cs

b29b295

Added the trailing commas in object initializers in ControllerBase.cs…

5867644

… back per @dougbu's comment.

Added the missing XML comments for the constructors I added in Redire…

129920a

…ctToActionResult.cs.

Added missing dot to end of XML comment for Fragment in RedirectToAct…

a39a4e3

…ionResult.cs.

Added missing constructor XML comments to RedirectToActionResult.

6c7c312

Added XML comment for Permanent property in RedirectToActionResult.

1b26743

Added XML comment for RedirectToActionResult class.

b463b00

Fixed Fragment property XML comment per @dougbu comments in RedirectT…

bffddfe

…oRouteResult.

Added missing XML comments to the two constructors I added in Redirec…

c4939a9

…tToRouteResult.

Added XML comments to the existing constructors in RedirectToRouteRes…

bf81dcf

…ult.

Added XML comment to Permanent property in RedirectToRouteResult.

e8508a2

Added XML comment RedirectToRouteResult class and synced the Redirect…

d89b0df

…ToActionResult XML comment.

dougbu suggested changes Nov 28, 2016

View reviewed changes

Joonas Westlin added 2 commits December 6, 2016 18:01

Small XML comment adjustments per review in ControllerBase, RedirectT…

55189b0

…oActionResult, and RedirectToRouteResult

Modified BasicWebSite test project HomeController to call the largest…

7add385

… overloads of RedirectToAction and RedirectToRoutePermanent

Added more tests for redirect results and ControllerBase functions

56a4892

dougbu approved these changes Dec 6, 2016

View reviewed changes

Reverted changes to test website HomeController

b02fb81

dougbu merged commit 1dd1d49 into aspnet:dev Dec 6, 2016

juunas11 deleted the redirect-to-action-with-fragment branch December 6, 2016 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirects with fragment #5519

Redirects with fragment #5519

juunas11 commented Nov 8, 2016

dnfclas commented Nov 8, 2016

dnfclas commented Nov 8, 2016

Eilon commented Nov 18, 2016

dougbu left a comment

dougbu Nov 21, 2016

juunas11 Nov 28, 2016

dougbu Nov 21, 2016

dougbu Nov 21, 2016

juunas11 Nov 28, 2016

dougbu Nov 28, 2016

dougbu Nov 21, 2016

dougbu Nov 21, 2016

dougbu Nov 21, 2016

dougbu Nov 21, 2016

juunas11 Nov 28, 2016

dougbu Nov 28, 2016

dougbu Nov 21, 2016

dougbu Nov 21, 2016

juunas11 Nov 21, 2016

dougbu Nov 21, 2016

dougbu left a comment

dougbu Nov 28, 2016

dougbu Nov 28, 2016

dougbu Nov 28, 2016

dougbu Nov 28, 2016

dougbu commented Dec 6, 2016

juunas11 commented Dec 6, 2016

juunas11 commented Dec 6, 2016

dougbu left a comment

dougbu Dec 6, 2016

dougbu commented Dec 6, 2016

		@@ -53,6 +73,11 @@ public class RedirectToActionResult : ActionResult, IKeepTempDataResult

		public bool Permanent { get; set; }

Redirects with fragment #5519

Redirects with fragment #5519

Conversation

juunas11 commented Nov 8, 2016

dnfclas commented Nov 8, 2016

dnfclas commented Nov 8, 2016

Eilon commented Nov 18, 2016

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Dec 6, 2016

juunas11 commented Dec 6, 2016

juunas11 commented Dec 6, 2016

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Dec 6, 2016