Custom error messages with validation message tag helper #8035 #8087

kishanAnem · 2018-07-15T07:26:30Z

I think I fixed this bug. Is this right way to fix?
If yes, I'll add test cases.

dougbu

Very close. And, yes, needs tests.

Separately, please rebase your branch on our release/2.2. Although not messing up the comparison, 6 extra commits are visible.

dougbu · 2018-07-15T19:44:23Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

@@ -71,11 +71,13 @@ public ValidationMessageTagHelper(IHtmlGenerator generator)
                    };
                }

+                var childContent = await output.GetChildContentAsync();


Set childContent to null if output.IsContentModified; the GetChildContentAsync() call is expensive.

dougbu · 2018-07-15T19:45:28Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

                var tagBuilder = Generator.GenerateValidationMessage(
                    ViewContext,
                    For.ModelExplorer,
                    For.Name,
-                    message: null,
+                    message: childContent.IsEmptyOrWhiteSpace ? null : childContent.GetContent(),


This condition also needs to check output.IsContentModified and to pass message: null in that case.

dougbu · 2018-07-15T19:55:05Z

By the way, test failures are caused by not checking output.IsContentModified where I mentioned.

dougbu · 2018-07-15T20:16:32Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

@@ -89,7 +91,7 @@ public ValidationMessageTagHelper(IHtmlGenerator generator)
                        // We check for whitespace to detect scenarios such as:
                        // <span validation-for="Name">
                        // </span>
-                        var childContent = await output.GetChildContentAsync();
+
                        if (childContent.IsEmptyOrWhiteSpace)
                        {
                            // Provide default message text (if any) since there was nothing useful in the Razor source.


Should have mentioned:

This comment is no longer correct since message may have come from the Razor source now

The else block starting at line 103 is no longer needed

The comment above about whitespace belongs above the GenerateValidationMessage(...) call

I suspect this block could now be narrowed to:

if (!output.IsContentModified && tagBuilder.HasInnerHtml)) { output.Content.SetHtmlContent(tagBuilder.InnerHtml); }

kishanAnem · 2018-07-17T15:46:06Z

Hi @dougbu

But I'm able to run tests from CMD line. I'm getting following error, when I run tests on visual studio.
Sorry for asking this favor. iIf you have time, please help me out.

[17-07-2018 09:01:26 PM Informational] ------ Run test started ------
[17-07-2018 09:02:59 PM Error] Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process exited with error: It was not possible to find any compatible framework version
The specified framework 'Microsoft.NETCore.App', version '2.2.0-preview1-26618-02' was not found.

Check application dependencies and target a framework version installed at:
C:\Program Files\dotnet\
Installing .NET Core prerequisites might help resolve this problem:
http://go.microsoft.com/fwlink/?LinkID=798306&clcid=0x409
The .NET Core framework and SDK can be installed from:
https://aka.ms/dotnet-download
The following versions are installed:
1.0.5 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.0.9 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.0.10 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.0.11 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.1.0-preview1-001100-00 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.1.2 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.1.6 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.1.7 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
1.1.8 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.0.0 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.0.5 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.0.6 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.0.7 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.1.0-preview1-26216-03 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.1.0 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.2.0-preview1-26527-01 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.2.0-preview1-26609-01 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
2.2.0-preview1-26609-02 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
3.0.0-preview1-26621-01 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

dougbu · 2018-07-17T16:14:48Z

@kishanAnem this likely means your Path environment variable includes C:\Program Files\dotnet. Remove that and add %UserProfile%\.dotnet\x64. Then restart Visual Studio.

kishanAnem · 2018-07-17T16:26:18Z

Thank you very much. It worked... 👍

kishanAnem · 2018-07-19T14:49:18Z

Hi @dougbu

commit title is misleading sorry for that tried to revert.
Please let me know fix is correct. 2 functional test are failed. if fix is correct, will fix function test cases.

dougbu

Looks better but lost the IsEmptyOrWhiteSpace check

dougbu · 2018-07-19T16:29:33Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

@@ -71,37 +71,29 @@ public ValidationMessageTagHelper(IHtmlGenerator generator)
                    };
                }

+                string message = null;
+                TagHelperContent tagHelperContent = null;


No need for this outside the next block i.e. just use var tagHelperContent = await output.GetChildContentAsync();

