Description of problem: In the webadmin when you enter a non number input into a field that expects to have number, the validation message appears as "This field cannot be empty" instead of "Invalid number", this is confusing since if the user for some reason doesn't know that the field should be a number and enter something else he has no way to know what he did wrong Version-Release number of selected component (if applicable): How reproducible: 100% Steps to Reproduce: 1. In Webadmin go to the disks tab and click on "New" 2. In the size input enter an alphabetical string (e.g. "abc") 3. Click on OK Actual results: The size input is marked with red border and when hovering above the input a tooltip says "This field can't be empty" Expected results: The tooltip should say something like "Invalid number" Additional info:
Created attachment 964855 [details] screen-cast: text-box is being unexpectedly emptied
IntegerValidation.java seems to be OK, the problem may be with the widget itself, that wrongfully passes (sometimes) an empty value to the UI business logic, rather than the real value. I have noticed another strange behavior, which may be related - see attachment 964855 [details]. Steps to reproduce: 1. In Webadmin go to the disks tab and click on "New" 2. In the size input enter an alphabetical string (e.g. "abc") 3. Click on OK [hovering on the red-bordered 'size' text-box wrongfully displays "This field can't be empty", as reported in the BZ description] 4. Change the size input value to something valid (e.g. "1"). ** make sure to keep the 'Alias' field empty, so that clicking on "OK" will result in validation failure and in the dialog remaining open. ** 5. Click on OK ['size' text-box is not red-bordered anymore. dialog remains open due to validation failure of the 'Alias' field. 6. Change the size input value to something invalid again (e.g. "a") 7. Click on OK ** notice how the 'size' field is emptied ** it is also becoming red-bordered with a "This field can't be empty" tool-tip, which somewhat makes sense, as this field is indeed empty; the problem is that it is getting emptied ... [removed UserExperience keyword, no user-experience-design advice is needed here, this is a plain bug; a message in the form of "this field must contain an integer number" or similar should appear] Additional info: this happens also in the 'VLAN Tagging' field within the New Logical Network dialog, probably happening for other fields as well. However, this doesn't happen in all text fields; for example, in the "Total Virtual CPUs" field in the New VM dialog, there is a proper validation failure message when following the same steps to reproduce detailed in Comment #0 [see http://i.imgur.com/aimDCq5.png] - worth comparing for reference.
So the problem is the following: For the Disk Size field the editor is an IntegerEntityModelEditor. This editor attempts to parse the contents of the field and convert it to an Integer before passing it to the model. This obviously fails as the value is not an integer and it passes null to the model. The model runs its own validation and of the validators is not empty, which fails due to the editor setting the value of the model to null (It can't set an Integer value as it can't convert the value to an integer). This results in the weird error message. Now why does it work for the Total Virtual CPUs field? This field is configured to be a StringEntityModelTextBoxEditor. The field in the model is also a string. So the editor passes the value unchanged to the model, where the IntegerValidator fails the validation and sets the proper error message. So there are several options on how to proceed from here: 1. Change all the IntegerEntityModelEditors to String ones, and change the fields to be strings as well. Then allow the Model validators to fail the value of the field like before. This sacrifices type safety in the editor for better validation error messages by the models. 2. Change the model validation methods to not check for empty values, and assume that if the value is empty, that an invalid value has been entered and return an error message like 'the value must be a number'. Then this message will show up for both empty and invalid values. This will require a change to all models with IntegerEntityModelEditors. This is a somewhat ad hoc approach. 3. Modify the IntegerEntityModelEditor to simply not allow non integer values to be entered. That way we are sure that the conversion will work and only empty values will produce the empty message. 4. Attempt to hook our editors into the GWT validation framework. Then we can show the error message before we even get to the model validation. This should hopefully also allow for us to use JSR-303 annotations on the models to define the constraints of each value and should greatly simplify the validation routines on the models as we no longer have to check all the mundane things as the validator framework does it for us. This is of course a large under taking, but in the end will result in less code and much nicer validation routines. @Einav, After reading all that which approach do you want to take. IMO 4 is the best option in the long run, 1 very hokey but should be able to be done quickly, 2 is less hokey and should be able to be done quickly too, 3 is more or less putting a bandaid on a broken system.
Alexander, option 4 sounds really good, I have some concerns though, which may require a lot more work than maybe initially anticipated: after looking at [1]: it seems that with the JSR-303 annotations we will be able to do things such as: --- public class Person implements Serializable { @NotNull @Size(min = 4, message = "Name must be at least 4 characters long.") private String name; } --- I just want to verify that it would work with our uicommonweb models: In our models, we are not working directly with primitives and Strings and such - we are working mostly with EntityModels. Will we be able to e.g. set the "min" value of the "@Size" annotation from a parameter (that will be used to initialize the EntityModel or similar)? Will we be able to somehow annotate the EntityModel instances themselves, rather than the primitives/Strings/... in them? also, will we be able to localize and "parameterize" the "message" value? i.e. instead of something like: "Name must be at least 4 characters long." I would probably want something like constructMessage(ApplicationMessages.getNameMustBeAtLeastMin, Min). any thoughts on that? [1] http://www.gwtproject.org/doc/latest/DevGuideValidation.html
@Einav I am well aware that option 4 is a lot of work (maybe even not possible with our current model setup). We would have to devote some time and resources into doing research (maybe a PoC for one model or something), to see if it is at all possible. This will definitely take quite a bit of time to do the research. This is why I gave you some other less time consuming options to consider in case option 4 research is simply not possible due to resource constraints. So to answer all your questions in one answer, I don't know I will have to try and find out if I can integrate it into our project.
(In reply to Alexander Wels from comment #5) > @Einav > > I am well aware that option 4 is a lot of work (maybe even not possible with > our current model setup). We would have to devote some time and resources > into doing research (maybe a PoC for one model or something), to see if it > is at all possible. This will definitely take quite a bit of time to do the > research. > > This is why I gave you some other less time consuming options to consider in > case option 4 research is simply not possible due to resource constraints. > So to answer all your questions in one answer, I don't know I will have to > try and find out if I can integrate it into our project. thanks for the clarification, Alexander - I just needed a rough understanding of how much investigation/work will be needed in order to implement option 4. now that I have a better understanding, it doesn't sound like a 3.6 material to me. however, in order to solve this problem, I wouldn't want to wait for after 3.6. to my understanding, implementing proposed solution 2 will only resolve the problem described in comment #0, however it probably won't resolve the problem that I described in comment #2 (maybe it is not related at all and/or I'm missing something). So for a quick solution, I prefer to simply avoid using IntegerEntityModelEditors completely and resort to your proposed solution 1.
> thanks for the clarification, Alexander - I just needed a rough > understanding of how much investigation/work will be needed in order to > implement option 4. now that I have a better understanding, it doesn't sound > like a 3.6 material to me. > however, in order to solve this problem, I wouldn't want to wait for after > 3.6. > > to my understanding, implementing proposed solution 2 will only resolve the > problem described in comment #0, however it probably won't resolve the > problem that I described in comment #2 (maybe it is not related at all > and/or I'm missing something). > > So for a quick solution, I prefer to simply avoid using > IntegerEntityModelEditors completely and resort to your proposed solution 1. I would agree that if our plate is full already this would probably be too much of a stretch for 3.6. Proposed solution 2 may or may not solve the issue of disappearing values, I would have to try, I *think* the value disappears because of the conversion failure sending null to the model, and then the view gets populated from the model (which is null at the time). So only solution 1 should prevent that from happening. So if we want this in the 3.6 timeframe I would suggest going with either option 1 or 3, and you seem to prefer option 1 so I will implement that.
after discussing with Alexander: - I initially chose solution 1 since it "should be able to be done quickly", however it seems that we have *a lot* of these instances to fix, including potentially the LongEntityEditors and maybe event Float/Double entity editors. - solution 2 is out of the question, since it would result in values that are not consistent across the view/editor/model components, which is very prone to unexpected behaviors. - solution 3 is not bad, but it has ux implications; we will need to somehow hint the user that he can fill only numbers (we don't want the user hitting the 'a' keyboard key, getting frustrated with 'why the value is not being filled in the text box'?). so IMO this will require a styling change to these text-boxes (e.g. adding a gray placeholder text - see http://getbootstrap.com/css/#forms - "Enter email" for example, displaying a label/tooltip in case user is pressing on illegal characters in the keyboard, etc.). Changing all of the relevant text-boxes accordingly may take a while, and for consistency considerations, we'd might even want to change *all* of the text-boxes in the application, which may be even more work... moreover, I would want to perform overall styling changes only once we properly adopt PatternFly in the dialogs. this leaves us with solution 4 - solving this properly... Alexander will start with research / PoC and we will continue from there.
After doing the research the conclusion is that solution 4 will not solve the problem of entering aa into an integer box and having the model contain null for the value. The problem occurs in the editor parsing the value and being unable to convert it. I have managed to hook into the parsing exception and I will update the editor at parsing time to mark it as invalid at that point. This should prevent the strange behavior.
*** Bug 1182573 has been marked as a duplicate of this bug. ***
*** Bug 1199089 has been marked as a duplicate of this bug. ***
oVirt 3.6.0 has been released on November 4th, 2015 and should fix this issue. If problems still persist, please open a new BZ and reference this one.