Issue 1853047: Converting modules/company to use tryton Transaction module (Closed)
Patch Set 1 #Patch Set 2 : Fixed Transaction() calls #
Total comments: 13
Patch Set 3 : Made changes per ced's notes #Patch Set 4 : Fixed unittests #
Total comments: 17
Patch Set 5 : Fixes per ced notes, fixed up report classes #
Total comments: 1
Patch Set 6 : initialize localcontext when None #Patch Set 7 : fixed localcontext initialization so as not to clobber an empty dict #
Total comments: 1
Patch Set 8 : Fix per ced #Patch Set 9 : Fixed report parse() #MessagesTotal messages: 16
And update INSTALL http://codereview.appspot.com/1853047/diff/2001/3001 File company.py (right): http://codereview.appspot.com/1853047/diff/2001/3001#newcode3 company.py:3: "Company" Can be removed. http://codereview.appspot.com/1853047/diff/2001/3001#newcode59 company.py:59: self.pool.get('ir.rule').domain_get(Transaction().cursor.dbname) Cache has changed: self.pool.get('ir.rule').domain_get.reset() http://codereview.appspot.com/1853047/diff/2001/3001#newcode101 company.py:101: if Transaction().context.get('company'): Could be one line http://codereview.appspot.com/1853047/diff/2001/3001#newcode124 company.py:124: [('parent', 'child_of', [company_id])]) tabs http://codereview.appspot.com/1853047/diff/2001/3001#newcode124 company.py:124: [('parent', 'child_of', [company_id])]) One line per search clause. http://codereview.appspot.com/1853047/diff/2001/3001#newcode145 company.py:145: [('parent', 'child_of', [user.main_company.id])]) one line per clause http://codereview.appspot.com/1853047/diff/2001/3001#newcode154 company.py:154: context_only=context_only) tabs http://codereview.appspot.com/1853047/diff/2001/3001#newcode185 company.py:185: field_id) tabs. http://codereview.appspot.com/1853047/diff/2001/3001#newcode212 company.py:212: if Transaction().context.get('company'): Could be one line http://codereview.appspot.com/1853047/diff/2001/3001#newcode271 company.py:271: user_ids = user_obj.search([('main_company', '=', False)]) one line per clause http://codereview.appspot.com/1853047/diff/2001/3001#newcode288 company.py:288: Transaction().context['company'] = user.company You must use with statement http://codereview.appspot.com/1853047/diff/2001/3001#newcode300 company.py:300: Transaction().context = Transaction().context.copy() Use with statement http://codereview.appspot.com/1853047/diff/2001/3002 File cron.py (right): http://codereview.appspot.com/1853047/diff/2001/3002#newcode12 cron.py:12: def _callback(self, cron): There is many use of cursor, you could instantiate a cursor variable.
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/10001/11002 File company.py (right): http://codereview.appspot.com/1853047/diff/10001/11002#newcode101 company.py:101: return Transaction().context.get['company'] or False get needs () http://codereview.appspot.com/1853047/diff/10001/11002#newcode154 company.py:154: context_only=context_only) Wrong indent http://codereview.appspot.com/1853047/diff/10001/11002#newcode185 company.py:185: field_id) Wrong indent http://codereview.appspot.com/1853047/diff/10001/11002#newcode212 company.py:212: return Transaction().context.get['company'] or False get () http://codereview.appspot.com/1853047/diff/10001/11002#newcode294 company.py:294: def parse(self, report, objects, datas): Report has localcontext http://codereview.appspot.com/1853047/diff/10001/11002#newcode298 company.py:298: with Transaction().set_user(user): In fact it is localcontext that must be filled. http://codereview.appspot.com/1853047/diff/10001/11003 File cron.py (right): http://codereview.appspot.com/1853047/diff/10001/11003#newcode13 cron.py:13: _cursor = Transaction().cursor Why putting an _ ?
Sign in to reply to this message.
I will fix the other simple ones, but I want to understand a couple of the other comments first.. http://codereview.appspot.com/1853047/diff/10001/11002 File company.py (right): http://codereview.appspot.com/1853047/diff/10001/11002#newcode185 company.py:185: field_id) On 2010/07/31 19:43:07, ced wrote: > Wrong indent I thought the convention was a single indent of 4 spaces (more than the previous line) when continuing lines in an implicit continuation [i.e, within ( ) ]. How much should this be indented? http://codereview.appspot.com/1853047/diff/10001/11002#newcode294 company.py:294: def parse(self, report, objects, datas): On 2010/07/31 19:43:07, ced wrote: > Report has localcontext Ok, I'm not exactly sure I understand what I must do here then... Can you explain a little more? http://codereview.appspot.com/1853047/diff/10001/11003 File cron.py (right): http://codereview.appspot.com/1853047/diff/10001/11003#newcode13 cron.py:13: _cursor = Transaction().cursor On 2010/07/31 19:43:07, ced wrote: > Why putting an _ ? Mostly to indicate this is a private variable with very local scope. But If you would prefer simply "cursor", I will change it.
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/10001/11002 File company.py (right): http://codereview.appspot.com/1853047/diff/10001/11002#newcode185 company.py:185: field_id) On 2010/07/31 22:44:48, pheller wrote: > On 2010/07/31 19:43:07, ced wrote: > > Wrong indent > > I thought the convention was a single indent of 4 spaces (more than the previous > line) when continuing lines in an implicit continuation [i.e, within ( ) ]. > > How much should this be indented? 8 http://codereview.appspot.com/1853047/diff/10001/11002#newcode294 company.py:294: def parse(self, report, objects, datas): On 2010/07/31 22:44:48, pheller wrote: > On 2010/07/31 19:43:07, ced wrote: > > Report has localcontext > > Ok, I'm not exactly sure I understand what I must do here then... Can you > explain a little more? > You can look at account patch http://codereview.appspot.com/1853047/diff/10001/11003 File cron.py (right): http://codereview.appspot.com/1853047/diff/10001/11003#newcode13 cron.py:13: _cursor = Transaction().cursor On 2010/07/31 22:44:48, pheller wrote: > On 2010/07/31 19:43:07, ced wrote: > > Why putting an _ ? > > Mostly to indicate this is a private variable with very local scope. But If you > would prefer simply "cursor", I will change it. We don't use this convention and variables in method are always local
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/10001/11002 File company.py (right): http://codereview.appspot.com/1853047/diff/10001/11002#newcode185 company.py:185: field_id) On 2010/08/01 06:53:03, ced wrote: > On 2010/07/31 22:44:48, pheller wrote: > > On 2010/07/31 19:43:07, ced wrote: > > > Wrong indent > > > > I thought the convention was a single indent of 4 spaces (more than the > previous > > line) when continuing lines in an implicit continuation [i.e, within ( ) ]. > > > > How much should this be indented? > > 8 Ok, so this is why I'm confused: Please see: http://codereview.appspot.com/1903042/diff/11001/12002 (right side) Lines 1500-1501; 8 space additional indent on line continuation (right side) Lines 1503-1504; 4 space additional indent on line continuation http://codereview.appspot.com/1853047/diff/10001/11002#newcode294 company.py:294: def parse(self, report, objects, datas): On 2010/08/01 06:53:03, ced wrote: > On 2010/07/31 22:44:48, pheller wrote: > > On 2010/07/31 19:43:07, ced wrote: > > > Report has localcontext > > > > Ok, I'm not exactly sure I understand what I must do here then... Can you > > explain a little more? > > > You can look at account patch > Ok, so I only have to add localcontext to the parse() signature line, and pass in the call to super? Or should I actually use localcontext to set the user (instead of Transaction().set_user) ? Does this also apply to CompanyReport, and setting 'company' in the localcontext? (above)
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/10001/11002 File company.py (right): http://codereview.appspot.com/1853047/diff/10001/11002#newcode185 company.py:185: field_id) On 2010/08/01 16:38:08, pheller wrote: > On 2010/08/01 06:53:03, ced wrote: > > On 2010/07/31 22:44:48, pheller wrote: > > > On 2010/07/31 19:43:07, ced wrote: > > > > Wrong indent > > > > > > I thought the convention was a single indent of 4 spaces (more than the > > previous > > > line) when continuing lines in an implicit continuation [i.e, within ( ) ]. > > > > > > How much should this be indented? > > > > 8 > > Ok, so this is why I'm confused: > > Please see: http://codereview.appspot.com/1903042/diff/11001/12002 > > (right side) Lines 1500-1501; 8 space additional indent on line continuation > (right side) Lines 1503-1504; 4 space additional indent on line continuation This is because there is double brackets. It is the way vim indent. http://codereview.appspot.com/1853047/diff/10001/11002#newcode294 company.py:294: def parse(self, report, objects, datas): On 2010/08/01 16:38:08, pheller wrote: > On 2010/08/01 06:53:03, ced wrote: > > On 2010/07/31 22:44:48, pheller wrote: > > > On 2010/07/31 19:43:07, ced wrote: > > > > Report has localcontext > > > > > > Ok, I'm not exactly sure I understand what I must do here then... Can you > > > explain a little more? > > > > > You can look at account patch > > > > Ok, so I only have to add localcontext to the parse() signature line, and pass > in the call to super? Yes > Or should I actually use localcontext to set the user > (instead of Transaction().set_user) ? Yes > > Does this also apply to CompanyReport, and setting 'company' in the > localcontext? (above) Yes
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/21001/3005 File company.py (right): http://codereview.appspot.com/1853047/diff/21001/3005#newcode285 company.py:285: localcontext['company'] = user.company.id You must initialize localcontext if it is None
Sign in to reply to this message.
Ok for me
Sign in to reply to this message.
http://codereview.appspot.com/1853047/diff/25001/26002 File company.py (right): http://codereview.appspot.com/1853047/diff/25001/26002#newcode287 company.py:287: localcontext['company'] = user.company.id It should be: localcontext['company'] = user.company
Sign in to reply to this message.
|
