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

Jump to bottom

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

Merged
merged 4 commits into from Nov 9, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
671dfd2
DNN-10462: Added support for DnnUrlHelper in DnnController
Nov 7, 2017
c1365d6
DNN-10462: Added unit tests for DnnController. Added Controller Init…
Nov 8, 2017
3e59867
DNN-10462: Added unit tests to DnnUrlHelper
Nov 8, 2017
d104dff
DNN-10462: Removed redundent null controller check in the DnnUrlHelper
Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions DNN Platform/DotNetNuke.Web.Mvc/Framework/Controllers/DnnController.cs
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Text;
using System.Web.Mvc;
using System.Web.Routing;
using System.Web.UI;
using DotNetNuke.Entities.Modules;
using DotNetNuke.Entities.Modules.Actions;
Expand All @@ -14,6 +15,7 @@
using DotNetNuke.UI.Modules;
using DotNetNuke.Web.Mvc.Framework.ActionResults;
using DotNetNuke.Web.Mvc.Framework.Modules;
using DotNetNuke.Web.Mvc.Helpers;

namespace DotNetNuke.Web.Mvc.Framework.Controllers
{
Expand All @@ -36,6 +38,8 @@ public TabInfo ActivePage

public Page DnnPage { get; set; }

public new DnnUrlHelper Url { get; set; }

public string LocalResourceFile { get; set; }

public string LocalizeString(string key)
Expand Down Expand Up @@ -119,6 +123,12 @@ protected override PartialViewResult PartialView(string viewName, object model)
};
}

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

public ViewEngineCollection ViewEngineCollectionEx { get; set; }
}
}
3 changes: 3 additions & 0 deletions DNN Platform/DotNetNuke.Web.Mvc/Framework/Controllers/IDnnController.cs
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@
using System.Web.UI;
using DotNetNuke.Entities.Modules.Actions;
using DotNetNuke.UI.Modules;
using DotNetNuke.Web.Mvc.Helpers;

namespace DotNetNuke.Web.Mvc.Framework.Controllers
{
Expand All @@ -28,5 +29,7 @@ public interface IDnnController : IController

ViewEngineCollection ViewEngineCollectionEx { get; set; }

DnnUrlHelper Url { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


}
}
15 changes: 15 additions & 0 deletions DNN Platform/DotNetNuke.Web.Mvc/Helpers/DnnUrlHelper.cs
View file
Open in desktop
Expand Up @@ -22,6 +22,21 @@ public DnnUrlHelper(ViewContext viewContext)
{
}

public DnnUrlHelper(RequestContext requestContext, IDnnController controller)
{
Requires.NotNull("controller", controller);

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

if (_controller == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
throw new InvalidOperationException("The DnnUrlHelper class can only be used in Views that inherit from DnnWebViewPage");
}

ModuleContext = _controller.ModuleContext;
}

public DnnUrlHelper(ViewContext viewContext, RouteCollection routeCollection)
{
Requires.NotNull("viewContext", viewContext);
Expand Down