Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8.0 import parsers #4

Closed
wants to merge 67 commits into from
Closed

8.0 import parsers #4

wants to merge 67 commits into from

Conversation

NL66278
Copy link
Contributor

@NL66278 NL66278 commented Dec 24, 2014

  • Add framework to allow for easy conversion of 7.0 and previous style bank statement import parsers.
  • As an example provide a (fully functional) convertion of the camt parser.

TODO: add more of the 7.0 functionality.

REMARK: settings have been removed here, as all the settings really apply to reconciliation, not to import.

Base module to write parsers for bank statement import files.

The module account_bank_statement_import, backported from odoo 9.0, is
exentended, so parsers developed might hope to be compatible with future
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/exentended/extended

@NL66278
Copy link
Contributor Author

NL66278 commented Jan 14, 2015

Please do not merge for the moment.

Rethinkink my solution to the problem of lots of Undefined Banks in the statement - now solved by supressing the creation - I think the automatic creation should rather be enhanced.

This will make the import improvements be more compatible with the standard Odoo functionality of linking autocreated bank accounts to manually created partners on reconciliation.

@NL66278
Copy link
Contributor Author

NL66278 commented Jan 16, 2015

Bank accounts are created again. I created the module bank_account_search to make this more reliable. This module could also be used separate from the import functions.

@alexis-via
Copy link
Contributor

I tried to used the module bank_statement_parse_camt. After figuring out that my file was camt.052 and not camt.053 and doing a few hacks to make it work with camt.052, I was very surprised to see the changes to the data model of the bank_statement_line and to the view of the bank_statement_line. This is probably fine when you ONLY use camt format, but when you use CAMT together with other formats (for other bank accounts in other banks), then it's a problem to add some fields and hide others. It also seems that bank_statement_parse has a lot of generic method that should belong to account_bank_statement_import : are they all complementary ?

It seems to me that "bank_statement_parse" is a sort of "framework" above the standard "account_bank_statement_import" framework... isn't it ?

@NL66278
Copy link
Contributor Author

NL66278 commented Feb 20, 2015

Hello Alexis,

The bank_statement_parse is a framework above a framework in the sense
that it provides for easy conversion of the parsers written for 7.0 and
before. These parsers returned arrays of Bank Statement objects,
containing Bank Transaction objects. The convert_statements method added
to account_bank_statement_import does the heavy lifting of pressing the
information returned from these parsers into the format expected by the
framework backported from Odoo master.

Taking my conversion of the camt parser as a lead, one of our customers
provided the conversion of the mt940 parser in the space of two days.

I tested importing ofx files, camt files (from several banks) and mt940
files all in one database. This did not pose a problem.

I found the method used to automatically create bank-accounts lacking in
the originally backported code (before Odoo changed everything). It
still does not do a proper recognition of IBAN bank accounts. The
original code did not do a proper recognition of existing bank accounts
with a different formatting for the number. They did solve this at the
expense of needing a full table scan for the bank account number search.
That is why I have the bank_account_search module as a requirement for
the bank_statement_parse. It provides an indexed (and unique!) bank
number key, without all the formatting, allowing the user to format the
bank account in the user interface, just like they wish, while still
providing for fast searches.

As for the extra fields in bank_statement_line, their rationale is as
follows:

  • data is for debugging purposes (to have the unformatted data that was
    used to create the line)
  • transfer_type might be crucial information to identify where an amount
    came from (cash payment? cash withdrawal? direct debit?). Unfortunately
    the contents of these fields are not standardized between banks, but I
    think all, or nearly all banks have them.
  • remote_owner is needed to make a transaction recognizable for the
    users when we do have a name, but the transaction has not been linked to
    an existing or new partner yet.
  • remote_account will show the bank account information just as it was
    received in the transaction import file - unless a regular bank account
    could be linked or created. This one might not be strictly necesarry.

Another aspect of my code is that when a bank account is automatically
created, it stores all the partner data available. Later on, this can be
used to find the right existing partner, of provide the data to create a
new partner. See the bank_statement_link_partner module proposed for the
bank-statement-reconcile project.

I realise there is a lot to be said about all of this and I really
apreciate all remarks and questions.

Thanks for looking at the code!

Kind regards, Ronald

Op 19-02-15 om 23:22 schreef Alexis de Lattre:

I tried to used the module bank_statement_parse_camt. After figuring out
that my file was camt.052 and not camt.053 and doing a few hacks to make
it work with camt.052, I was very surprised to see the changes to the
data model of the bank_statement_line and to the view of the
bank_statement_line. This is probably fine when you ONLY use camt
format, but when you use CAMT together with other formats (for other
bank accounts in other banks), then it's a problem to add some fields
and hide others. It also seems that bank_statement_parse has a lot of
generic method that should belong to account_bank_statement_import : are
they all complementary ?

It seems to me that "bank_statement_parse" is a sort of "framework"
above the standard "account_bank_statement_import" framework... isn't it ?


Reply to this email directly or view it on GitHub
#4 (comment).

