Review: Needs Fixing
Hello,
Thanks for the proposal ! I do have comments, I will try to explain them
clearly.
**Coding style**
Coding style should be improved. The first example concerns function
declarations that are not coherent through the proposal. Example :
- def _case_open_notification(self, case, context=None):
- def _case_reset_notification(self, cr, uid, ids, context=None):
Why providing different call signatures for similar functions ? It leads to
having void functions declared in crm_base that are not overwriten in
inheriting classes because you do not have the same arguments. To be coherent
with OpenERP coding style, please use the second one that is standard.
I have another example of confusing arguments :
- _case_pending_notification(self, case, context=None)
This method is called further in the code with
"self._case_pending_notification(cases, context=context)"; this function is
designed for 1 case, and you give it several cases.
- def _case_cancel_notification(self, phonecall, context=None):
message = _("Phonecall is <b>cancelled</b>.")
phonecall[0].message_append_note( _('System notification'), ..."
You get "phonecall" (= 1 phonecall record, I assume) as argument, and you call
message_append_note on "phonecall[0]". The meaning is quite upset.
Also, please be concise when it helps the reading. Example :
- if obj.type=="opportunity":
message = _("Opportunity is <b>created</b>.")
elif obj.type=="lead" :
message = _("Lead is <b>created</b>.")
else:
message = _("Case is <b>created</b>.")
could be: message = _("%s has been *created*.") % ('Opportunity' if obj.type
== 'opportunity' else 'Lead'). As type has only two values and lead bu default,
will the 'Case' type be used ?
**Language**
We should use, I think, present perfect simple with passive form: "object has
been created" instead of "object is created", because the action is
instantaneous compared to future readings.
**crm_base class**
- _case_opportunity_meeting_notification: seems to be a method beloging to
crm.meeting, not crm.base
- _case_escalate_notification: should be defined in crm_case because
case_escalate is also defined in crm_case
Also update your function declarations as specified above
You also have void function such as _case_cancel_notification. As in other
objects messages are quite generic when dealing with usual actions, why not
having a default message such as "%s has been created" % self._description ?
For meetings or phonecalls this could lead to removing a lot of similar code.
This is a proposition, I do not have all crm_case heriting objects in mind.
**crm.meeting**
- when creating a meeting from opportunity, the opensocial widget should not
appear in the wizard form view
- why is there lines about phonecalls in meetings
(_case_opportunity_meeting_notification) ?
**Documentation**
This point is totally new :) . We are struggling to improve two main aspects of
our work: (technical) documentation and merge process.
Concerning documentation, each addon will probably have its own 'doc' directory
with .rst files containing its documentation. In order to ease the merge
process, specifications should be provided with the merge proposal as a
documentation.
To begin with this process, please add in crm a doc directory, with a .rst file
containing the specifications of what have been done. As everything is still
not specified, you can write the specifications in a opensocial.rst file for
example, that can contain the specifications coming form the pad, with some
explanations on the main modifications in the object model or behavior.
Best regards,
Thibault.
--
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-social-crm-bth/+merge/96754
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-addons/trunk-social-tde.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
