1214509 - Template interpolate method should have data argument with a default value

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Stéphane Roucheray]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150929144111

Steps to reproduce:

When a template string has no property to interpolate[1] you need to pass an empty object as the data parameter to prepare it[2]

[1] https://github.com/mozilla-b2g/gaia/pull/31920/files#diff-56289627d91906a9c3b0f834ef19f514R533
[2] https://github.com/mozilla-b2g/gaia/pull/31920/files#diff-33b9f27b0cd1246a89a6096fa1769c95R1722



Expected results:

The Template.prototype.interpolate method should have a default value for the data argument:
> Template.prototype.interpolate = function(data = {}, options) {

Comment 1

[Stéphane Roucheray]

We could also take advantage of this bug to have a default value to the options parameter (instead of testing and initializing it later) as commented in the code[1]

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/template.js#L117

Comment 2

[Stéphane Roucheray]

I realized this morning that an error was thrown when calling Template.prototype.prepare or Template.prototype.interpolate without data parameter only because there still was a property (I forgot to delete) to interpolate inside the template string.

Julien, I updated the pull request of bug https://bugzilla.mozilla.org/show_bug.cgi?id=1037620 with the template string property deleted so it has been merged this morning to master with the right template. But I have now to delete the empty object passed to the prepare method in conversation.js[1]

Do you want me to make this change attached to this bug or mark this bug as Invalid and open another one.


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/conversation/js/conversation.js#L1722

Comment 3

[Julien Wajsberg [:julienw]]

Ah yeah I understand.

I think we can bugmorph this bug by:

* still using default arguments for "data" and "options" as you suggested
* in the callback in [1], add something like `if (!(property in data)) { throw new Error('something sensible'); }`

[1] https://github.com/mozilla-b2g/gaia/blob/6905956c041a42107aaa8b503baaf83c9a599591/shared/js/template.js#L125

What do you think ?

Comment 4

[Julien Wajsberg [:julienw]]

(or get `data[property]` value and check if it's === undefined)

Comment 5

[Stéphane Roucheray]

> I think we can bugmorph this bug by:
I am learning Mozilla's lingo ;)
 
> * still using default arguments for "data" and "options" as you suggested
> * in the callback in [1], add something like `if (!(property in data)) {
> throw new Error('something sensible'); }`
Nice idea ! It would help avoiding the kind of mistake I did.

I take it.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

Comment 7

[Julien Wajsberg [:julienw]]

Just a notice that because we'll be throwing, I'd like to avoid landing this before our soonish branching (November 2nd). I'm afraid to break other apps :)

So you can finish the work, ask review, etc, but not ask checkin-needed before we branch.

Hope this sounds clear :)

Comment 8

[Stéphane Roucheray]

Totally clear, I have seen the mails about priority on blockers and target release. So no problem, especially I think I have to add some other tests in there.

However I cannot ask for review here, I don't have the options in the details view of the attachment. Maybe that's because I am not the assignee ? Any idea ?

Comment 9

[Julien Wajsberg [:julienw]]

Maybe it is; you can try now.

Comment 10

[Stéphane Roucheray]

That's it ! Thanks.(In reply to Julien Wajsberg [:julienw] from comment #9)
> Maybe it is; you can try now.

That was it ! Thanks !

Comment 11

[Stéphane Roucheray]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

Remove review tag because there are some failed test.

Comment 12

[Stéphane Roucheray]

Throwing generate errors in unit tests, still don't know how many but potentially it could be a lot.
I need to crawl and fix all them before asking for a review.

It is to mention that an error can be thrown if a value is not passed at all for a template identifier (this is the case we want to fix) but of course also if an undefined value is passed (intentionally or not). I suppose user data is passed to some templates so throwing could be problematic in some cases on the field that are not covered by unit tests. I am still wondering if it wouldn't be better to warn in the console instead of throwing here.

An alternative would be just to test 
> property in data
it would handle the missing property case but would still allow to have undefined user data.

Julien what do you think ?

Comment 13

[Julien Wajsberg [:julienw]]

I still think it's ok to throw :) But after the branching (happens November 2nd or 3rd) so that we don't break anything in v2.5 ;)

I think an "undefined" value is never expected. "null" I'm not sure, I'd say we could log a warning for "null" values, but it's very rarely useful either. If the user wants to ignore the placeholder, he could just use an empty string.