NL66278 and others added 19 commits February 20, 2015 15:04
[ENH] Add framework to migrate 7.0 and before import parsers.
    directories in odoo modules 'lib' or they will be shredded by
    the default .gitignore!
    - Basic functionality to convert 7.0 and previous style imports is
      now functional.
- nl.po files added to all modules
- Added CAMT.053 to the help text of the import wizard
- Changed OpenERP to Odoo in view and translation files
    No configuration menu for statement import.
    Show bank-accounts - having nothing to do with configuration -
    directly in Bank and Cash menu.
    Separate general improvements in import framework from module that
    enables pre 8.0 style statement import parsers.
[FIX] Do away with bank_statement_parser, because it contains only
    a trivial amount of code, used in only a few parsers. The code
    itself is moved to new methods added to account.bank.statement and
    res.partner.bank respectively.
    - Add transfer_type and data to account_bank_statement_line.
[ENH] Module bank_statement_parse_camt:
    - If transaction is a cash transaction, use additional
      transaction details to fill message.
Undid the split in bank_statement_import_improve and
bank_statement_parse modules, as parsing is integral to the
improvements.

For camt files make sure transfer_type (transaction code) and data are
available in bank transaction.

Added transfer_type to transaction details in statement view.
Bank accounts in imported statements will always be shown, even if not
recognized in existing bank accounts.

Also complete (re-)merging of bank_statement_parse and
bank_statement_import_improve.
@oca-clabot
Copy link

Hey @NL66278, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

  • @NL66278 (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

@NL66278
Copy link
Contributor Author

NL66278 commented Mar 30, 2015

Please update the CLA database. I have sent in a signed CLA at least a year ago.

@NL66278
Copy link
Contributor Author

NL66278 commented Mar 30, 2015

@lmignon I looked into your code for bulk statement import and in itself it is very nice and clean. The problem is that is involves a rewrite of the base code (backported from master), splitting import_file and _import_file, that is very nice in itself too, but imcpmpatible with the idea of a backport.

@gurneyalex
Copy link
Member

@NL66278 the issue was a missing link between you github login and the CLA. I've now fixed this.

@lmignon
Copy link
Sponsor Contributor

lmignon commented Mar 30, 2015

@NL66278 Thank you for the review. A PR has been submitted on master for a refactoring of the import framework and is under review. odoo/odoo#5863. The same changes are ready to be proposed on OCA once the one on master will be merged. (https://github.com/acsone/bank-statement-import/tree/8.0-refactor-new-api)
The refactoring is required to avoid copy/paste of the code of the base class. I don't try to provide a replacement of the base import, just to enhance what is already done in the base import. An alternative would be to also redefine in my PR the import_file method but IMO it's very dangerous since the base implementation would not have been called and breaks modularity. (In your PR, what happens if an other addons has made changes to the import_file method?)
In my PR I try to provide a solution for one need: how to import a lot of bank statement files at once?
My answer is:

  • accept zipfile as input for statement import,
  • process each included file one by one by calling the normal import process in it's own transaction,
  • display a report if some files parts of the archive are not properly processed.
    With the given answer it's also possible to mix different formats of bank statement files in the same archive.

Regards,

lmi

@oca-clabot
Copy link

Hey @NL66278,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

@NL66278
Copy link
Contributor Author

NL66278 commented Apr 2, 2015

I proposed an alternative PR building on the work done by @lmignon.
See: #15

I will propose to use the zipfile functions and search functionality for bank accounts from Laurent, which were anyway very similiar to what I had in this PR, except that thanx to the refactoring that Laurent did it is no longer needed to overrule the whole base import.

@kvdb
Copy link

kvdb commented Apr 20, 2015

I'd like to develop a new bank-statement importer for the Dutch ING creditcard statements (https://www.ingbanknetservice.nl). However, I'm not sure on which base to build it. Is there already an agreement on it? Or when can that be expected?

@NL66278
Copy link
Contributor Author

NL66278 commented Apr 30, 2015

@kvdb No agreement has been reached. But I think the mainstream is to go with the new api version of the import framework. Unfortunately that has yet to be accepted by Odoo SA for master.

@sbidoul
Copy link
Member

sbidoul commented Apr 30, 2015

@NL66278 same point of view here.

We have been trying to contact @qdp-odoo to see if the branch can be merged in odoo master, but no luck so far.

@nbessi
Copy link
Contributor

nbessi commented May 19, 2015

Should we close in this PR ?

@lmignon lmignon mentioned this pull request May 19, 2015
@gfcapalbo
Copy link

Found conflicts, needs rebasing.

@NL66278
Copy link
Contributor Author

NL66278 commented Jun 11, 2015

As the approach based on the new api has been accepted by the community, I close this PR. Even though it still contains some extra features, but I will provide these based on the new api as well.

@NL66278 NL66278 closed this Jun 11, 2015
@NL66278 NL66278 deleted the 8.0_import_parsers branch October 2, 2015 08:35
zamberjo pushed a commit to aurestic/bank-statement-import that referenced this pull request Nov 15, 2019
yibudak pushed a commit to yibudak/bank-statement-import that referenced this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.