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

DNN-10462: Added support for DnnUrlHelper in DnnController for MVC #1925

Merged
merged 4 commits into from Nov 9, 2017

Conversation

Projects
None yet
3 participants
@ahoefling
Contributor

ahoefling commented Nov 7, 2017

Added support in the DnnController for DnnUrlHelper.

  • I added the DnnUrlHelper inside of the IDnnController and DnnController. Since the ASP.NET MVC object isn't really useful in DNN Routing I created our DNNUrlHelper as a new property which hides the base Url implementation. In the DnnController
  • The DnnController now overrides the ASP.NET MVC Controller method Initialize(RequestContext requestContext) and sets our DnnUrlHelper. This is the exact same way that the Controller works from ASP.NET MVC. This method will get called on every request and be able to appropriately set the DnnUrlHelper

Our code

protected override void Initialize(RequestContext requestContext)
{
    base.Initialize(requestContext);
    Url = new DnnUrlHelper(requestContext, this);
}

ASP.NET MVC Code
Code Snippet from ASP.NET MVC repo

protected override void Initialize(RequestContext requestContext)
{
    base.Initialize(requestContext);
    Url = new UrlHelper(requestContext);
}

Usage
Now in a DNN MVC Module we can handle routing inside of the controller. Suppose you are in a scenario where you are handling a form submission with a HttpPost and you want to redirect to a new controller route. Now you can just specify Url.Action("NewRoute"), see snippet below:

public class FooController : DnnController
{
    public ActionResult Index()
    {
        return View();
    }

    [HttpPost]
    public ActionResult Index(object data)
    {
        // do something with form data
        
        return Redirect(Url.Action("NewAction"));
    }

    public ActionResult NewAction()
    {
        return View();
    }
}
@@ -28,5 +29,7 @@ public interface IDnnController : IController
ViewEngineCollection ViewEngineCollectionEx { get; set; }
DnnUrlHelper Url { get; set; }

This comment has been minimized.

@amarjit-dhunna

amarjit-dhunna Nov 8, 2017

Contributor

Why do we need this property here? System.Web.Mvc.Controller already has the Url property which is being overridden by using the new keyword in DnnController.

This comment has been minimized.

@ahoefling

ahoefling Nov 8, 2017

Contributor

As far as I know the DnnUrlHelper does not inherit from the System.Web.Mvc.UrlHelper UrlHelper which are different objects. This is why I chose to create the DnnUrlHelper inside the IDnnController. This way if someone is using the interface they will have access to the appropriate DnnUrlHelper.

See DnnUrlHelper from the platform code. Snippet below:

namespace DotNetNuke.Web.Mvc.Helpers
{
    public class DnnUrlHelper
    {

Taking a closer look at the System.Web.Mvc UrlHelper. It appears we can take our current DnnUrlHelper and inherit from it and override all the Action methods. Without making this change we will not be able to properly override the Url property in the DnnController

Here are some proposed changes based off the feedback provided:

  • Update the DnnUrlHelper to inherit and override the System.Web.Mvc.UrlHelper
  • Remove the DnnUrlHelper from the IDnnController
    • This can be replaced with the base class System.Web.Mvc.UrlHelper or omitted completly
  • We should be able to still override the Initialize() method and set the System.Web.Mvc.UrlHelper equal to our implementation of DnnUrlHelper

If these changes all make sense to you, I'll go ahead and update the PR

This comment has been minimized.

@amarjit-dhunna

amarjit-dhunna Nov 8, 2017

Contributor

@ahoefling Regarding DnnUrlHelper in IDnnController since you are using new and same property name, it automatically hides the UrlHelper from System.Web.Mvc.Controller. We are doing same for User property as well. Further, I can agree on the point of reuse the helper if IDnnController is used but I don't see much usage of implementing it alone. But we can still leave it the way you did it assuming it to be better way of hiding the property in this scenario.

Regarding inheriting from UrlHelper, We can do that way but we can not just override the implementation of Action methods and leave others untouched. If we want to inherit from UrlHelper then all other methods should be overridden and implemented in a way that they are useful for DNN MVC framework. We don't want to include methods in DnnUrlHelper which are not useful and will create incorrect results.
If you still want to use that approach then that will need to implement all the methods from UrlHelper, verify the proper working for them and add test cases for all of them.

This comment has been minimized.

@ahoefling

ahoefling Nov 8, 2017

Contributor

@amarjit-dhunna wrote:

Regarding DnnUrlHelper in IDnnControllersince you are using new and same property name, it automatically hides the UrlHelper from System.Web.Mvc.Controller. We are doing same for User property as well.

It sounds like the first approach is an established best practice that we have used for the User object. Based on your feedback I think it will be best to use the new opperator to hide the the base implementation of UrlHelper so module devs will get just the working DnnUrlHelper.

I believe it to be best to keep the IDnnController the way currently proposed in this PR. Since the Controller class uses a different object for the Url property (UrlHelper vs DnnUrlHelper) it will be difficult to write any generic code around the IDnnController where the developer would want to leverage the DnnUrlHelper.

@amarjit-dhunna wrote:

Regarding inheriting from UrlHelper, We can do that way but we can not just override the implementation of Action methods and leave others untouched.

I completely agree with you and while this may be a great solution in the future i think it would be best handled in another PR that expands upon the changes here. This may be something I am willing to pick up after we get through this change.

Action Items I have gathered from this discussion:

  • Lets keep the PR as proposed including leaving the reference to IDnnUrlHelper on the IDnnController
  • Add unit tests as necessary to cover new code paths

Please sign off on this direction if you are okay with it and I'll update the PR when ready

This comment has been minimized.

@amarjit-dhunna

amarjit-dhunna Nov 8, 2017

Contributor

Yes. So only task pending for you is to add unit tests to cover the new code.

@amarjit-dhunna

This comment has been minimized.

Contributor

amarjit-dhunna commented Nov 8, 2017

@ahoefling Changes look good. One question added in the code. Further, Can you add some unit tests for testing the expected output?

@ahoefling

This comment has been minimized.

Contributor

ahoefling commented Nov 8, 2017

@amarjit-dhunna thanks for the feedback, I replied to the code comment in-line and I'm looking for feedback prior to pushing more changes. Once we have a direction, I'll make the changes and add test coverage

@ahoefling

This comment has been minimized.

Contributor

ahoefling commented Nov 8, 2017

Properly isolating the overidden Initialize method is very difficult. I'm going to add some code into the FakeDnnController to simulate it's execution instead.

I attempted mocking out the dependencies to create an instance of MvcHandler to simulate the full stack of the request but due to several internal objects in System.Web.Mvc and System.Web it is very difficult and will add a lot of unnecessary complications to the test code. I believe our test case of just calling initialize is sufficient.

(I'm still working on test code)

ahoefling added some commits Nov 8, 2017

DNN-10462: Added unit tests for DnnController. Added Controller Initi…
…alize simulation code so we can properly test the new DnnController Initialize
@@ -77,7 +93,7 @@ public virtual bool IsLocalUrl(string url)
public virtual string Action()
{
return _viewContext.RequestContext.HttpContext.Request.RawUrl;
return UrlHelper.RequestContext.HttpContext.Request.RawUrl;

This comment has been minimized.

@ahoefling

ahoefling Nov 8, 2017

Contributor

I updated this code to normalize the 2 code paths we have in the DnnUrlHelper. Now regardless if you are using the viewContext or requestContext this method generates the url using the same RequestContext path which is retrieved from the UrlHelper

UrlHelper = new UrlHelper(requestContext);
_controller = controller;
if (_controller == null)

This comment has been minimized.

@amarjit-dhunna

amarjit-dhunna Nov 9, 2017

Contributor

I think we don't need this check in this Constructor. I can't think of a case in which controller can be null.

This comment has been minimized.

@ahoefling

ahoefling Nov 9, 2017

Contributor

I agree, it was more or less copied and pasted from the other constructor. I'll update and push new changes

@ahoefling

This comment has been minimized.

Contributor

ahoefling commented Nov 9, 2017

@amarjit-dhunna we should be all set

@amarjit-dhunna amarjit-dhunna merged commit 89f58e4 into dnnsoftware:development Nov 9, 2017

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