Upload for sale opportunity review

[udono, 2010-01-26 13:38:38]

Looks good, some comments from me

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode4
sale_opportunity.py:4: from trytond.model import ModelView,
ModelSQL,ModelWorkflow, fields
ModelSQL, ModelWorkflow,

http://codereview.appspot.com/193111/diff/1/5#newcode5
sale_opportunity.py:5: from trytond.pyson import Equal, Eval, Not, PYSONEncoder,
Date, In
PYSONEncoder, Date not used

http://codereview.appspot.com/193111/diff/1/5#newcode32
sale_opportunity.py:32: party =
fields.Many2One('party.party','Party',required=True,select=1,domain=[('active','in',[True,False])])
less or equal 80 Columns

http://codereview.appspot.com/193111/diff/1/5#newcode41
sale_opportunity.py:41: )
help="Percentage between 0 and 100"

http://codereview.appspot.com/193111/diff/1/5#newcode45
sale_opportunity.py:45: },depends = ['state'])
Maybe a One2Many to a loss_reason table, to choose from?

http://codereview.appspot.com/193111/diff/1/5#newcode54
sale_opportunity.py:54: 'wrong_percentage': 'Probability should be between 0 &
100!',
/should/[needs to|must]/

http://codereview.appspot.com/193111/diff/1/5#newcode92
sale_opportunity.py:92:
product=fields.Many2One('product.product','Product',required=True)
product = fields

http://codereview.appspot.com/193111/diff/1/5#newcode125
sale_opportunity.py:125: """,[])
Did we need to use SQL to catch the _history table? Is there no way to get the
object and use search, browse etc?

[sharoonthomas, 2010-01-26 13:47:24]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode4
sale_opportunity.py:4: from trytond.model import ModelView,
ModelSQL,ModelWorkflow, fields
On 2010/01/26 13:38:39, udo.spallek wrote:
> ModelSQL, ModelWorkflow,

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode5
sale_opportunity.py:5: from trytond.pyson import Equal, Eval, Not, PYSONEncoder,
Date, In
On 2010/01/26 13:38:39, udo.spallek wrote:
> PYSONEncoder, Date not used

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode32
sale_opportunity.py:32: party =
fields.Many2One('party.party','Party',required=True,select=1,domain=[('active','in',[True,False])])
On 2010/01/26 13:38:39, udo.spallek wrote:
> less or equal 80 Columns

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode41
sale_opportunity.py:41: )
On 2010/01/26 13:38:39, udo.spallek wrote:
> help="Percentage between 0 and 100"

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode45
sale_opportunity.py:45: },depends = ['state'])
On 2010/01/26 13:38:39, udo.spallek wrote:
> Maybe a One2Many to a loss_reason table, to choose from?

IMHO that would be unnecessary, Its a multiline field, so append in such a case?

http://codereview.appspot.com/193111/diff/1/5#newcode54
sale_opportunity.py:54: 'wrong_percentage': 'Probability should be between 0 &
100!',
On 2010/01/26 13:38:39, udo.spallek wrote:
> /should/[needs to|must]/

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode92
sale_opportunity.py:92:
product=fields.Many2One('product.product','Product',required=True)
On 2010/01/26 13:38:39, udo.spallek wrote:
> product = fields

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode125
sale_opportunity.py:125: """,[])
I thought it was needed because we need to COALESCE user and date, otherwise its
not needed
On 2010/01/26 13:38:39, udo.spallek wrote:
> Did we need to use SQL to catch the _history table? Is there no way to get the
> object and use search, browse etc?

[bch, 2010-01-26 14:02:14]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode13
sale_opportunity.py:13: ]
Extra spaces after '['.
']' should be aligned with his brother.

And ',' must be followed by a blank space. If you use emacs, you can use this
command to spot them all:

"M-x occur RET ,\S "

(there is a whitespace after \S)

http://codereview.appspot.com/193111/diff/1/5#newcode20
sale_opportunity.py:20: #Fields
We avoid comments like that. There are not needed when the code is homogeneous
across all the modules.

http://codereview.appspot.com/193111/diff/1/5#newcode52
sale_opportunity.py:52: ]
An sql constraint would be quicker, see
http://hg.tryton.org/hgwebdir.cgi/modules/stock/file/b55d45337e81/inventory.p...
for an example

[ced, 2010-01-26 14:03:08]

Don't forget to put ir.model.access for sale.opportunity and
sale.opportunity.lines

http://codereview.appspot.com/193111/diff/1/3
File __tryton__.py (right):

http://codereview.appspot.com/193111/diff/1/3#newcode19
__tryton__.py:19: 'sale_opportunity_view.xml',
We don't separate xml like that.
One xml file for data related to one Model

http://codereview.appspot.com/193111/diff/1/4
File party.xml (right):

