[11.0][MIG] base_transaction_id #190

yvaucher · 2017-11-17T16:31:43Z

No description provided.

(lp:c2c-financial-addons/6.1 rev 24.1.5)

…t working yet, just for backup) (lp:c2c-financial-addons/6.1 rev 24.1.8)

…ll as on the automatic reconciliation wizard. (lp:c2c-financial-addons/6.1 rev 24.1.10)

…eed both finaly (lp:c2c-financial-addons/6.1 rev 24.1.15)

(lp:c2c-financial-addons/6.1 rev 58)

…ostly based on : account_statement_ext -> provide profile per bank statement, remove period, choose to use balance check or not,... account_statement_base_completion -> provide a completion rule system to fullfill the bank statement (partner, account,...) account_statement_base_import -> provide a base to create your own file parser for each bank/office and link it to a profile account_statement_transactionid_completion and account_statement_transactionid_import to use the transaction ID recorded in th SO account_advanced_reconcile -> An advanced way to setup reconciliation rules on every account account_financial_report_webkit -> some little fixes (lp:c2c-financial-addons/6.1 rev 63)

(lp:c2c-financial-addons/6.1 rev 77)

…me account than the invoice's one (the payable / receivable)

the values before the write

ghost · 2018-01-17T15:34:57Z

base_transaction_id/README.rst

+* Joël Grand-Guillaume <joel.grandguillaume@camptocamp.com>
+* Alexandre Fayolle <alexandre.fayolle@camptocamp.com>
+* Guewen Baconnier <guewen.baconnier@camptocamp.com>
+* Pedro Baeza <pedro.baezo@gmail.com>


is that email correct?

I removed it as he didn't contributed directly to that module, he has few commits through OCA module maintenance

Yeah, I haven't made anything significant here.

ghost · 2018-01-17T15:35:27Z

base_transaction_id/README.rst

+Bugs are tracked on `GitHub Issues
+<https://github.com/OCA/bank-statement-reconcile/issues>`_. In case of trouble, please
+check there if your issue has already been reported. If you spotted it first,
+help us smashing it by providing a detailed and welcomed feedback.


From latest README.rst template:
help us smash it by providing detailed and welcomed feedback.

ghost · 2018-01-17T15:40:47Z

base_transaction_id/i18n/base_transaction_id.pot

@@ -0,0 +1,58 @@
+# Translation of Odoo Server.


Remove this file

ghost · 2018-01-17T15:45:32Z

base_transaction_id/__manifest__.py

+ 'depends': [
+     'sale',
+ ],
+ 'website': 'https://www.odoo-community.org/',


https://github.com/OCA/account-reconcile

@yvaucher remember to update this

ghost · 2018-01-17T15:45:59Z

base_transaction_id/__manifest__.py

+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+{'name': 'Base transaction id for financial institutes',
+ 'version': '11.0.1.0.0',
+ 'author': "Camptocamp,Odoo Community Association (OCA)",


Separate authors w/ space, please

https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#modules

According to contribution guidelines this is correct

author + ',Odoo Community Association (OCA)

Odoo parses both with or without spaces, so no problem in any

@pedrobaeza that just doesn't look good when it gets rendered, imho

The parsing is the same as said. You can try.

ghost · 2018-01-17T15:46:56Z

base_transaction_id/views/invoice.xml

+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+
+  <record model="ir.ui.view" id="invoice_view_custom">


id must be declared before model

Please give me a reference on this, according to the OCA codebase both are used:

└─> ack record.*model=.*id= --xml | wc -l
24252
└─> ack record.*id=.*model= --xml | wc -l
21737

EDIT: wrong greping
EDIT2: removing occurence of OpenUpgrade which is not revelent

@yvaucher https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#format

ghost · 2018-01-17T15:54:16Z

base_transaction_id/README.rst

+===========
+
+Bugs are tracked on `GitHub Issues
+<https://github.com/OCA/bank-statement-reconcile/issues>`_. In case of trouble, please


https://github.com/OCA/account-reconcile/issues

yvaucher · 2018-01-18T13:20:05Z

Thanks for the review, I did the fixes accordingly

simahawk

@yvaucher code LGTM. Just some nitpicking non-blocking remarks

simahawk · 2018-01-19T08:18:59Z

base_transaction_id/README.rst

+Base transaction id for financial institutes
+============================================
+
+Adds transaction id to invoice and sale models and views.


keep ID always uppercase, just to make it homogeneous when referring to it

simahawk · 2018-01-19T08:19:36Z

base_transaction_id/__manifest__.py

@@ -0,0 +1,20 @@
+# Copyright 2012 Yannick Vaucher, Camptocamp SA


2012-2018 ? Same for other headings

18615b1

Here are the changes I made, I don't think it deserve a bump on copyright as no code was change in python files.

simahawk · 2018-01-19T08:22:18Z

base_transaction_id/models/account_bank_statement_line.py

+    @api.multi
+    def get_reconciliation_proposition(self, excluded_ids=None):
+        """ Look for transaction_ref to give them as proposition move line """
+        if self.name:


self.ensure_one() ?

This was written at a time we considered ensure_one to be called anyway. I can add it thought

simahawk · 2018-01-19T08:24:25Z

base_transaction_id/models/account_bank_statement_line.py

+
+    @api.multi
+    def get_reconciliation_proposition(self, excluded_ids=None):
+        """ Look for transaction_ref to give them as proposition move line """


not a requirement but a good habit: no space around docstring text + end w/ period ;)