Actually I'm even thinking of testing if the value is a string :) Too strict ?

Comment 14

[Julien Wajsberg [:julienw]]

Note: next time, please put a "need info" flag if you need information from me. I always read all my bugmail, but at least needinfo flags are more visible, so I get to it faster and I never forget about it :)

Comment 15

[Stéphane Roucheray]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> I still think it's ok to throw :) But after the branching (happens November
> 2nd or 3rd) so that we don't break anything in v2.5 ;)
> 
> I think an "undefined" value is never expected. "null" I'm not sure, I'd say

There are cases[1] 

> we could log a warning for "null" values, but it's very rarely useful
> either. If the user wants to ignore the placeholder, he could just use an
> empty string.
> 
> Actually I'm even thinking of testing if the value is a string :) Too strict ?

This is stricter, without a doubt :) It would require to fix expected behaviour of some tests has in the previous example[2]

If you think it is worth it, I can go for this.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/js/utils.js#L269-L270
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/test/unit/utils_test.js#L572-L581

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Stéphane Roucheray from comment #15)
> (In reply to Julien Wajsberg [:julienw] from comment #13)
> > I still think it's ok to throw :) But after the branching (happens November
> > 2nd or 3rd) so that we don't break anything in v2.5 ;)
> > 
> > I think an "undefined" value is never expected. "null" I'm not sure, I'd say
> 
> There are cases[1] 

I think it's wrong, we should use the empty string instead of null. In this case, if I'm not wrong, it's used in [3], so the string 'null' is likely hidden, but I'm sure it's in the markup ;)
 
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/js/shared_components.js#L24-L30

> 
> > we could log a warning for "null" values, but it's very rarely useful
> > either. If the user wants to ignore the placeholder, he could just use an
> > empty string.
> > 
> > Actually I'm even thinking of testing if the value is a string :) Too strict ?
> 
> This is stricter, without a doubt :) It would require to fix expected
> behaviour of some tests has in the previous example[2]

sure !

> 
> If you think it is worth it, I can go for this.

Yeah I think it's worth it :)
If you can check carefully where it's used (not only in SMS) it would be a good idea as well.

Comment 17

[Stéphane Roucheray]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

Hi Julien I did a second push. Can you review it please.
Template seems to be used in CLOCK and TV_APPS apps in addition to SMS app. All the tests passed after my last commit[1] so I infer everything is OK.

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=bdaf89477fe07a91e49d20f3b4682de527ee2fb2

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

I left a few comments on github :)
please ask review again when you're ready !

sorry again for the delay...

Comment 19

[Stéphane Roucheray]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

I fixed the requested nit. Can you give a look please !

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

This seems good to me. r=me. I left a nit if you want to fix it but this is not mandatory.

However I see this is used in tv_apps/smart-system and I'd like someone from the TV team to check whether this is working properly there. Please don't request a check-in before this check is done :)

Hey Evelyn, can you find somebody to check the various uses of the Template library in smart-system and validate this still works properly ? Especially the library now throws early in case something does not look correct, instead of ignoring silently.

Comment 21

[Evelyn Hung]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

Sean, I know it's a new topic to you, but could you check our usage in Smart-system and make sure the change here won't break TV? Thanks.

Comment 22

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8673827 [details] [review]
[gaia] sroucheray:1214509-template-interpolate > mozilla-b2g:master

Hi Julien,

There are 4 files in Smart-System to use Template.interpolate method, and the cases in Smart-System won't be affected by default value changes of Template.interpolate method.

Here is the tested result after applying the patch.
AFAIK, there are no IME support ([1] and [2]) in Smart System, so I did not test [1] and [2](showIMEList).
The result of [3] and [4] is not affected by the patch.

So the patch is given f+ by me.

Thanks. :)

[1] https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/ime_menu.js#L38
    https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/ime_menu.js#L91
[2] https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/app_install_manager.js#L502
[3] https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/permission_manager.js#L566
[4] https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/value_selector/value_selector.js#L154

Comment 23

[Julien Wajsberg [:julienw]]

Great !

Stéphane, now you need to add "r=julien" on the first line of the commit log, and add "checkin-needed" on the bug once you're ready.

Thanks again for your work !

Comment 24

[Stéphane Roucheray]

Sorry for the delay I had missed the mail !

Comment 25

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/59654b7eb48b5cd05c7cfb83ab3f042ea32072ab