http://codereview.appspot.com/193111/diff/1/4#newcode7
party.xml:7: <field name="name">Leads/Opportunities</field>
"Sale Opportunities"?

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode8
sale_opportunity.py:8: STATES = [  ('lead','Open Lead'),
We use PEP8.
So don't indent to align list, just use 8 cols.

http://codereview.appspot.com/193111/diff/1/5#newcode8
sale_opportunity.py:8: STATES = [  ('lead','Open Lead'),
"Opened Lead" or just "Lead"

http://codereview.appspot.com/193111/diff/1/5#newcode20
sale_opportunity.py:20: #Fields
An empty line is enough to separate fields definition.

http://codereview.appspot.com/193111/diff/1/5#newcode21
sale_opportunity.py:21: title = fields.Char('Title', required=True,
translate=True,select=1)
Space before select=1

http://codereview.appspot.com/193111/diff/1/5#newcode23
sale_opportunity.py:23: date = fields.DateTime('Date',required=True,select=1,
depends on state

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
Put comma at the end of line like that if we add new states we don't need to
change this line and back-porting patch will be easier.

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
It could be: Not(Equal(Eval('state'), 'lead'))

http://codereview.appspot.com/193111/diff/1/5#newcode32
sale_opportunity.py:32: party =
fields.Many2One('party.party','Party',required=True,select=1,domain=[('active','in',[True,False])])
80 cols

http://codereview.appspot.com/193111/diff/1/5#newcode32
sale_opportunity.py:32: party =
fields.Many2One('party.party','Party',required=True,select=1,domain=[('active','in',[True,False])])
I think we must not allow to select inactive parties.
If there are logically deleted, it is because we don't want any more to use it.

http://codereview.appspot.com/193111/diff/1/5#newcode33
sale_opportunity.py:33: address =
fields.Many2One('party.address','Address',domain=[('party','=',Eval('party'))],select=2)
depends on party

http://codereview.appspot.com/193111/diff/1/5#newcode35
sale_opportunity.py:35: state =
fields.Selection(STATES,'Stage',required=True,select=1,sort=False,readonly=True)
Stage -> State

http://codereview.appspot.com/193111/diff/1/5#newcode40
sale_opportunity.py:40: },depends = ['state'],select=2
Not space around = in function params.

http://codereview.appspot.com/193111/diff/1/5#newcode42
sale_opportunity.py:42: change_history_lines =
fields.One2Many('sale.opportunity.historylookup','sale_opportunity','History')
Try to have the same names for field, model and string.

Maybe:
history_lines, sale.opportunity.history.lines, 'History Lines'

http://codereview.appspot.com/193111/diff/1/5#newcode45
sale_opportunity.py:45: },depends = ['state'])
Perhaps readonly and filled by a wizard when going to lost state.

http://codereview.appspot.com/193111/diff/1/5#newcode51
sale_opportunity.py:51: ('check_percentage', 'wrong_percentage')
It could be an _sql_constraints, it will be faster.

http://codereview.appspot.com/193111/diff/1/5#newcode54
sale_opportunity.py:54: 'wrong_percentage': 'Probability should be between 0 &
100!',
It don't know if it is good to use '&' char.

http://codereview.appspot.com/193111/diff/1/5#newcode63
sale_opportunity.py:63: if (probability <0) or (probability>100):
space around operators:

if (probability < 0) or (probability > 100):

http://codereview.appspot.com/193111/diff/1/5#newcode73
sale_opportunity.py:73: def end_lead(self, cursor, user, opp_id, context=None):
We try to always write function that works with list of ids.

http://codereview.appspot.com/193111/diff/1/5#newcode76
sale_opportunity.py:76: This is called from the workflow
Not necessary to tell from were it is called because it could be called from
other places and the comment will not be accurate.

http://codereview.appspot.com/193111/diff/1/5#newcode77
sale_opportunity.py:77: opp_id: id of the record to be flagged with the current
datetime
We use the syntax:

:param opp_id: ...

And describe all params.

http://codereview.appspot.com/193111/diff/1/5#newcode80
sale_opportunity.py:80:
self.write(cursor,user,opp_id,{'end_date':datetime.datetime.today()})
You must always pass the context

http://codereview.appspot.com/193111/diff/1/5#newcode87
sale_opportunity.py:87: _name = "sale.opportunity.lines"
name it "sale.opportunity.line" 
It define only one line.

http://codereview.appspot.com/193111/diff/1/5#newcode93
sale_opportunity.py:93: quantity = fields.Float('Quantity')
One empty line between class definition and instantiation.

http://codereview.appspot.com/193111/diff/1/5#newcode93
sale_opportunity.py:93: quantity = fields.Float('Quantity')
I think quantity must be required like in sale.line

http://codereview.appspot.com/193111/diff/1/5#newcode98
sale_opportunity.py:98: 'History of Sales oportunity'
typo: apportunity

http://codereview.appspot.com/193111/diff/1/5#newcode107
sale_opportunity.py:107: user = fields.Many2One('res.user','User/Empl')
Only User

http://codereview.appspot.com/193111/diff/1/5#newcode123
sale_opportunity.py:123: title, state, probability
One line per field, it is more readable.

http://codereview.appspot.com/193111/diff/1/5#newcode124
sale_opportunity.py:124: FROM sale_opportunity__history
Use self._table to be more generic.

http://codereview.appspot.com/193111/diff/1/5#newcode124
sale_opportunity.py:124: FROM sale_opportunity__history
Indent like in other modules:

SELECT
    field,
    field,
FROM "table"
WHERE ... = ..
    ... = ...
GROUP BY ...

http://codereview.appspot.com/193111/diff/1/6
File sale_opportunity_view.xml (right):

