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.

Sign up for GitHub

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

OSOE-653: Relative URL prefixed with /Admin in case of an admin controller action if [Route] attribute is used in Lombiq.HelpfulLibraries #208

Merged
merged 3 commits into from
Jul 12, 2023
Merged

OSOE-653: Relative URL prefixed with /Admin in case of an admin controller action if [Route] attribute is used in Lombiq.HelpfulLibraries #208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b246ce9
Checking if the action have route attribute too
DemeSzabolcs Jul 10, 2023
6102cf4
Adding a test action with custom route but also admin attribute
DemeSzabolcs Jul 10, 2023
dfb290a
Merge remote-tracking branch 'origin/dev' into issue/OSOE-653
DemeSzabolcs Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lombiq.HelpfulLibraries.OrchardCore/Mvc/TypedRoute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ public string ToString(HttpContext httpContext)
/// </summary>
public override string ToString()
{
var prefix = _isAdminLazy.Value ? "/Admin/" : "/";
var isAdminWithoutRoute = _isAdminLazy.Value && _action.GetCustomAttribute(typeof(RouteAttribute)) == null;

var prefix = isAdminWithoutRoute ? "/Admin/" : "/";
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the context but is it correct to use an hardcoded /Admin prefix, shouldn't it be retrieved from the current AdminOptions.AdminUrlPrefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I didn't change that since that was not the focus of the issue and I didn't want to mess around. But it's a good point, I can create an issue for that!

Copy link
Member

Choose a reason for hiding this comment

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

#209

var route = _routeLazy.Value;
var arguments = _arguments.Any()
? "?" + string.Join('&', _arguments.Select((key, value) => $"{key}={WebUtility.UrlEncode(value)}"))
Expand Down
4 changes: 4 additions & 0 deletions Lombiq.HelpfulLibraries.Tests/Controllers/RouteTestController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ public class RouteTestController : Controller
[Route("I/Am/Routed")]
public IActionResult Route() => Content(string.Empty);

[Admin]
[Route("I/Am/Routed/Admin")]
public IActionResult AdminRoute() => Content(string.Empty);

[Route("content/{id}")]
public IActionResult RouteSubstitution(int id) => Content(string.Empty);

Expand Down
7 changes: 7 additions & 0 deletions Lombiq.HelpfulLibraries.Tests/UnitTests/Models/TypedRouteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ static Expression<Action<RouteTestController>> AsExpression(
noTenant,
},
new object[]
{
"/I/Am/Routed/Admin",
AsExpression(controller => controller.AdminRoute()),
noMoreArguments,
noTenant,
},
new object[]
{
"/I/Am/Routed?wat=is+this",
AsExpression(controller => controller.Route()),
Expand Down