[Feature] String should be an acceptable value for a checkbox if it can be parsed as a boolean by Elfocrash · Pull Request #5845 · aspnet/Mvc

Elfocrash · 2017-02-22T15:58:57Z

If the asp-for value is string with the values "true" or "false" the code is able to process that as valid boolean and generate a proper checkbox around that.

However the typeof check in the GenerateCheckBox method prevents that.

Added a check that allows the value to be processed and create a checkbox if the value is a valid bool string.

…ed as a boolean

dnfclas · 2017-02-22T15:59:02Z

Hi @Elfocrash, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, DNFBOT;

Eilon · 2017-02-28T19:52:08Z

Hi @Elfocrash can you share more details about this PR? I believe the reason that MVC requires Boolean-looking values is that MVC's model binding feature only works with those values. In fact, it has to do extra stuff such as rendering a second "hidden checkbox" (really a hidden input field) to work with the whole system.

I'm concerned that with this change, it'll be easy for developers to render content that won't "round trip" when it's posted back to the server.

Elfocrash · 2017-02-28T20:30:07Z

Hello @Eilon.
I totally understand what you mean. My point is, it is not uncommon that some generic configuration values are saved as string are retrieved as string on a key-value/string-string pattern.
When you want to load them up and alter them you would have to parse true or false string values as bools in order for the input tag helper to render it as a checkbox but then for the asp-for value you have to use the parsed parameter, making keeping track of the original property impossible.
The way i work around it is i create a second hidden input that has the string value and with js i change box the visible and the hidden in order for my changes to be saved.

With this change, you don't need to mess with creating extra inputs and keeping track of many things in order to have model binding working.

Let me know if something is unclear and i will try to explain it better.

Eilon · 2017-03-04T17:50:31Z

@Elfocrash thanks, I think I understand the scenario. The concern I have is that the most common case with checkboxes is with Boolean values, and that this change would make it easy for developers to create unexpected behavior (because of the incompatibility with model binding).

Elfocrash · 2017-03-04T18:41:52Z

I see. Well if it doesn't make sense to do, then i will solve this locally with a custom tag helper. Thanks for considering.

Eilon · 2017-03-05T00:15:30Z

@Elfocrash I'll discuss with some others on the team before we make any final decision. We definitely appreciate your work and we want to make sure we balance compatibility and flexibility.

Elfocrash · 2017-03-06T10:20:35Z

@Eilon Great to hear. Locking something like a checkbox on a very specific datatype feels as a restriction, especially with the level of flexibility that C# and .NET offers as a whole.

Eilon · 2017-03-07T18:41:25Z

Ok I talked to @rynowak and @danroth27 and they convinced me this is a good change 😄 I had actually misunderstood part of this change, so they helped me understand it better.

@Elfocrash do you think you could add a unit test to cover this new scenario, both for positive and negative cases? (E.g. using the string trUE should work, and using the string bad would be ignored.)

Eilon · 2017-03-07T18:41:46Z

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

