Merge lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-account_easy_reconcile-analytic-account into lp:banking-addons/bank-statement-reconcile-70

Proposed by Guewen Baconnier @ Camptocamp
Status: Rejected
Rejected by: Leonardo Pistone
Proposed branch: lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-account_easy_reconcile-analytic-account
Merge into: lp:banking-addons/bank-statement-reconcile-70
Diff against target: 56 lines (+10/-0)
3 files modified
account_easy_reconcile/base_reconciliation.py (+3/-0)
account_easy_reconcile/easy_reconcile.py (+5/-0)
account_easy_reconcile/easy_reconcile.xml (+2/-0)
To merge this branch: bzr merge lp:~camptocamp/banking-addons/bank-statement-reconcile-7.0-account_easy_reconcile-analytic-account
Reviewer Review Type Date Requested Status
Leonardo Pistone Needs Resubmitting
Ana Juaristi Olalde (community) code review Approve
Pedro Manuel Baeza code review Approve
Yannick Vaucher @ Camptocamp code review, no test Approve
Review via email: mp+213007@code.launchpad.net

This proposal supersedes a proposal from 2014-03-27.

Description of the change

Allow to set an analytic account on write-off entries created during reconciliations.

To post a comment you must log in.
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Please put False default to avoid extrange values (browse_null or similar) on dictionary on l.32:

'analytic_account_id': (rec_method.analytic_account_id and
                        rec_method.analytic_account_id.id or False),

Regards.

review: Needs Fixing (code review)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> Please put False default to avoid extrange values (browse_null or similar) on
> dictionary on l.32:
>
> 'analytic_account_id': (rec_method.analytic_account_id and
> rec_method.analytic_account_id.id or False),
>
> Regards.

The code is similar to the other lines around. Also, what would be the problem to have a browse_null here?

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Guewen, I don't remember exactly the actual reason for that, but there is a possible side effect that somebody pointed me on another MP at the same time I reviewed this (maybe you were even involved in that review also).

Regards.

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Hello Pedro, I don't see either how it could return browse_null.

IMO, we could even simply write

'analytic_account_id': rec_method.analytic_account_id.id,

As if rec_method.analytic_account_id is browse_null
rec_method.analytic_account_id.id would return False

But LGTM like this.

review: Approve (code review, no test)
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Yeah, I would probably remember bad the issue.

Regards.

review: Approve (code review)
Revision history for this message
Ana Juaristi Olalde (ajuaristio) wrote :

LGTM

review: Approve (code review)
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

This project is now hosted on https://github.com/OCA/bank-statement-reconcile. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

Revision history for this message
Leonardo Pistone (lepistone) wrote :
review: Needs Resubmitting

Unmerged revisions

138. By Guewen Baconnier @ Camptocamp

In account_easy_reconcile, now write-off entries can be created with an analytic account

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'account_easy_reconcile/base_reconciliation.py'
--- account_easy_reconcile/base_reconciliation.py 2013-01-04 08:39:10 +0000
+++ account_easy_reconcile/base_reconciliation.py 2014-03-27 09:26:20 +0000
@@ -188,6 +188,9 @@
188 period_id = self.pool.get('account.period').find(188 period_id = self.pool.get('account.period').find(
189 cr, uid, dt=date, context=context)[0]189 cr, uid, dt=date, context=context)[0]
190190
191 if rec.analytic_account_id:
192 rec_ctx['analytic_id'] = rec.analytic_account_id.id
193
191 ml_obj.reconcile(194 ml_obj.reconcile(
192 cr, uid,195 cr, uid,
193 line_ids,196 line_ids,
194197
=== modified file 'account_easy_reconcile/easy_reconcile.py'
--- account_easy_reconcile/easy_reconcile.py 2013-02-13 15:54:48 +0000
+++ account_easy_reconcile/easy_reconcile.py 2014-03-27 09:26:20 +0000
@@ -56,6 +56,9 @@
56 required=True,56 required=True,
57 string='Date of reconciliation'),57 string='Date of reconciliation'),
58 'filter': fields.char('Filter', size=128),58 'filter': fields.char('Filter', size=128),
59 'analytic_account_id': fields.many2one(
60 'account.analytic.account', 'Analytic Account',
61 help="Analytic account for the write-off"),
59 }62 }
6063
61 _defaults = {64 _defaults = {
@@ -198,6 +201,8 @@
198 rec_method.account_lost_id.id),201 rec_method.account_lost_id.id),
199 'account_profit_id': (rec_method.account_profit_id and202 'account_profit_id': (rec_method.account_profit_id and
200 rec_method.account_profit_id.id),203 rec_method.account_profit_id.id),
204 'analytic_account_id': (rec_method.analytic_account_id and
205 rec_method.analytic_account_id.id),
201 'journal_id': (rec_method.journal_id and206 'journal_id': (rec_method.journal_id and
202 rec_method.journal_id.id),207 rec_method.journal_id.id),
203 'date_base_on': rec_method.date_base_on,208 'date_base_on': rec_method.date_base_on,
204209
=== modified file 'account_easy_reconcile/easy_reconcile.xml'
--- account_easy_reconcile/easy_reconcile.xml 2013-02-13 15:54:48 +0000
+++ account_easy_reconcile/easy_reconcile.xml 2014-03-27 09:26:20 +0000
@@ -124,6 +124,7 @@
124 <field name="account_lost_id" attrs="{'required':[('write_off','>',0)]}"/>124 <field name="account_lost_id" attrs="{'required':[('write_off','>',0)]}"/>
125 <field name="account_profit_id" attrs="{'required':[('write_off','>',0)]}"/>125 <field name="account_profit_id" attrs="{'required':[('write_off','>',0)]}"/>
126 <field name="journal_id" attrs="{'required':[('write_off','>',0)]}"/>126 <field name="journal_id" attrs="{'required':[('write_off','>',0)]}"/>
127 <field name="analytic_account_id" groups="analytic.group_analytic_accounting"/>
127 <field name="date_base_on"/>128 <field name="date_base_on"/>
128 </form>129 </form>
129 </field>130 </field>
@@ -141,6 +142,7 @@
141 <field name="account_lost_id" attrs="{'required':[('write_off','>',0)]}"/>142 <field name="account_lost_id" attrs="{'required':[('write_off','>',0)]}"/>
142 <field name="account_profit_id" attrs="{'required':[('write_off','>',0)]}"/>143 <field name="account_profit_id" attrs="{'required':[('write_off','>',0)]}"/>
143 <field name="journal_id" attrs="{'required':[('write_off','>',0)]}"/>144 <field name="journal_id" attrs="{'required':[('write_off','>',0)]}"/>
145 <field name="analytic_account_id" groups="analytic.group_analytic_accounting"/>
144 <field name="date_base_on"/>146 <field name="date_base_on"/>
145 </tree>147 </tree>
146 </field>148 </field>

Subscribers

People subscribed via source and target branches