openerp-dev-web team mailing list archive

["Rucha \(Open ERP\)" <rpa@xxxxxxxxxxx>]

Review: Needs Fixing
its working nice as per functionality but some improvements are needed, here they are:

1) We need only one selection box in this wizard so remove the text box for Products

2) Need to put good help for this wizard (left side below the image) saying what this wizard will do

3) def trnslate_create
   is this a typo? 

4)  def trnslate_create
    if res_id == False :
       res_id = 0
    no need to use this code

5) def trnslate_create
    instead of removing already existing translation and creating new translation again for the same, can't we update the existing one?

6) def execute
    if context is None:
        context = {}
    this can be removed as context isn't used later in the code of the function

7) def execute
    Put all self.pool.get.. outside loop
    too many variables.. too many useless assignation(res_id = m_id), inconsistent mixtures of spaces and commas
    variable name ir_model for model ir_model_fields is misleading, improve variables
    variables used for result of search will be list of ids so it will be good if you put _ids instead of _id

8) def execute
    164 + if ir_menu_partner_id:
    165	+   for m_id in ir_menu_partner_id:
    ir_menu_partner_id is a list, and no indexing is done for this variable in code, so no need to use "if", you can directly use loop, it won't simply loop if the list is empty

9) def execute
    [('field_description','like','Product')]
    this will only replace term Product, we want to replace "product" , "Products", "products" too,
    use "ilike", also find these terms too: "Product Categories" , "Product Type" ...


10) def execute
    155	+ for p_id in ir_model_partner_id:
    156	+ brw_ir_model = ir_model.browse(cr ,uid ,p_id , context=context)
    157	+ name1 = brw_ir_model.field_description
    158	+ name2 = name1.replace('Partner',o.partner)
    159	+ obj2 = brw_ir_model.model_id.model
    160	+ field = brw_ir_model.name
    161	+ partner_name = obj2 +',' + field

    I don't understand why you replace old name with new name and then use it.. simply use it without any operations on it (as said in comment 7 remove useless variable assignations)

11) this wizard is working only once, if I run again its not working.


-- 
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-configuration-rework-product-terminlg-aag/+merge/61345
Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-addons/trunk-configuration-rework.