http://codereview.appspot.com/193111/diff/1/6#newcode8
sale_opportunity_view.xml:8: <![CDATA[
Indent CDATA tag with others.

http://codereview.appspot.com/193111/diff/1/6#newcode9
sale_opportunity_view.xml:9: <form string="Sales Lead/Opportunity">
string in form are always in singular.
"Sale Lead/Opportunity"

http://codereview.appspot.com/193111/diff/1/6#newcode31
sale_opportunity_view.xml:31: <field name="change_history_lines">
Define form and tree view for history outside.
And only inside other view when you want different view.

http://codereview.appspot.com/193111/diff/1/6#newcode43
sale_opportunity_view.xml:43: <field name="lines" colspan="4">
Better in notebook?

http://codereview.appspot.com/193111/diff/1/6#newcode76
sale_opportunity_view.xml:76: <tree string="CRM Lead/Opportunity">
string in tree view are always in plural.
"Sale Leads/Opportunities"

http://codereview.appspot.com/193111/diff/1/6#newcode87
sale_opportunity_view.xml:87: <record model="ir.action.act_window"
id="act_saleopportunity_form">
If it is possible to fit 80cols, do it.

http://codereview.appspot.com/193111/diff/1/6#newcode87
sale_opportunity_view.xml:87: <record model="ir.action.act_window"
id="act_saleopportunity_form">
id="act_sale_opportunity_form"

http://codereview.appspot.com/193111/diff/1/6#newcode88
sale_opportunity_view.xml:88: <field name="name">All Sales Lead and
Opportunity</field>
Same structure than for sale so just
"Sale Leads/Opportunities"

http://codereview.appspot.com/193111/diff/1/6#newcode91
sale_opportunity_view.xml:91: </record>
You must define also ir.action.act_window.view

http://codereview.appspot.com/193111/diff/1/6#newcode92
sale_opportunity_view.xml:92: <record model="ir.action.act_window"
id="act_saleopportunity_form_open">
id="act_sale_opportunity_form_lead"

http://codereview.appspot.com/193111/diff/1/6#newcode93
sale_opportunity_view.xml:93: <field name="name">Open Sales Leads</field>
Opened Leads

http://codereview.appspot.com/193111/diff/1/6#newcode99
sale_opportunity_view.xml:99: <field name="name">Sales Opportunities</field>
Simply: "Opportunities"

http://codereview.appspot.com/193111/diff/1/6#newcode105
sale_opportunity_view.xml:105: <field name="name">Converted Leads</field>
Converted Opportunities

http://codereview.appspot.com/193111/diff/1/6#newcode122
sale_opportunity_view.xml:122: <menuitem name="Leads and Opportunities"
id="menu_sale_leadsnopp" parent="sale.menu_sale"/>
Let the default name if it is good.

http://codereview.appspot.com/193111/diff/1/6#newcode123
sale_opportunity_view.xml:123: <menuitem parent="menu_sale_leadsnopp"
action="act_saleopportunity_form" id="menu_sale_opportunity_form"
icon="tryton-list"/>
Let the default icon if it is good.

http://codereview.appspot.com/193111/diff/1/7
File sale_opportunity_workflow.xml (right):

http://codereview.appspot.com/193111/diff/1/7#newcode4
sale_opportunity_workflow.xml:4: <record model="workflow"
id="sale_opportunity_workflow">
id="opportunity_workflow"
No need to prefix with sale_

http://codereview.appspot.com/193111/diff/1/7#newcode9
sale_opportunity_workflow.xml:9: <record model="workflow.activity"
id="sale_activity_lead">
id="opportunity_activity_lead"

http://codereview.appspot.com/193111/diff/1/7#newcode20
sale_opportunity_workflow.xml:20: <field name="action">write({'state':
'opportunity','probability':50})</field>
Put a default probability value on the Model

http://codereview.appspot.com/193111/diff/1/7#newcode26
sale_opportunity_workflow.xml:26: <field
name="action">end_lead()&#10;write({'state': 'converted'})</field>
Use only one function it is better to override.

http://codereview.appspot.com/193111/diff/1/7#newcode42
sale_opportunity_workflow.xml:42: <record model="workflow.transition"
id="saleopp_transition_lead_opportunity">
id="opportunity_transition_lead_opportunity"

[ced, 2010-01-26 14:06:25]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode45
sale_opportunity.py:45: },depends = ['state'])
On 2010/01/26 13:47:25, sharoonthomas wrote:
> On 2010/01/26 13:38:39, udo.spallek wrote:
> > Maybe a One2Many to a loss_reason table, to choose from?
> 
> IMHO that would be unnecessary, Its a multiline field, so append in such a
case?
> 

But it will standardize the reasons. And statistics could be done. Maybe, we
should have both a selection field + a comment field.

[udono, 2010-01-26 14:08:09]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode45
sale_opportunity.py:45: },depends = ['state'])
On 2010/01/26 13:47:25, sharoonthomas wrote:
> On 2010/01/26 13:38:39, udo.spallek wrote:
> > Maybe a One2Many to a loss_reason table, to choose from?
> IMHO that would be unnecessary, Its a multiline field, so append in such a
case?
Yes, understand. But you allredy have a container for  multi textlines with
field 'description'. For me it sounds duplicated to have two kinds of free text
fields on forms like this. 

Searching/restricting/standarzing a list of allowed/possible reasons for loss is
much simpler for me to understand.

But maybe Iam wrong?

http://codereview.appspot.com/193111/diff/1/5#newcode125
sale_opportunity.py:125: """,[])
On 2010/01/26 13:47:25, sharoonthomas wrote:
> I thought it was needed because we need to COALESCE user and date, otherwise
its
> not needed
I don't know if I understand correctly, but #121 and #122 doesn't make much
sense for me, because of duplicate the information of #117-118 and #119-120

[ced, 2010-01-26 14:17:04]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode121
sale_opportunity.py:121: COALESCE(write_date, create_date) AS date,
You should cast into a datetime field format.

http://codereview.appspot.com/193111/diff/1/5#newcode125
sale_opportunity.py:125: """,[])
On 2010/01/26 14:08:09, udo.spallek wrote:
> On 2010/01/26 13:47:25, sharoonthomas wrote:
> > I thought it was needed because we need to COALESCE user and date, otherwise
> its
> > not needed
> I don't know if I understand correctly, but #121 and #122 doesn't make much
> sense for me, because of duplicate the information of #117-118 and #119-120

