> 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): I have
> used _case_casename_notificationname(self, cr, uid, ids, context=None)
> everywhere.
> 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.
> I have modified code for this i have used for loop.
> 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 ?
> I have modified this statement wherever required and by default is lead.
> **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.
> I have changed language as you said for present perfect simple with passive
> form. just not changed for pending status.
> **crm_base class**
> - _case_opportunity_meeting_notification: seems to be a method beloging to
> crm.meeting, not crm.base
I have removed this method from crm_base.
> - _case_escalate_notification: should be defined in crm_case because
> case_escalate is also defined in crm_case
I have replaced this method from crm_base to 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) ?
> I have renamed this method. this method checked meeting is created from
> opportunity, phonecall or neither opportunity nor phonecall.
> **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
