Dear Holger,


It's an honor that you have looked at my code so deeply, glad to know 
you're liking it :)

About your questions:

*- your js tests will run twice, once from the web module, once triggered from tests/test_js.py. Is this possibly a leftover from development?**
*
I did this because i want to launch my tests on Travis (for web unleashed sources on github), I think we can keep them as a "fast_suite".
Do you see any disadvantages to keep web unit test runnable on server side too ?

*- any reason to deviate from putting qweb templates into static/src/xml?**
*
I was not aware of community rules when I started this module, I will change it to be compliant with OpenERP rules.

*- I understand the reasoning behind the name, but something more descriptive than web_unleashed would be better IMHO**
*
I named it like this because it's my feeling when I'm using it.
The original web module on OpenERP is, at least, obscure and using well known libraries instead give so much advantages... (I'm still hope that OpenERP SA will go on this way one day...)

*- core/unleashed.js should use translated strings where appropriate
*
I don't see where there's strings to translate in the core, can you give me more details on this point ?
*
- Could you elaborate where the context you pass to sync() comes from? From my understanding, it's passed every time you'd call fetch() on the model, but where does that come from?**
**UnleashedView gets it from the dataset (you shouldn't eval that then btw but a copy of it before passing it to the server in connector.js), which is fine, but I can't see where it's propagated further to the models? (I'm not a backbone expert if that's not obvious by now)*

sync() is used by Backbone Model as a interface to achieve CRUD methods.
This function is called by Backbone with specific parameters (crud method, model/collection, options), so the js context at sync execution is the Backbone Model.

I use the Connector as an interface between Backbone models and the JSON-RPC API. For example, when fetch() is called, it will go through sync > connector > read, and the domain (filter), group by (group), order by (order), selected fields and the OpenERP context can be set on the "options" object and will be processed by the Connector.

sync() function should maybe set a default OpenERP context... but the context is only available at the OpenERP view level, not when sync() function is defined by web unleashed.
I think it's up to the developer to pass the context to the Backbone model, and then passing it to CRUD methods...
However we can imagine to add to BaseModel/BaseCollection a context option, if they have it, the model will pass it to CRUD methods automatically.

>*you shouldn't eval that then btw but a copy of it before passing it to the server in connector.js*

OK, but actually, I use the context only to get the user id and the language...
Maybe you can explain me what kind of side effects I can have by evaluating it in my view, at init ?

 From my understanding, the context is used only for view configuration on client side (get user id/language, some custom parameters, etc...).
Further, even if I don't pass the context to the server, the uid will be correctly passed to python methods on server side (fortunately :)), so my OpenERP modules using web unleashed are not defining any context on their models, but maybe I'm wrong...


I will wait your answer before making appropriate changes on web-unleashed.

Also, we have done few changes on web_unleashed during the past weeks 
but maybe it's better if we can finish this merge proposal before I 
merge our changes and do another merge proposal :)


Best regards,
Michel


On 01/20/2014 09:59 PM, Holger Brunn (Therp) wrote:
> Looks pretty good! I'm yet to try it in a serious project, but for the time being: Well done!
>
> Some nits so far:
>
> - your js tests will run twice, once from the web module, once triggered from tests/test_js.py. Is this possibly a leftover from development?
>
> - any reason to deviate from putting qweb templates into static/src/xml?
>
> - I understand the reasoning behind the name, but something more descriptive than web_unleashed would be better IMHO
>
> - core/unleashed.js should use translated strings where appropriate
>
> Now a serious question: Could you elaborate where the context you pass to sync() comes from? From my understanding, it's passed every time you'd call fetch() on the model, but where does that come from? UnleashedView gets it from the dataset (you shouldn't eval that then btw but a copy of it before passing it to the server in connector.js), which is fine, but I can't see where it's propagated further to the models? (I'm not a backbone expert if that's not obvious by now)