Code review comment for lp:~hirt/ocb-addons/6.1_base_contact_finalize

Etienne Hirt (hirt) wrote :

Dear Omar,

thanks for the review and your valuable comments. Please check my
answers below.

Best Regards

Etienne

On 20.11.2013 01:19, Omar (Pexego) wrote:
> Review: Needs Fixing code review, no test
>
> At first sight, you should remove pdb import and the commented lines, on the other hand,
mostly done
> you convert the partner_id field, from related to function field without search function, this field could be used in several addons as filter, with function field will not work anymore at least without search function.
With the related field you have no real control what partner is selected
as main partner.
Originally I wanted to have conditional storage but this somehow did not
work. I removed it then because contacts are nowhere searched for
partners as far as I know. Also IMHO the related fields can not be
searched if not stored.
What do you propose?
> IMHO I prefer name_search as works now, because your way don't support, for example, with compound first names or more than one last name, this is applyable to view_init function too.
Please check again the proposed search function. Because it's intention
is to extend the original function instead of limit it. Should we also
consider one lastname with multiple/compound first names?

Without the view_init function all newly entered contacts are filled
into lastname only. Thus the proposed function is an extension but not
perfect as it can not guess that multiple Firstnames are used.
>
> It's a huge diff as Pedro said and I think that isn't the best way to complete this addon but we can wait for opinion of other reviewers, for now, I will mark it as needs fixing, for the up things.
>
> Regards

« Back to merge proposal