openerp-community-reviewer team mailing list archive

[Michel Meyer <mmeyer@xxxxxxxxx>]

Hi Holger,


I did some changes on sources according to your comments, you can get 
them on lp:~trobz/web-addons/web-unleashed 
<https://code.launchpad.net/%7Etrobz/web-addons/web-unleashed> branch.

To resume my changes:
- 13 
<http://bazaar.launchpad.net/%7Etrobz/web-addons/web-unleashed/revision/13>. 
ensure a context can be passed on any JSON-RPC call made by backbone models
- 14 
<http://bazaar.launchpad.net/%7Etrobz/web-addons/web-unleashed/revision/14>. 
remove this.context on UnleashedView (accessible with 
this.dataset.get_context())
- 15 
<http://bazaar.launchpad.net/%7Etrobz/web-addons/web-unleashed/revision/15>. 
add a method to instantiate translated Error object
- 12 
<http://bazaar.launchpad.net/%7Etrobz/web-addons/web-unleashed/revision/12>. 
rename template folder into xml


*Context*

I was not aware of all the implication of the context, thanks for all 
these details ! It pushed me to look more deeply in OpenERP sources...
And after some research, this is what I understand about how OpenERP 
manage the context internally:

/# for read queries
/
instance.web.Query generate a/compoundContext /based on multiple contexts:
- the user context (instance.session.user_context)
- the instance.web.Model.context, if any
- additional context specified for this query, if any (= options.context 
on unleashed backbone models)
- re evaluate all these contexts together and generate the final one... 
(really, don't ask me to look deeper in pyeval, seems so odd...)


# for count queries

instance.web.Query generate a/compoundContext /based on:
- the user context (instance.session.user_context)
- the instance.web.Model.context, if any
- additional context specified for this query, if any (= options.context 
on unleashed backbone models)

This is done my calling this._model.context(this._context)}) at Query 
execution.


/# for other method (create, update, delete)/
/
- /indirectly, in instance.web.JsonRPC core object, the user context is 
set by default (if not context is passed as on options to call method)

It's done with:
_.defaults(params, {
     context: this.user_context || {}
});


I'm still not sure why OpenERP doesn't use the 
instance.web.Model.context() method in instance.web.Model.call(), but 
they may have a good reason to do so...

On unleashed code, at instance.web.Model instantiation in sync(), I pass 
the options.context as the second parameter, so Model has a default 
context used at execution for all type of query.

Finally, unleashed sync() function allow developers to pass a custom 
context to instance.web.Model object, and the rest (user context, 
etc...) is managed by OpenERP and I don't think there's extra work to do 
on this...


*Error Translation**
*
Ok, I was thinking throwing errors is not made for the final user, it's 
more for developers.

Nonetheless, I added a method on unleashed modules to instantiate a 
translated Error object and I changed sources to use it on all error thrown.

Note: in OpenERP sources, at core level, Error are not translated, so we 
may have a mix of translated/not translated error message...




It's a long email again, but I think the subject is interesting and 
unleashed is made to be a core component on other OpenERP modules, so 
going into details is important.



Thanks again for your review and all your comments !
Michel



On 01/27/2014 07:48 PM, Holger Brunn (Therp) wrote:
> Michel, thanks for your comments. I already doubted my programming skills when I couldn't find out where the context is propagated, good to hear it's not.
>
> On context: You definitely need to think of a way to pass the user's standard context (openerp.session.user_context) to your models. Otherwise, you lose stuff like the timezone (which causes all functions dealing with datetimes to assume UTC, and you really don't want that) and default values. That's what currently comes to my mind, there are also other things like the currently active project or the like.
>
> On evaluating context: It can contain code line time(), so if you eval it on view initialization, you'll always pass the same time.
>
> Translations: The messages in the Errors you throw are not translated to my understanding. Even though most of them look like they never should occur in a production environment, I still think it's good practice to translate them.
>
> Tests: I don't see real world disadvantages, so if you've reason to keep them there, keep them


-- 
https://code.launchpad.net/~trobz/web-addons/web-unleashed/+merge/195542
Your team Web-Addons Core Editors is requested to review the proposed merge of lp:~trobz/web-addons/web-unleashed into lp:web-addons.