simahawk · 2018-01-19T08:24:37Z

base_transaction_id/models/account_move.py

+    @api.model
+    def domain_move_lines_for_reconciliation(self, excluded_ids=None,
+                                             str=False):
+        """ Add transaction_ref in search of move lines"""


docstring: same

simahawk · 2018-01-19T08:27:16Z

base_transaction_id/views/invoice.xml

+    <field name="model">account.invoice</field>
+    <field name="inherit_id" ref="account.invoice_form"/>
+    <field name="arch" type="xml">
+      <xpath expr="//page[@name='other_info']/group/group/field[@name='origin']" position="after">


I'd replace group/group with // to make it more robust in case is moved. Even if as you are selecting the field origin.... why not selecting only that? Very likely we don't have the same filed twice. No?

Yes, can obviously be simplified.

It was probably failing in v10 as it was changed from
<field name="origin" position="after"> to this.

If there are 1 origin field in the invoice lines before the origin field of the main object.

So it might be safer to stick with the expr but simplify it a bit.

yvaucher · 2018-01-19T14:26:16Z

base_transaction_id/models/account_move.py

+
+    @api.model
+    def domain_move_lines_for_reconciliation(self, excluded_ids=None,
+                                             str=False):


Bad idea to use str as it is a builtin not sure were the super is defined though

Defined in src/addons/account/models/account_move.py

Let's try a PR on master odoo/odoo#22392

yvaucher · 2018-01-22T16:19:41Z

Squashed commits

ghost

LGTM w/ some remarks

ghost · 2018-02-01T16:18:58Z

base_transaction_id/models/account_bank_statement_line.py

+    @api.multi
+    def get_reconciliation_proposition(self, excluded_ids=None):
+        """Look for transaction_ref to give them as proposition move line."""
+        if self.name:


ensure_one?

Would make it more explicit right.

ghost · 2018-02-01T16:22:07Z

base_transaction_id/models/account_move.py