No, write_date and create_date are timestamp so it is different from DateTime
fields. So i think it is good like that.

[yangoon1, 2010-01-26 14:23:10]

Many comments for you, so I won't stand back;)

General remark:

I don't see the difference between States lead and opportunity. 

AFAIS leads is the generic expression for all sort of contact to be established
between two parties, it could be a subscription to a newsletter as well as the
request for some brochure.
I understand opportunities as a subset of leads, designed to generate sales.
Since this module aims at the latter, I would prefer throughout the module the
description Opportunity (not Lead/Opportunity or Sale Lead/Opportunity).

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode9
sale_opportunity.py:9: ('opportunity','Opportunity'),
What's the difference between state lead and state opportunity?

[sharoonthomas, 2010-01-26 14:34:33]

http://codereview.appspot.com/193111/diff/1/5
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1/5#newcode13
sale_opportunity.py:13: ]
On 2010/01/26 14:02:14, bch wrote:
> Extra spaces after '['.
> ']' should be aligned with his brother.
> 
> And ',' must be followed by a blank space. If you use emacs, you can use this
> command to spot them all:
> 
> "M-x occur RET ,\S "
> 
> (there is a whitespace after \S)
> 

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode20
sale_opportunity.py:20: #Fields
On 2010/01/26 14:02:14, bch wrote:
> We avoid comments like that. There are not needed when the code is homogeneous
> across all the modules.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode20
sale_opportunity.py:20: #Fields
On 2010/01/26 14:03:08, ced wrote:
> An empty line is enough to separate fields definition.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode21
sale_opportunity.py:21: title = fields.Char('Title', required=True,
translate=True,select=1)
On 2010/01/26 14:03:08, ced wrote:
> Space before select=1

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
On 2010/01/26 14:03:08, ced wrote:
> Put comma at the end of line like that if we add new states we don't need to
> change this line and back-porting patch will be easier.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
On 2010/01/26 14:03:08, ced wrote:
> Put comma at the end of line like that if we add new states we don't need to
> change this line and back-porting patch will be easier.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
On 2010/01/26 14:03:08, ced wrote:
> It could be: Not(Equal(Eval('state'), 'lead'))

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode25
sale_opportunity.py:25: 'readonly': Not(In(Eval('state'),['lead']))
On 2010/01/26 14:03:08, ced wrote:
> Put comma at the end of line like that if we add new states we don't need to
> change this line and back-porting patch will be easier.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode32
sale_opportunity.py:32: party =
fields.Many2One('party.party','Party',required=True,select=1,domain=[('active','in',[True,False])])
Agree
On 2010/01/26 14:03:08, ced wrote:
> I think we must not allow to select inactive parties.
> If there are logically deleted, it is because we don't want any more to use
it.

http://codereview.appspot.com/193111/diff/1/5#newcode33
sale_opportunity.py:33: address =
fields.Many2One('party.address','Address',domain=[('party','=',Eval('party'))],select=2)
On 2010/01/26 14:03:08, ced wrote:
> depends on party

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode35
sale_opportunity.py:35: state =
fields.Selection(STATES,'Stage',required=True,select=1,sort=False,readonly=True)
On 2010/01/26 14:03:08, ced wrote:
> Stage -> State

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode42
sale_opportunity.py:42: change_history_lines =
fields.One2Many('sale.opportunity.historylookup','sale_opportunity','History')
On 2010/01/26 14:03:08, ced wrote:
> Try to have the same names for field, model and string.
> 
> Maybe:
> history_lines, sale.opportunity.history.lines, 'History Lines'

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode51
sale_opportunity.py:51: ('check_percentage', 'wrong_percentage')
On 2010/01/26 14:03:08, ced wrote:
> It could be an _sql_constraints, it will be faster.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode52
sale_opportunity.py:52: ]
On 2010/01/26 14:02:14, bch wrote:
> An sql constraint would be quicker, see
>
http://hg.tryton.org/hgwebdir.cgi/modules/stock/file/b55d45337e81/inventory.p...
> for an example

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode54
sale_opportunity.py:54: 'wrong_percentage': 'Probability should be between 0 &
100!',
Not used any more, made into SQL Constrain
On 2010/01/26 14:03:08, ced wrote:
> It don't know if it is good to use '&' char.

http://codereview.appspot.com/193111/diff/1/5#newcode63
sale_opportunity.py:63: if (probability <0) or (probability>100):
Not used anymore
On 2010/01/26 14:03:08, ced wrote:
> space around operators:
> 
> if (probability < 0) or (probability > 100):

http://codereview.appspot.com/193111/diff/1/5#newcode73
sale_opportunity.py:73: def end_lead(self, cursor, user, opp_id, context=None):
On 2010/01/26 14:03:08, ced wrote:
> We try to always write function that works with list of ids.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode76
sale_opportunity.py:76: This is called from the workflow
On 2010/01/26 14:03:08, ced wrote:
> Not necessary to tell from were it is called because it could be called from
> other places and the comment will not be accurate.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode77
sale_opportunity.py:77: opp_id: id of the record to be flagged with the current
datetime
On 2010/01/26 14:03:08, ced wrote:
> We use the syntax:
> 
> :param opp_id: ...
> 
> And describe all params.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode80
sale_opportunity.py:80:
self.write(cursor,user,opp_id,{'end_date':datetime.datetime.today()})
On 2010/01/26 14:03:08, ced wrote:
> You must always pass the context

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode87
sale_opportunity.py:87: _name = "sale.opportunity.lines"
cedk, one lead can have any number of lines
On 2010/01/26 14:03:08, ced wrote:
> name it "sale.opportunity.line" 
> It define only one line.