+            if (modelExplorer.ModelType == typeof(string))
+            {
+                bool potentialBool;
+                isValidBool = bool.TryParse(modelExplorer.Model.ToString(), out potentialBool);


Can Model be null here?

Eilon · 2017-03-07T18:42:39Z

@dougbu, assigning to you for model binding / HTML rendering expertise.

…ool logic for the checkbox

Eilon · 2017-03-07T21:33:08Z

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

+            try
+            {
+                // Act
+                tagHelper.Process(context, output);


I think there's some kind of Assert.Throws(...) helper, no? Might want to scan through some other tests to see the pattern. It can make the code a bit cleaner.

Good point, i tend to use mstest were you normally do ExpectedException 😄. Just a sec

Eilon · 2017-03-07T21:33:22Z

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

+        [InlineData("trUE")]
+        [InlineData("FAlse")]
+        public void CheckBoxHandlesParsableStringsAsBoolsCorrectly(
+        string possibleBool)


Indent this 😄

Eilon · 2017-03-07T21:41:59Z

@Elfocrash looking very good, thanks for submitting the tests! I think just some small changes are needed.

Elfocrash · 2017-03-07T23:15:29Z

@Eilon Did all the changes. Let me know if there is something more that needs to be done, thanks.

Eilon · 2017-03-08T00:35:41Z

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

+            string possibleBool)
+        {
+            // Arrange
+            var originalContent = "original content";


BTW if these aren't really variables, and they're just constants, you could do const string ... instead. That way it's clearer that they're never modified.

@Eilon we rarely use const in our test code beyond a few test classes for confirming our handling of consts and occasional values used in multiple assertions and shared (complicated) setup methods. Thoughts?

These values really are constants, and they really aren't variables, so I can't think of even one good reason to avoid using a const.

Eilon · 2017-03-08T00:37:05Z

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

+            var ex = Record.Exception(() => tagHelper.Process(context, output));
+
+            // Assert
+            Assert.Null(ex);


So this asserts that there was no exception, but where does it check that the right thing got rendered? We could presumably do something similar to how other checkbox tag helpers work? (I'm hoping we have some similar tests for this!)

We intentionally have very few "does not throw" tests and definitely don't use Record.Exception() to confirm the call records nothing. So, please set up an expectation similar to https://github.com/Elfocrash/Mvc/blob/6edb9c9fce2b5d78ed3d533df71abd18495f8d5b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs#L86-L88 and assert that's what comes out.

Eilon · 2017-03-08T00:37:37Z

@Elfocrash I think it's really close!

dougbu

Thanks for removing this restriction. PR is close.

dougbu · 2017-03-08T05:18:19Z

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

+                isValidBool = bool.TryParse(modelExplorer.Model?.ToString() ?? string.Empty, out potentialBool);
+            }
+
+            if (typeof(bool) != modelExplorer.ModelType && !isValidBool)


This makes sense given the history of the code but will be a bit easier to read as

var isValidBool = false; if (modelExplorer.ModelType == typeof(bool)) { isValidBool = true; } else if (modelExplorer.ModelType == typeof(string)) { // what you've got } if (!isValidBool) // et cetera

dougbu · 2017-03-08T05:35:10Z

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

+
+            if (typeof(bool) != modelExplorer.ModelType && !isValidBool)
            {
                throw new InvalidOperationException(Resources.FormatInputTagHelper_InvalidExpressionResult(


Please update the Resources.resx file and reword this. Resource currently says only that the model must be of type bool.

Better yet, update the resource to state the model must of type bool or string and add a new one for the string that can't be parsed as a bool e.g.

if (modelExplorer.ModelType == typeof(string)) { if (modelExplorer.Model != null) { // throw with new resource if parsing fails } } else if (modelExplorer.ModelType != typeof(bool)) { // throw with updated resource, passing one more parameter }

dougbu · 2017-03-08T05:45:43Z

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

+            string possibleBool)
+        {
+            // Arrange
+            var originalContent = "original content";


@Eilon we rarely use const in our test code beyond a few test classes for confirming our handling of consts and occasional values used in multiple assertions and shared (complicated) setup methods. Thoughts?

dougbu · 2017-03-08T06:04:01Z

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

+            var ex = Record.Exception(() => tagHelper.Process(context, output));
+
+            // Assert
+            Assert.Null(ex);


We intentionally have very few "does not throw" tests and definitely don't use Record.Exception() to confirm the call records nothing. So, please set up an expectation similar to https://github.com/Elfocrash/Mvc/blob/6edb9c9fce2b5d78ed3d533df71abd18495f8d5b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/InputTagHelperTest.cs#L86-L88 and assert that's what comes out.

Elfocrash · 2017-03-08T08:25:54Z

@doubgu will do, thanks

… exception and added content expectation and reorganized the code.

Elfocrash · 2017-03-08T10:53:55Z

@Eilon @dougbu All changes are now in place.

Eilon

Looks good to me!

Eilon · 2017-03-08T17:30:31Z

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

+                }
+                else if (modelExplorer.ModelType != typeof(bool))
+                {
+                    throw new InvalidOperationException(Resources.FormatInputTagHelper_InvalidExpressionResult(


A new record for number of string.Format substitutions, perhaps?? 😄

dougbu

Smaller touch-ups left.

dougbu · 2017-03-08T18:05:04Z

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

+            var expectedContent = $"{content}<input{isChecked} class=\"HtmlEncode[[form-control]]\" " +
+                                           "id=\"HtmlEncode[[IsACar]]\" name=\"HtmlEncode[[IsACar]]\" type=\"HtmlEncode[[checkbox]]\" " +
+                                           "value=\"HtmlEncode[[true]]\" /><input name=\"HtmlEncode[[IsACar]]\" type=\"HtmlEncode[[hidden]]\" " +
+                                           "value=\"HtmlEncode[[false]]\" />";


Just indent 4 spaces from the previous line.

dougbu · 2017-03-08T18:07:33Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/Resources.resx

+    <value>Unexpected '{1}' expression result type '{2}' for {0}. '{1}' must be of type '{3}' or '{4}' that can be parsed as a '{3}' if '{5}' is '{6}'.</value>
+  </data>
+  <data name="InputTagHelper_InvalidStringResult" xml:space="preserve">
+    <value>Unexpected '{1}' expression result type '{2}' for {0}. '{1}' is of type '{3}' for {5} but cannot be parsed as a '{4}'.</value>


The result type is fine. This should be more like Unexpected expression result value '{1}' for {0}. '{1}' cannot be parsed as a '{2}'.

dougbu · 2017-03-08T18:09:56Z

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

+                       "type",
+                       "checkbox"));
+                }
+


I wasn't clear enough in my earlier comments. This block and the one above can be combined. No need for the isValidBool variable. Then you're only checking the types once.

dougbu

LGTM

I have a small question on the resources for @Eilon (offline) but expect to get this in soon.

dougbu · 2017-03-08T22:22:47Z

015dafc

String should be an acceptable value for a checkbox if it can be pars…

4bc62a0

…ed as a boolean

dnfclas added the cla-signed label Feb 22, 2017

Eilon suggested changes Mar 7, 2017

View reviewed changes

Eilon assigned dougbu Mar 7, 2017

Added null check for the model and added two tests for the possible b…

32dd742

…ool logic for the checkbox

Eilon suggested changes Mar 7, 2017

View reviewed changes

Eilon mentioned this pull request Mar 7, 2017

Strings that can be parsed as a Boolean should be an acceptable value for a checkbox #5919

Closed

Elfocrash added 3 commits March 7, 2017 21:49

Checkbox bool parsing tests now use Assert.Throws

428b955

Fixed broken Assert.Throws text

dc8a103

string should be var in test method

6edb9c9

Eilon suggested changes Mar 8, 2017

View reviewed changes

dougbu suggested changes Mar 8, 2017

View reviewed changes

Tests now also check the content and other meta data based on the value

8bd1268

Elfocrash added 2 commits March 8, 2017 10:15

Throw different error for different scenarios, removed does not throw…

7e45fb0

… exception and added content expectation and reorganized the code.

Removed a resource that was added accidentally

e0ff31e

Eilon approved these changes Mar 8, 2017

View reviewed changes

dougbu suggested changes Mar 8, 2017

View reviewed changes

Added small changes as per @dougbu's request

f4b33f1

dougbu approved these changes Mar 8, 2017

View reviewed changes

dougbu merged commit 015dafc into aspnet:dev Mar 8, 2017

Conversation

Elfocrash commented Feb 22, 2017

Uh oh!

dnfclas commented Feb 22, 2017

Uh oh!

Eilon commented Feb 28, 2017

Uh oh!

Elfocrash commented Feb 28, 2017

Uh oh!

Eilon commented Mar 4, 2017

Uh oh!

Elfocrash commented Mar 4, 2017

Uh oh!

Eilon commented Mar 5, 2017

Uh oh!

Elfocrash commented Mar 6, 2017

Uh oh!

Eilon commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon commented Mar 7, 2017

Uh oh!

Elfocrash commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon commented Mar 8, 2017

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elfocrash commented Mar 8, 2017

Uh oh!

Elfocrash commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu commented Mar 8, 2017

Elfocrash commented Mar 7, 2017 •

edited

Loading

Elfocrash commented Mar 8, 2017 •

edited

Loading