+    )
+
+    @api.multi
+    def prepare_move_lines_for_reconciliation_widget(self,


It's better to open the line right after the parenthesis :)

ghost · 2018-02-01T16:22:47Z

base_transaction_id/models/invoice.py

+class AccountInvoice(models.Model):
+    _inherit = 'account.invoice'
+
+    transaction_id = fields.Char(string='Transaction ID',


same remark about code formatting

While I also prefer this format with return after opening parethesis I prefer not to change this to not alter too much the code base history.

grindtildeath

Please update website key in manifest, otherwise LGTM 👍

yvaucher · 2018-03-05T15:58:44Z

Changed website key in __manifest__.py and added an ensure_one.

Other demands seems irrelevant to me for that module migration.

asaunier · 2018-04-05T10:16:00Z

@yvaucher The PR has been approved and it seems there is nothing left to do with it. What about merging? :)

yvaucher · 2018-04-05T12:24:55Z

@OCA/accounting-maintainers anyone who could merge this?

As it is my PR I don't want to force merge.

pedrobaeza · 2018-04-05T13:54:30Z

Can you maybe squash a bit the commit history? 🙏

asaunier

Squashing some commits would indeed help clarifying the history but I have the feeling this PR could be merged as is. @yvaucher already did some squashing 2.5 months ago. Isn't this PR been opened for too long now? :P

pedrobaeza · 2018-04-06T09:41:22Z

Yeah, but 50 commits for such a simple module I think it's too much, but I let you the final call.

yvaucher · 2018-04-06T10:55:57Z

@pedrobaeza ok I'll squash few of my commits and drop the ones "Make modules uninstallable"

I guess I can easily make it around 40 commits, this is a quite old module thus we have many commits.

yvaucher · 2018-04-06T11:53:26Z

@pedrobaeza I squashed the commits in a new branch, thus I'm closing this one

#206

yvaucher and others added 30 commits November 16, 2017 16:28

[ADD] mod base_transaction_id

3f085af

(lp:c2c-financial-addons/6.1 rev 24.1.5)

[IMP] First commit for improving the bank statement with treasury (no…

57b7175

…t working yet, just for backup) (lp:c2c-financial-addons/6.1 rev 24.1.8)

[IMP] Quite a huge work on the new intermediate statement stuff as we…

24c59c8

…ll as on the automatic reconciliation wizard. (lp:c2c-financial-addons/6.1 rev 24.1.10)

[IMP] Merge treasury statement into bank.statement object. We don't n…

7d9e166

…eed both finaly (lp:c2c-financial-addons/6.1 rev 24.1.15)

[MRG] From customer branch

b3e7b27

(lp:c2c-financial-addons/6.1 rev 58)

[FIX] spelling

363bc07

(lp:c2c-financial-addons/6.1 rev 77)

[MIGR] Adapt import to fit last recomandation

3a24fcb

[FIX] Import osv for osv.except

598768c

[FIX] pep8, pylint, eyeballing

8edbce2

[FIX] standardize the naming of the argument 'cr' instead of 'cursor'

ad588c3

[FIX] Remove the active key in the __openerp__.py

37dca35

[ADD] account_move_line.transaction_ref field in base_transaction_id

1bebfbc

copy the transaction_ref to the move lines from the invoice

ddb66ee

[IMP] Translation template files.

54f6b36

the transaction id is copied only on account move lines having the sa…

ff2fb85

…me account than the invoice's one (the payable / receivable)

Launchpad automatic translations update.

3cce87a

Set as uninstallable and moved to __unported__ all modules.

13862a7

Move base_transaction_id from __unported__ to root folder

2658e55

Set base_transaction_id as installable

4d630d6

Use the 'data' key instead of 'update_xml'

499c62d

Use the new api for account.invoice as the base model use it

932346f

The module now depends on 'sale_stock' and 'stock_account'

c509958

Avoid to write 2 times on the invoice by using the method that prepare

0ec8242

the values before the write

Empty the transaction id of a sale order on copy

31707fd

Some cleaning

dbe17da

Indent xml with 2 spaces

a0b05fc

Cleaning of the views

7274c2e

Update translations, add fr translation

4740415

Add the transaction ref field to the form view of the move lines

6cacb28

leemannd and others added 2 commits November 16, 2017 16:31

[MIG] base_transaction_id: Migrated to 10.0

76c062b

OCA Transbot updated translations from Transifex

9791606

yvaucher mentioned this pull request Nov 17, 2017

[11.0] mig base modules OCA/l10n-switzerland#367

Closed

4 tasks

pedrobaeza added this to the 11.0 milestone Nov 17, 2017

pedrobaeza added the needs review label Nov 17, 2017

pedrobaeza mentioned this pull request Nov 17, 2017

Migration to version 11.0 #186

Closed

26 tasks

yvaucher force-pushed the 11-0-mig-base_transaction_id branch from 65b7868 to 18615b1 Compare November 29, 2017 17:40

ghost suggested changes Jan 17, 2018

View reviewed changes

simahawk requested changes Jan 19, 2018

View reviewed changes

yvaucher commented Jan 19, 2018

View reviewed changes

simahawk approved these changes Jan 22, 2018

View reviewed changes

yvaucher force-pushed the 11-0-mig-base_transaction_id branch from e484090 to bdfc16d Compare January 22, 2018 16:19

ghost approved these changes Feb 1, 2018

View reviewed changes

grindtildeath approved these changes Feb 23, 2018

View reviewed changes

[MIG] base_transaction_id: Migration to 11.0

b6f25e6

yvaucher force-pushed the 11-0-mig-base_transaction_id branch from bdfc16d to b6f25e6 Compare March 5, 2018 15:53

mpanarin mentioned this pull request Mar 15, 2018

[ADD] l10n_ch_import_isr_v11: new module OCA/l10n-switzerland#402

Merged

2 tasks

asaunier approved these changes Apr 6, 2018

View reviewed changes

yvaucher mentioned this pull request Apr 6, 2018

[11.0][MIG] base_transaction_id #206

Merged

yvaucher closed this Apr 6, 2018

		@@ -0,0 +1,20 @@
		# Copyright 2012 Yannick Vaucher, Camptocamp SA

[11.0][MIG] base_transaction_id #190

[11.0][MIG] base_transaction_id #190

Conversation

yvaucher commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvaucher Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvaucher commented Jan 18, 2018

simahawk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvaucher commented Jan 22, 2018

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grindtildeath left a comment

Choose a reason for hiding this comment

yvaucher commented Mar 5, 2018 • edited Loading

asaunier commented Apr 5, 2018

yvaucher commented Apr 5, 2018

pedrobaeza commented Apr 5, 2018

asaunier left a comment

Choose a reason for hiding this comment

pedrobaeza commented Apr 6, 2018

yvaucher commented Apr 6, 2018

yvaucher commented Apr 6, 2018

yvaucher Jan 18, 2018 •

edited

Loading

yvaucher commented Mar 5, 2018 •

edited

Loading