dougbu · 2018-07-19T16:29:39Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

+                if (!output.IsContentModified)
+                {
+                    tagHelperContent = await output.GetChildContentAsync();
+                    message = tagHelperContent.GetContent();


Need to check tagHelperContent.IsEmptyOrWhiteSpace before updating message.

dougbu · 2018-07-19T16:30:25Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

                var tagBuilder = Generator.GenerateValidationMessage(
                    ViewContext,
                    For.ModelExplorer,
                    For.Name,
-                    message: null,
+                    message: output.IsContentModified ? null : message,


Just message since it's null if output.IsContentModified

dougbu · 2018-07-19T16:32:41Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

@@ -358,16 +358,16 @@ public class ValidationMessageTagHelperTest

        [Theory]
        [InlineData("Content of validation message", "Content of validation message")]
-        [InlineData("\r\n  \r\n", "New HTML")]


This test should work as it did before.

dougbu · 2018-07-19T16:34:56Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

+
+            var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
+            var setup = generator
+                .Setup(mock => mock.GenerateValidationMessage(


This setup should confirm the expected message is passed

dougbu · 2018-07-19T16:37:56Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

+            validationMessageTagHelper.ViewContext = viewContext;
+
+            // Act
+            validationMessageTagHelper.ProcessAsync(context, output).Wait();


If you're calling ProcessAsync(...), use await and change the method's signature to include async Task

dougbu

Almost…

dougbu · 2018-07-21T18:01:10Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

                    tag: null,
                    htmlAttributes: htmlAttributes);

                if (tagBuilder != null)
                {
                    output.MergeAttributes(tagBuilder);

-                    // Do not update the content if another tag helper targeting this element has already done so.


Restore this comment

dougbu · 2018-07-21T18:01:44Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

                    if (!output.IsContentModified)
                    {
-                        // We check for whitespace to detect scenarios such as:
-                        // <span validation-for="Name">
-                        // </span>


Restore this comment but above line 78

dougbu · 2018-07-21T18:02:58Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/ValidationMessageTagHelper.cs

-                        // </span>
-                        var childContent = await output.GetChildContentAsync();
-                        if (childContent.IsEmptyOrWhiteSpace)
+                        if (string.IsNullOrWhiteSpace(message) && tagBuilder.HasInnerHtml)


Get rid of the string.IsNullOrWhiteSpace(message) check and the else because whitespace should be impossible given the earlier tagHelperContent.IsEmptyOrWhiteSpace check and the IHtmlGenerator has already done the right thing with message. This block should just be

if (!output.IsContentModified && tagBuilder.HasInnerHtml) { output.Content.SetHtmlContent(tagBuilder.InnerHtml); }

dougbu · 2018-07-21T18:04:32Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

@@ -364,10 +364,11 @@ public class ValidationMessageTagHelperTest
            string expectedOutputContent)
        {
            // Arrange
-            var tagBuilder = new TagBuilder("span2");
+            var tagBuilder = new TagBuilder("span");


After looking more closely at these tests, I see ProcessAsync_MergesTagBuilderFromGenerateWithoutValidationMessage() should be redundant. Remove the new test and revert current changes to ProcessAsync_MergesTagBuilderFromGenerateValidationMessage(...). Then, add an expectedMessage parameter and the right data values ("Content of validation message" and null) and use expectedMessage in the Setup(...) at line 379.

Don't add the ProcessAsync_MergesTagBuilderFromGenerateWithoutValidationMessage(...) case ([InlineData(null, null, "New HTML")]) here because there's a bug in DefaultTagHelperContent.IsEmptyOrWhiteSpaceCore(). It returns false when the only content is null.

Congrats on unearthing that aspnet/Razor bug!

Filed aspnet/Razor#2497 and am doing a quick fix

Thanks, this happens with great support.

dougbu · 2018-07-21T18:32:53Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

+                    It.IsAny<ViewContext>(),
+                    It.IsAny<ModelExplorer>(),
+                    It.IsAny<string>(),
+                    "",


FYI we always use string.Empty unless a constant is required

dougbu · 2018-07-21T18:32:59Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/ValidationMessageTagHelperTest.cs

+
+            var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
+            var setup = generator
+                .Setup(mock => mock.GenerateValidationMessage(


dougbu · 2018-07-22T02:39:31Z

@kishanAnem please git rebase -i HEAD~9 and delete the first 6 commits. Those commits don't contribute real differences, just confuse the branches.

(HEAD~9 might need to be HEAD~10 or more if you've added new commits locally.)

dougbu · 2018-07-22T02:42:48Z

And, ignore the VSTS failure. Not sure why the CacheTagHelperTest.ProcessAsync_AwaitersUseTheResultOfExecutor(...) test isn't working there. But, it's definitely not related to changes in this PR.

kishanAnem · 2018-07-22T07:38:52Z

Hi @dougbu ,
Sorry.
I'm trying to git rebase -i HEAD~9 . I endup to two more commits.
I think you understand i'm naive to GIT. I will revert those commits.

kishanAnem · 2018-07-22T07:44:06Z

Hi @dougbu
I think revert these changes also. Will open new PR.

aspnet#8035 PR aspnet#8087 aspnet/Razor#2497

dougbu suggested changes Jul 15, 2018

View reviewed changes

dougbu self-assigned this Jul 15, 2018

dougbu mentioned this pull request Jul 15, 2018

Custom error messages with validation message tag helper #8035

Closed

dougbu reviewed Jul 15, 2018

View reviewed changes

dougbu suggested changes Jul 19, 2018

View reviewed changes

dougbu suggested changes Jul 21, 2018

View reviewed changes

dougbu mentioned this pull request Jul 21, 2018

DefaultTagHelperContent.IsEmptyOrWhiteSpace is incorrectly false when only content is null aspnet/Razor#2497

Closed

kishanAnem force-pushed the dev branch from 0b9ffaf to c2d8123 Compare July 22, 2018 07:33

kishanAnem closed this Jul 22, 2018

kishanAnem force-pushed the dev branch from c2d8123 to 777782a Compare July 22, 2018 07:41

kishanAnem pushed a commit to kishanAnem/Mvc that referenced this pull request Jul 22, 2018

Custom error messages with validation message tag helper aspnet#8035

c0ba374

aspnet#8035 PR aspnet#8087 aspnet/Razor#2497

kishanAnem mentioned this pull request Jul 22, 2018

Custom error messages with validation message tag helper #8035 #8123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom error messages with validation message tag helper #8035 #8087

Custom error messages with validation message tag helper #8035 #8087

kishanAnem commented Jul 15, 2018

dougbu left a comment

dougbu Jul 15, 2018

kishanAnem Jul 16, 2018

dougbu Jul 15, 2018

dougbu commented Jul 15, 2018

dougbu Jul 15, 2018

kishanAnem commented Jul 17, 2018

dougbu commented Jul 17, 2018

kishanAnem commented Jul 17, 2018

kishanAnem commented Jul 19, 2018 •

edited

dougbu left a comment

dougbu Jul 19, 2018

dougbu Jul 19, 2018

dougbu Jul 19, 2018

dougbu Jul 19, 2018

dougbu Jul 19, 2018

dougbu Jul 21, 2018

dougbu Jul 19, 2018

dougbu left a comment

dougbu Jul 21, 2018

dougbu Jul 21, 2018

dougbu Jul 21, 2018

dougbu Jul 21, 2018

dougbu Jul 21, 2018

kishanAnem Jul 22, 2018

dougbu Jul 21, 2018

dougbu Jul 21, 2018

dougbu commented Jul 22, 2018

dougbu commented Jul 22, 2018

kishanAnem commented Jul 22, 2018

kishanAnem commented Jul 22, 2018

Custom error messages with validation message tag helper #8035 #8087

Custom error messages with validation message tag helper #8035 #8087

Conversation

kishanAnem commented Jul 15, 2018

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

dougbu commented Jul 15, 2018

Choose a reason for hiding this comment

kishanAnem commented Jul 17, 2018

dougbu commented Jul 17, 2018

kishanAnem commented Jul 17, 2018

kishanAnem commented Jul 19, 2018 • edited

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

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

dougbu commented Jul 22, 2018

dougbu commented Jul 22, 2018

kishanAnem commented Jul 22, 2018

kishanAnem commented Jul 22, 2018

kishanAnem commented Jul 19, 2018 •

edited