http://codereview.appspot.com/193111/diff/1/5#newcode98
sale_opportunity.py:98: 'History of Sales oportunity'
On 2010/01/26 14:03:08, ced wrote:
> typo: apportunity

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode107
sale_opportunity.py:107: user = fields.Many2One('res.user','User/Empl')
On 2010/01/26 14:03:08, ced wrote:
> Only User

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode123
sale_opportunity.py:123: title, state, probability
On 2010/01/26 14:03:08, ced wrote:
> One line per field, it is more readable.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode124
sale_opportunity.py:124: FROM sale_opportunity__history
On 2010/01/26 14:03:08, ced wrote:
> Use self._table to be more generic.

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode124
sale_opportunity.py:124: FROM sale_opportunity__history
On 2010/01/26 14:03:08, ced wrote:
> Indent like in other modules:
> 
> SELECT
>     field,
>     field,
> FROM "table"
> WHERE ... = ..
>     ... = ...
> GROUP BY ...

Done.

http://codereview.appspot.com/193111/diff/1/5#newcode125
sale_opportunity.py:125: """,[])
Now I am not sure, can you help??
On 2010/01/26 14:08:09, udo.spallek wrote:
> On 2010/01/26 13:47:25, sharoonthomas wrote:
> > I thought it was needed because we need to COALESCE user and date, otherwise
> its
> > not needed
> I don't know if I understand correctly, but #121 and #122 doesn't make much
> sense for me, because of duplicate the information of #117-118 and #119-120

[sharoonthomas, 2010-01-26 15:24:05]

http://codereview.appspot.com/193111/diff/1/3
File __tryton__.py (right):

http://codereview.appspot.com/193111/diff/1/3#newcode19
__tryton__.py:19: 'sale_opportunity_view.xml',
On 2010/01/26 14:03:08, ced wrote:
> We don't separate xml like that.
> One xml file for data related to one Model

Done.

http://codereview.appspot.com/193111/diff/1/4
File party.xml (right):

http://codereview.appspot.com/193111/diff/1/4#newcode7
party.xml:7: <field name="name">Leads/Opportunities</field>
On 2010/01/26 14:03:08, ced wrote:
> "Sale Opportunities"?

Done.

http://codereview.appspot.com/193111/diff/1/6
File sale_opportunity_view.xml (right):

http://codereview.appspot.com/193111/diff/1/6#newcode8
sale_opportunity_view.xml:8: <![CDATA[
On 2010/01/26 14:03:08, ced wrote:
> Indent CDATA tag with others.

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode9
sale_opportunity_view.xml:9: <form string="Sales Lead/Opportunity">
On 2010/01/26 14:03:08, ced wrote:
> string in form are always in singular.
> "Sale Lead/Opportunity"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode31
sale_opportunity_view.xml:31: <field name="change_history_lines">
On 2010/01/26 14:03:08, ced wrote:
> Define form and tree view for history outside.
> And only inside other view when you want different view.

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode43
sale_opportunity_view.xml:43: <field name="lines" colspan="4">
On 2010/01/26 14:03:08, ced wrote:
> Better in notebook?

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode76
sale_opportunity_view.xml:76: <tree string="CRM Lead/Opportunity">
On 2010/01/26 14:03:08, ced wrote:
> string in tree view are always in plural.
> "Sale Leads/Opportunities"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode87
sale_opportunity_view.xml:87: <record model="ir.action.act_window"
id="act_saleopportunity_form">
On 2010/01/26 14:03:08, ced wrote:
> id="act_sale_opportunity_form"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode88
sale_opportunity_view.xml:88: <field name="name">All Sales Lead and
Opportunity</field>
On 2010/01/26 14:03:08, ced wrote:
> Same structure than for sale so just
> "Sale Leads/Opportunities"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode91
sale_opportunity_view.xml:91: </record>
On 2010/01/26 14:03:08, ced wrote:
> You must define also ir.action.act_window.view

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode92
sale_opportunity_view.xml:92: <record model="ir.action.act_window"
id="act_saleopportunity_form_open">
On 2010/01/26 14:03:08, ced wrote:
> id="act_sale_opportunity_form_lead"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode93
sale_opportunity_view.xml:93: <field name="name">Open Sales Leads</field>
On 2010/01/26 14:03:08, ced wrote:
> Opened Leads

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode99
sale_opportunity_view.xml:99: <field name="name">Sales Opportunities</field>
On 2010/01/26 14:03:08, ced wrote:
> Simply: "Opportunities"

Done.

http://codereview.appspot.com/193111/diff/1/6#newcode105
sale_opportunity_view.xml:105: <field name="name">Converted Leads</field>
On 2010/01/26 14:03:08, ced wrote:
> Converted Opportunities

Done.

http://codereview.appspot.com/193111/diff/1/7
File sale_opportunity_workflow.xml (right):

http://codereview.appspot.com/193111/diff/1/7#newcode4
sale_opportunity_workflow.xml:4: <record model="workflow"
id="sale_opportunity_workflow">
On 2010/01/26 14:03:08, ced wrote:
> id="opportunity_workflow"
> No need to prefix with sale_

Done.

http://codereview.appspot.com/193111/diff/1/7#newcode9
sale_opportunity_workflow.xml:9: <record model="workflow.activity"
id="sale_activity_lead">
On 2010/01/26 14:03:08, ced wrote:
> id="opportunity_activity_lead"

Done.

http://codereview.appspot.com/193111/diff/1/7#newcode20
sale_opportunity_workflow.xml:20: <field name="action">write({'state':
'opportunity','probability':50})</field>
On 2010/01/26 14:03:08, ced wrote:
> Put a default probability value on the Model

Done.

http://codereview.appspot.com/193111/diff/1/7#newcode26
sale_opportunity_workflow.xml:26: <field
name="action">end_lead()&#10;write({'state': 'converted'})</field>
On 2010/01/26 14:03:08, ced wrote:
> Use only one function it is better to override.

Done.

http://codereview.appspot.com/193111/diff/1/7#newcode42
sale_opportunity_workflow.xml:42: <record model="workflow.transition"
id="saleopp_transition_lead_opportunity">
On 2010/01/26 14:03:08, ced wrote:
> id="opportunity_transition_lead_opportunity"

Done.

[udono, 2010-01-26 15:27:22]

On 2010/01/26 14:17:04, ced wrote:
> http://codereview.appspot.com/193111/diff/1/5
> File sale_opportunity.py (right):
> http://codereview.appspot.com/193111/diff/1/5#newcode121
> sale_opportunity.py:121: COALESCE(write_date, create_date) AS date,
> You should cast into a datetime field format.
> http://codereview.appspot.com/193111/diff/1/5#newcode125
> sale_opportunity.py:125: """,[])
> On 2010/01/26 14:08:09, udo.spallek wrote:
> > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > I thought it was needed because we need to COALESCE user and date,
otherwise
> > its
> > > not needed
> > I don't know if I understand correctly, but #121 and #122 doesn't make much
> > sense for me, because of duplicate the information of #117-118 and #119-120
> No, write_date and create_date are timestamp so it is different from DateTime
> fields. So i think it is good like that.
What I ask me is, why we have a write_date and a write_user on history table. Is
there any UPDATE of an existing field in historisation?

