#14163 closed defect (fixed)
StatefulModel does not handle falsy values
Reported by: | ben hockey | Owned by: | rahul |
---|---|---|---|
Priority: | high | Milestone: | 1.7.1 |
Component: | DojoX MVC | Version: | 1.7.0 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
this simple case shows how StatefuleModel? changes null values to empty strings. the null should be retained.
var d = {prop: null}; var m = new dojox.mvc.StatefulModel({data: d}); console.log(d.prop, m.toPlainObject().prop);
fyi - this is in the 1.7.0 release.
Attachments (2)
Change History (17)
comment:1 Changed 9 years ago by
Version: | 1.7.0b1 → 1.7.0 |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
a possible fix is this (UPDATED):
-
StatefulModel.js
370 370 // the data structure. 371 371 // tags: 372 372 // private 373 var data = (args && args.data) || this.data; 374 if(data){ 375 this._createModel(data); 376 } 373 var data = (args && "data" in args) ? args.data : this.data; 374 this._createModel(data); 377 375 }, 378 376 379 377 //////////////////////// PRIVATE METHODS //////////////////////// … … 385 383 // The input for the model, as a plain JavaScript object. 386 384 // tags: 387 385 // private 388 if(lang.isObject(obj) && !(obj instanceof Date) && !(obj instanceof RegExp) ){386 if(lang.isObject(obj) && !(obj instanceof Date) && !(obj instanceof RegExp) && obj !== null){ 389 387 for(var x in obj){ 390 388 var newProp = new StatefulModel({ data : obj[x] }); 391 389 this.set(x, newProp);
i would have added some tests but wasn't sure where to put them
comment:4 Changed 9 years ago by
Summary: | StatefulModel does not handle null values → StatefulModel does not handle falsy values |
---|
Changed 9 years ago by
Attachment: | mvc-14163.patch added |
---|
comment:7 Changed 9 years ago by
Ben, Thanks for finding the problem and providing the fix. I just added the test into the date test where I had the test for the regexp. I also made an adjustment to the order that the robot tests were run, because this order seems to work more consistently for me.
comment:8 Changed 9 years ago by
ed, thanks for the patch.
i added one more test - for an empty string. since this case was not failing i had not mentioned it but i think it should be included in the tests so that the tests will reveal unexpected regressions.
once you can confirm that you're happy with the patch, i can commit it if you like.
comment:9 Changed 9 years ago by
hmm... just noticed a typo in my diff
doh.is("", test5_model.f.get('value'),"test5_model.s.get(value) should be an empty string");
should be
// was testing test5_model.f before!!! doh.is("", test5_model.s.get('value'),"test5_model.s.get(value) should be an empty string");
the tests passed without any error so we might be getting false positives from doh so i'm going to look into changing the tests. i'll update my diff once i've changed the tests.
comment:10 Changed 9 years ago by
ok, http://dojotoolkit.org/reference-guide/util/doh.html#doh-assertequal-obj1-obj2 says that doh.assertEqual (doh.is is an alias for doh.assertEqual) uses a fairly loose equality. i've updated the tests to use stricter equality.
comment:11 Changed 9 years ago by
Hi Ben,
Your update looks good, it is also good to know about the loose equality for assertEqual, I will have to remember that. If you could commit it that would be great.
comment:13 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|
comment:15 Changed 9 years ago by
Milestone: | 1.8 → 1.7.1 |
---|
the same problem occurs for each of the properties with the following data: