Issue 195047: Code after refactoring
DescriptionUpdated the code according to recommendations.
Please review
Patch Set 1 #
Total comments: 30
Patch Set 2 : added company and changed lead to open lead #Patch Set 3 : Changes from review #Patch Set 4 : Changed some missing reviews #Patch Set 5 : updated version no too #
Total comments: 24
Patch Set 6 : refractoring from review #
Total comments: 5
Patch Set 7 : Fixed email id #Patch Set 8 : changed open lead to lead #
MessagesTotal messages: 17
Hi, I had to make a new issue with the refactored files. Here we go... Please review the code once more..
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1/6 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/1/6#newcode5 __tryton__.py:5: 'version': '1.0.0', We use 0.0.1 for development module http://codereview.appspot.com/195047/diff/1/6#newcode19 __tryton__.py:19: 'opportunity_view.xml', Should be opportunity.xml http://codereview.appspot.com/195047/diff/1/7 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1/7#newcode8 opportunity.py:8: STATES = [('lead', 'Open Lead'), double space after = http://codereview.appspot.com/195047/diff/1/7#newcode24 opportunity.py:24: },depends=['state']) You remove 8cols of indentation to be aligned with states. Like http://hg.tryton.org/hgwebdir.cgi/modules/account_invoice/file/cf009c1eeb55/i... http://codereview.appspot.com/195047/diff/1/7#newcode41 opportunity.py:41: address = fields.Many2One('party.address', 'Address', domain=[('party', '=', Eval('party'))], still 80cols http://codereview.appspot.com/195047/diff/1/7#newcode78 opportunity.py:78: def end_lead(self, cursor, user, opp_id, context=None): Better to use ids :param ids: a list of opportunity ids or an opportunity id http://codereview.appspot.com/195047/diff/1/7#newcode87 opportunity.py:87: 'end_date':datetime.datetime.today(), indent 4cols http://codereview.appspot.com/195047/diff/1/7#newcode102 opportunity.py:102: quantity = fields.Float('Quantity') Add an empty line http://codereview.appspot.com/195047/diff/1/7#newcode136 opportunity.py:136: FROM """ + opportunity_object._table + """__history Quote table name http://codereview.appspot.com/195047/diff/1/8 File opportunity_view.xml (right): http://codereview.appspot.com/195047/diff/1/8#newcode8 opportunity_view.xml:8: <![CDATA[ Indent CDATA as other tags http://codereview.appspot.com/195047/diff/1/8#newcode188 opportunity_view.xml:188: Too much empty lines
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1/6 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/1/6#newcode4 __tryton__.py:4: 'name': 'Sales Leads and Opportunities', for me: Sale Opportunity http://codereview.appspot.com/195047/diff/1/6#newcode11 __tryton__.py:11: - Opportunities Perhaps: Sales extension - Create opportunities from leads http://codereview.appspot.com/195047/diff/1/6#newcode19 __tryton__.py:19: 'opportunity_view.xml', just opportunity.xml http://codereview.appspot.com/195047/diff/1/7 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1/7#newcode13 opportunity.py:13: ] I am still not quite sure about states: according to http://en.wikipedia.org/wiki/Sales_lead a followup state would be prospect (or even several prospects http://en.wikipedia.org/wiki/Sales_tunnel). So to remain in your design it would be: lead prospect finished (converted) cancel disqualified (lost) Or to stay with common generic tryton patterns: draft open sold cancel closed Different levels of prospect would be handled by Probability on the resp. 2nd state. http://codereview.appspot.com/195047/diff/1/7#newcode15 opportunity.py:15: 'Sale Leads & opportunities' Like we discussed in chat, would be for me just: 'Sale Opportunity'
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1/6 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/1/6#newcode4 __tryton__.py:4: 'name': 'Sales Leads and Opportunities', Changed to Sales Opportunities Management On 2010/01/26 20:39:16, yangoon wrote: > for me: > Sale Opportunity http://codereview.appspot.com/195047/diff/1/6#newcode5 __tryton__.py:5: 'version': '1.0.0', On 2010/01/26 20:12:22, ced wrote: > We use 0.0.1 for development module Done. http://codereview.appspot.com/195047/diff/1/6#newcode11 __tryton__.py:11: - Opportunities On 2010/01/26 20:39:16, yangoon wrote: > Perhaps: > Sales extension > - Create opportunities from leads > Done. http://codereview.appspot.com/195047/diff/1/6#newcode19 __tryton__.py:19: 'opportunity_view.xml', On 2010/01/26 20:12:22, ced wrote: > Should be opportunity.xml Done. http://codereview.appspot.com/195047/diff/1/6#newcode19 __tryton__.py:19: 'opportunity_view.xml', On 2010/01/26 20:39:16, yangoon wrote: > just opportunity.xml Done. http://codereview.appspot.com/195047/diff/1/7 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1/7#newcode8 opportunity.py:8: STATES = [('lead', 'Open Lead'), On 2010/01/26 20:12:22, ced wrote: > double space after = Done. http://codereview.appspot.com/195047/diff/1/7#newcode15 opportunity.py:15: 'Sale Leads & opportunities' On 2010/01/26 20:39:16, yangoon wrote: > Like we discussed in chat, would be for me just: > 'Sale Opportunity' Done. http://codereview.appspot.com/195047/diff/1/7#newcode24 opportunity.py:24: },depends=['state']) On 2010/01/26 20:12:22, ced wrote: > You remove 8cols of indentation to be aligned with states. > Like > http://hg.tryton.org/hgwebdir.cgi/modules/account_invoice/file/cf009c1eeb55/i... Done. http://codereview.appspot.com/195047/diff/1/7#newcode41 opportunity.py:41: address = fields.Many2One('party.address', 'Address', domain=[('party', '=', Eval('party'))], On 2010/01/26 20:12:22, ced wrote: > still 80cols Done. http://codereview.appspot.com/195047/diff/1/7#newcode78 opportunity.py:78: def end_lead(self, cursor, user, opp_id, context=None): But the workflow passes an ID as argument On 2010/01/26 20:12:22, ced wrote: > Better to use ids > :param ids: a list of opportunity ids or an opportunity id http://codereview.appspot.com/195047/diff/1/7#newcode102 opportunity.py:102: quantity = fields.Float('Quantity') On 2010/01/26 20:12:22, ced wrote: > Add an empty line Done. http://codereview.appspot.com/195047/diff/1/7#newcode136 opportunity.py:136: FROM """ + opportunity_object._table + """__history On 2010/01/26 20:12:22, ced wrote: > Quote table name Done. http://codereview.appspot.com/195047/diff/1/8 File opportunity_view.xml (right): http://codereview.appspot.com/195047/diff/1/8#newcode8 opportunity_view.xml:8: <![CDATA[ On 2010/01/26 20:12:22, ced wrote: > Indent CDATA as other tags Done. http://codereview.appspot.com/195047/diff/1/8#newcode188 opportunity_view.xml:188: On 2010/01/26 20:12:22, ced wrote: > Too much empty lines Done.
Sign in to reply to this message.
It misses ir.rule for the sale.opportunity* by company http://codereview.appspot.com/195047/diff/1019/1021 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1019/1021#newcode24 opportunity.py:24: }, depends=['state']) this is still not like http://hg.tryton.org/hgwebdir.cgi/modules/account_invoice/file/cf009c1eeb55/i... http://codereview.appspot.com/195047/diff/1019/1021#newcode48 opportunity.py:48: employee = fields.Many2One('company.employee','Employee',required=True, space after , http://codereview.appspot.com/195047/diff/1019/1021#newcode51 opportunity.py:51: },domain=[('company', '=', Eval('company'))]) space after , http://codereview.appspot.com/195047/diff/1019/1021#newcode51 opportunity.py:51: },domain=[('company', '=', Eval('company'))]) depends on company http://codereview.appspot.com/195047/diff/1019/1021#newcode57 opportunity.py:57: }) depends on party http://codereview.appspot.com/195047/diff/1019/1021#newcode79 opportunity.py:79: ('check_percentage', 'CHECK(probability >= 0 AND probability <= 100)', 'Probability must be between 0 and 100!') 80cols http://codereview.appspot.com/195047/diff/1019/1021#newcode126 opportunity.py:126: }, context=context) wrong indentation http://codereview.appspot.com/195047/diff/1019/1021#newcode174 opportunity.py:174: 'FROM "' + opportunity_object._table + '__history"' you could use the form: FROM "%s_history""" % opportunity_obj._table http://codereview.appspot.com/195047/diff/1019/1021#newcode178 opportunity.py:178: empty ending line http://codereview.appspot.com/195047/diff/1019/1022 File opportunity.xml (right): http://codereview.appspot.com/195047/diff/1019/1022#newcode45 opportunity.xml:45: </notebook> Generally we put the state against the buttons. http://codereview.appspot.com/195047/diff/1019/1022#newcode91 opportunity.xml:91: </record> act_window still misses ir.action.act_window.view http://codereview.appspot.com/195047/diff/1019/1022#newcode93 opportunity.xml:93: <menuitem parent="menu_sale_leadsnopp" action="act_sale_opp_form" don't abbreviate id http://codereview.appspot.com/195047/diff/1019/1022#newcode94 opportunity.xml:94: id="menu_sale_opp_form" icon="tryton-list" /> No need to define icon once you have views on action.
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1019/1021 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1019/1021#newcode21 opportunity.py:21: title = fields.Char('Title', required=True, translate=True, select=1, in many modules we have a 'name' field that which is used like a title. i think it would be more tryton like to name this field 'name'. in this case the _rec_name variable is not needed to be defined. http://codereview.appspot.com/195047/diff/1019/1021#newcode178 opportunity.py:178: On 2010/01/26 22:09:52, ced wrote: > empty ending line I think the empty ending line is good because you have a message by mercurial that the ending line is missing when you do a hg diff.
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1019/1021 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1019/1021#newcode178 opportunity.py:178: On 2010/01/26 22:45:44, timitos wrote: > On 2010/01/26 22:09:52, ced wrote: > > empty ending line > > I think the empty ending line is good because you have a message by mercurial > that the ending line is missing when you do a hg diff. Fix your diff program.
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/1019/1021 File opportunity.py (right): http://codereview.appspot.com/195047/diff/1019/1021#newcode24 opportunity.py:24: }, depends=['state']) On 2010/01/26 22:09:52, ced wrote: > this is still not like > http://hg.tryton.org/hgwebdir.cgi/modules/account_invoice/file/cf009c1eeb55/i... Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode48 opportunity.py:48: employee = fields.Many2One('company.employee','Employee',required=True, On 2010/01/26 22:09:52, ced wrote: > space after , Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode51 opportunity.py:51: },domain=[('company', '=', Eval('company'))]) On 2010/01/26 22:09:52, ced wrote: > space after , Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode57 opportunity.py:57: }) already there? On 2010/01/26 22:09:52, ced wrote: > depends on party http://codereview.appspot.com/195047/diff/1019/1021#newcode79 opportunity.py:79: ('check_percentage', 'CHECK(probability >= 0 AND probability <= 100)', 'Probability must be between 0 and 100!') On 2010/01/26 22:09:52, ced wrote: > 80cols Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode126 opportunity.py:126: }, context=context) On 2010/01/26 22:09:52, ced wrote: > wrong indentation Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode174 opportunity.py:174: 'FROM "' + opportunity_object._table + '__history"' On 2010/01/26 22:09:52, ced wrote: > you could use the form: > > FROM "%s_history""" % opportunity_obj._table Done. http://codereview.appspot.com/195047/diff/1019/1021#newcode178 opportunity.py:178: Yes, theres a message by mercurial On 2010/01/26 22:09:52, ced wrote: > empty ending line Done.
Sign in to reply to this message.
On 2010/01/26 22:50:45, ced wrote: > http://codereview.appspot.com/195047/diff/1019/1021 > File opportunity.py (right): > > http://codereview.appspot.com/195047/diff/1019/1021#newcode178 > opportunity.py:178: > On 2010/01/26 22:45:44, timitos wrote: > > On 2010/01/26 22:09:52, ced wrote: > > > empty ending line > > > > I think the empty ending line is good because you have a message by mercurial > > that the ending line is missing when you do a hg diff. > > Fix your diff program. yes sir.
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/29/30 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/29/30#newcode7 __tryton__.py:7: 'email': 'info@penlabs.co.in', changing your business? o missing
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/29/30 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/29/30#newcode7 __tryton__.py:7: 'email': 'info@penlabs.co.in', On 2010/01/27 10:53:35, udo.spallek wrote: > changing your business? o missing Done.
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/29/30 File __tryton__.py (right): http://codereview.appspot.com/195047/diff/29/30#newcode7 __tryton__.py:7: 'email': 'info@penlabs.co.in', ? email really ok Had to interrupt, already solved. http://codereview.appspot.com/195047/diff/29/31 File opportunity.py (right): http://codereview.appspot.com/195047/diff/29/31#newcode8 opportunity.py:8: STATES = [('lead', 'Open Lead'), why not just 'Lead'?
Sign in to reply to this message.
http://codereview.appspot.com/195047/diff/29/31 File opportunity.py (right): http://codereview.appspot.com/195047/diff/29/31#newcode8 opportunity.py:8: STATES = [('lead', 'Open Lead'), On 2010/01/27 11:30:15, yangoon wrote: > why not just 'Lead'? Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