[ced, 2010-01-26 15:31:42]

On 2010/01/26 15:27:22, udo.spallek wrote:
> On 2010/01/26 14:17:04, ced wrote:
> > On 2010/01/26 14:08:09, udo.spallek wrote:
> > > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > > I thought it was needed because we need to COALESCE user and date,
> otherwise
> > > its
> > > > not needed
> > > I don't know if I understand correctly, but #121 and #122 doesn't make
much
> > > sense for me, because of duplicate the information of #117-118 and
#119-120
> > No, write_date and create_date are timestamp so it is different from
DateTime
> > fields. So i think it is good like that.
> What I ask me is, why we have a write_date and a write_user on history table.
Is
> there any UPDATE of an existing field in historisation?

No there is no UPDATE on history tables but they reflect exactly the same
columns then the origin tables.
And we need those fields (create/write_date/uid) because client expect it for
any Models.

[udono, 2010-01-26 15:35:41]

On 2010/01/26 15:31:42, ced wrote:
> On 2010/01/26 15:27:22, udo.spallek wrote:
> > On 2010/01/26 14:17:04, ced wrote:
> > > On 2010/01/26 14:08:09, udo.spallek wrote:
> > > > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > > > I thought it was needed because we need to COALESCE user and date,
> > otherwise
> > > > its
> > > > > not needed
> > > > I don't know if I understand correctly, but #121 and #122 doesn't make
> much
> > > > sense for me, because of duplicate the information of #117-118 and
> #119-120
> > > No, write_date and create_date are timestamp so it is different from
> DateTime
> > > fields. So i think it is good like that.
> > What I ask me is, why we have a write_date and a write_user on history
table.
> Is
> > there any UPDATE of an existing field in historisation?
> 
> No there is no UPDATE on history tables but they reflect exactly the same
> columns then the origin tables.
> And we need those fields (create/write_date/uid) because client expect it for
> any Models.

But then we do not need COALESCE, IMHO. A simple:

create_date AS date,
create_uid  AS user,	

is not enough?

[sharoonthomas, 2010-01-26 15:38:17]

Hi,

It think what happens is a double update, one to the original table and second
to the history table. So all entries of the same parent record will have same
create date in history table.
Also, the first update to history table will not have write_id

On 2010/01/26 15:35:41, udo.spallek wrote:
> On 2010/01/26 15:31:42, ced wrote:
> > On 2010/01/26 15:27:22, udo.spallek wrote:
> > > On 2010/01/26 14:17:04, ced wrote:
> > > > On 2010/01/26 14:08:09, udo.spallek wrote:
> > > > > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > > > > I thought it was needed because we need to COALESCE user and date,
> > > otherwise
> > > > > its
> > > > > > not needed
> > > > > I don't know if I understand correctly, but #121 and #122 doesn't make
> > much
> > > > > sense for me, because of duplicate the information of #117-118 and
> > #119-120
> > > > No, write_date and create_date are timestamp so it is different from
> > DateTime
> > > > fields. So i think it is good like that.
> > > What I ask me is, why we have a write_date and a write_user on history
> table.
> > Is
> > > there any UPDATE of an existing field in historisation?
> > 
> > No there is no UPDATE on history tables but they reflect exactly the same
> > columns then the origin tables.
> > And we need those fields (create/write_date/uid) because client expect it
for
> > any Models.
> 
> But then we do not need COALESCE, IMHO. A simple:
> 
> create_date AS date,
> create_uid  AS user,	
> 
> is not enough?

[ced, 2010-01-26 15:43:46]

http://codereview.appspot.com/193111/diff/1006/1007
File __tryton__.py (right):

http://codereview.appspot.com/193111/diff/1006/1007#newcode19
__tryton__.py:19: 'sale_opportunity_view.xml',
Just opportunity.xml

http://codereview.appspot.com/193111/diff/1006/1009
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1006/1009#newcode12
sale_opportunity.py:12: ('lost', 'Lost')
Add an ending comma.

http://codereview.appspot.com/193111/diff/1006/1009#newcode27
sale_opportunity.py:27: )
Can be on the above line.

http://codereview.appspot.com/193111/diff/1006/1009#newcode31
sale_opportunity.py:31: }, depends=['state'])
} must be align with states={

http://codereview.appspot.com/193111/diff/1006/1009#newcode33
sale_opportunity.py:33: address = fields.Many2One('party.address', 'Address',
domain=[('party', '=', Eval('party'))], select=2,
80cols

http://codereview.appspot.com/193111/diff/1006/1009#newcode46
sale_opportunity.py:46: reason_4_loss = fields.Text('Reason for loss', states={
Is not better lost_reason ?

http://codereview.appspot.com/193111/diff/1006/1009#newcode54
sale_opportunity.py:54: ('check_percentage','CHECK(probability >= 0 AND
probaility <= 100)', 'Probability must be between 0 and 100!')
probaility -> probability

http://codereview.appspot.com/193111/diff/1006/1009#newcode71
sale_opportunity.py:71: :param opp_id: list of id of the record to be flagged
with the current datetime
and the context.

http://codereview.appspot.com/193111/diff/1006/1009#newcode72
sale_opportunity.py:72: :return: nothing
No need of return if it is not defined

http://codereview.appspot.com/193111/diff/1006/1009#newcode76
sale_opportunity.py:76: 'end_date':datetime.datetime.today(),
We indent with 8cols and not align to {

http://codereview.appspot.com/193111/diff/1006/1009#newcode124
sale_opportunity.py:124: FROM """ + self._table + """__history
You should quote the table name

http://codereview.appspot.com/193111/diff/1006/1010
File sale_opportunity_view.xml (right):

http://codereview.appspot.com/193111/diff/1006/1010#newcode33
sale_opportunity_view.xml:33: <page string="Items" id="tems">
id="items"

http://codereview.appspot.com/193111/diff/1006/1010#newcode157
sale_opportunity_view.xml:157: <menuitem name="Leads and Opportunities"
id="menu_sale_leadsnopp"
Generally we put menuitem just after the action definition.

http://codereview.appspot.com/193111/diff/1006/1010#newcode172
sale_opportunity_view.xml:172: <record model="workflow"
id="opportunity_workflow">
Seems to be intended with tab

[ced, 2010-01-26 15:47:45]

On 2010/01/26 15:35:41, udo.spallek wrote:
> On 2010/01/26 15:31:42, ced wrote:
> > On 2010/01/26 15:27:22, udo.spallek wrote:
> > > On 2010/01/26 14:17:04, ced wrote:
> > > > On 2010/01/26 14:08:09, udo.spallek wrote:
> > > > > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > > > > I thought it was needed because we need to COALESCE user and date,
> > > otherwise
> > > > > its
> > > > > > not needed
> > > > > I don't know if I understand correctly, but #121 and #122 doesn't make
> > much
> > > > > sense for me, because of duplicate the information of #117-118 and
> > #119-120
> > > > No, write_date and create_date are timestamp so it is different from
> > DateTime
> > > > fields. So i think it is good like that.
> > > What I ask me is, why we have a write_date and a write_user on history
> table.
> > Is
> > > there any UPDATE of an existing field in historisation?
> > 
> > No there is no UPDATE on history tables but they reflect exactly the same
> > columns then the origin tables.
> > And we need those fields (create/write_date/uid) because client expect it
for
> > any Models.
> 
> But then we do not need COALESCE, IMHO. A simple:
> 
> create_date AS date,
> create_uid  AS user,	
> 
> is not enough?

I guess we want the writer user and the update date. So COALESCE is required.

[udono, 2010-01-26 15:57:29]

On 2010/01/26 15:47:45, ced wrote:
> On 2010/01/26 15:35:41, udo.spallek wrote:
> > On 2010/01/26 15:31:42, ced wrote:
> > > On 2010/01/26 15:27:22, udo.spallek wrote:
> > > > On 2010/01/26 14:17:04, ced wrote:
> > > > > On 2010/01/26 14:08:09, udo.spallek wrote:
> > > > > > On 2010/01/26 13:47:25, sharoonthomas wrote:
> > > > > > > I thought it was needed because we need to COALESCE user and date,
> > > > otherwise
> > > > > > its
> > > > > > > not needed
> > > > > > I don't know if I understand correctly, but #121 and #122 doesn't
make
> > > much
> > > > > > sense for me, because of duplicate the information of #117-118 and
> > > #119-120
> > > > > No, write_date and create_date are timestamp so it is different from
> > > DateTime
> > > > > fields. So i think it is good like that.
> > > > What I ask me is, why we have a write_date and a write_user on history
> > table.
> > > Is
> > > > there any UPDATE of an existing field in historisation?
> > > 
> > > No there is no UPDATE on history tables but they reflect exactly the same
> > > columns then the origin tables.
> > > And we need those fields (create/write_date/uid) because client expect it
> for
> > > any Models.
> > 
> > But then we do not need COALESCE, IMHO. A simple:
> > 
> > create_date AS date,
> > create_uid  AS user,	
> > 
> > is not enough?
> 
> I guess we want the writer user and the update date. So COALESCE is required.

Ok, now understanding. So that the write|create date|user are comming from the
changed record in the original table. They are not the date and user writing
into the history table.

So sharoonthomas is right with COALESCE.

[yangoon1, 2010-01-26 16:05:46]

http://codereview.appspot.com/193111/diff/1006/1009
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1006/1009#newcode84
sale_opportunity.py:84: "Sale Opportunity Lines/Items"
I would recommend general usage of single quotes.
and just name it Sale Opportunity Lines

http://codereview.appspot.com/193111/diff/1006/1009#newcode96
sale_opportunity.py:96: 'History of Sales opportunity'
Sale Opportunity History Lines

http://codereview.appspot.com/193111/diff/1006/1010
File sale_opportunity_view.xml (right):

http://codereview.appspot.com/193111/diff/1006/1010#newcode31
sale_opportunity_view.xml:31: <field name="history_lines"/>
only spaces. no tabs

[sharoonthomas, 2010-01-26 16:07:59]

http://codereview.appspot.com/193111/diff/1006/1007
File __tryton__.py (right):

http://codereview.appspot.com/193111/diff/1006/1007#newcode19
__tryton__.py:19: 'sale_opportunity_view.xml',
On 2010/01/26 15:43:47, ced wrote:
> Just opportunity.xml

Done.

http://codereview.appspot.com/193111/diff/1006/1009
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1006/1009#newcode12
sale_opportunity.py:12: ('lost', 'Lost')
On 2010/01/26 15:43:47, ced wrote:
> Add an ending comma.

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode27
sale_opportunity.py:27: )
On 2010/01/26 15:43:47, ced wrote:
> Can be on the above line.

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode31
sale_opportunity.py:31: }, depends=['state'])
On 2010/01/26 15:43:47, ced wrote:
> } must be align with states={

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode33
sale_opportunity.py:33: address = fields.Many2One('party.address', 'Address',
domain=[('party', '=', Eval('party'))], select=2,
On 2010/01/26 15:43:47, ced wrote:
> 80cols

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode46
sale_opportunity.py:46: reason_4_loss = fields.Text('Reason for loss', states={
On 2010/01/26 15:43:47, ced wrote:
> Is not better lost_reason ?

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode54
sale_opportunity.py:54: ('check_percentage','CHECK(probability >= 0 AND
probaility <= 100)', 'Probability must be between 0 and 100!')
On 2010/01/26 15:43:47, ced wrote:
> probaility -> probability

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode71
sale_opportunity.py:71: :param opp_id: list of id of the record to be flagged
with the current datetime
On 2010/01/26 15:43:47, ced wrote:
> and the context.

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode72
sale_opportunity.py:72: :return: nothing
On 2010/01/26 15:43:47, ced wrote:
> No need of return if it is not defined

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode84
sale_opportunity.py:84: "Sale Opportunity Lines/Items"
On 2010/01/26 16:05:46, yangoon wrote:
> I would recommend general usage of single quotes.
> and just name it Sale Opportunity Lines

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode96
sale_opportunity.py:96: 'History of Sales opportunity'
On 2010/01/26 16:05:46, yangoon wrote:
> Sale Opportunity History Lines

Done.

http://codereview.appspot.com/193111/diff/1006/1009#newcode124
sale_opportunity.py:124: FROM """ + self._table + """__history
On 2010/01/26 15:43:47, ced wrote:
> You should quote the table name

Done.

[sharoonthomas, 2010-01-26 16:15:14]

http://codereview.appspot.com/193111/diff/1006/1010
File sale_opportunity_view.xml (right):

http://codereview.appspot.com/193111/diff/1006/1010#newcode33
sale_opportunity_view.xml:33: <page string="Items" id="tems">
On 2010/01/26 15:43:47, ced wrote:
> id="items"

Done.

http://codereview.appspot.com/193111/diff/1006/1010#newcode157
sale_opportunity_view.xml:157: <menuitem name="Leads and Opportunities"
id="menu_sale_leadsnopp"
On 2010/01/26 15:43:47, ced wrote:
> Generally we put menuitem just after the action definition.

Done.

[udono, 2010-01-26 16:39:03]

looks great now

http://codereview.appspot.com/193111/diff/1006/1007
File __tryton__.py (right):

http://codereview.appspot.com/193111/diff/1006/1007#newcode19
__tryton__.py:19: 'sale_opportunity_view.xml',
On 2010/01/26 16:07:59, sharoonthomas wrote:
> On 2010/01/26 15:43:47, ced wrote:
> > Just opportunity.xml
> 
> Done.

Done.

http://codereview.appspot.com/193111/diff/1028/35
File sale_opportunity.py (right):

http://codereview.appspot.com/193111/diff/1028/35#newcode15
sale_opportunity.py:15: 'Sale Leads & opportunities'
and _O_pportuni....

http://codereview.appspot.com/193111/diff/1028/35#newcode95
sale_opportunity.py:95: 'Sale Opportunity Lines/Items'
- /Items

[ced, 2010-02-26 08:47:17]

I think it can be closed