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

[10.0] mig bank statement import coda from 8.0 #54

Closed
wants to merge 23 commits into from

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Dec 5, 2017

No description provided.

lmignon and others added 21 commits December 5, 2017 09:58
The module is a rewrite of l10n_be_coda using the pycoda parser.
Use the Structured Communication as name else use the communication if it's provided else use '/'.
If the line is a details line use the communication from the related globalisation as name.
The module now depends of l10n_be_coda since the first one is automatically installed when installing l10n_be. The menuitem 'Import Coda File' from l10n_be_coda is  however removed to avoid confusion and force the use of the current module
…ote of statement line with the same transaction_ref
The declaration prevents the install of the module if the dependency is not met
…ty constraint with the demo data of l10n_be_coda.
this makes the test more robust (the previous version
did not work in presence of account_constraints, and
could not be run twice in a row on the same database)
@sbidoul sbidoul force-pushed the 10.0-mig-bank_statement_import_coda branch 2 times, most recently from d92bd46 to 33d9dd3 Compare December 5, 2017 19:37
@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage remained the same at 85.417% when pulling 33d9dd3 on acsone:10.0-mig-bank_statement_import_coda into bfdbfbf on OCA:10.0.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage remained the same at 85.417% when pulling 33d9dd3 on acsone:10.0-mig-bank_statement_import_coda into bfdbfbf on OCA:10.0.

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage remained the same at 85.417% when pulling 33d9dd3 on acsone:10.0-mig-bank_statement_import_coda into bfdbfbf on OCA:10.0.

@sbidoul sbidoul force-pushed the 10.0-mig-bank_statement_import_coda branch from 33d9dd3 to 22d285d Compare December 5, 2017 20:23
@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage remained the same at 88.55% when pulling 82ff425 on acsone:10.0-mig-bank_statement_import_coda into bfdbfbf on OCA:10.0.

@sbidoul
Copy link
Member Author

sbidoul commented Apr 11, 2018

@Olivier-LAURENT have you had a chance to test this?

Copy link

@Olivier-LAURENT Olivier-LAURENT left a comment

Choose a reason for hiding this comment

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

Reviewed and tested, thx for the migration. 👍

Copy link
Sponsor Contributor

@houssine78 houssine78 left a comment

Choose a reason for hiding this comment

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

@sbidoul I've just two small remarks which will be great for us to be adressed. Otherwise it could be merged.

if statements:
acc_number = statements[0].acc_number
currency = statement.currency
return currency, acc_number, vals_bank_statements
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

in our version we return return currency, self._get_acc_number(account_number), vals_bank_statements

this _get_acc_number function ensure that we have a defined journal for this account number.

def _get_acc_number(self, acc_number):
    #Check if we match the exact acc_number or the end of an acc number
    journal = self.env['account.journal'].search([('bank_acc_number', '=like', '%' + acc_number)])
    if not journal or len(journal) > 1: #if not found or ambiguious 
        return acc_number

return journal.bank_acc_number

if statement.new_balance_amount_sign == AmountSign.DEBIT:
balance_end_real = - balance_end_real
transactions = []
statement_date = statement.new_balance_date
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

why not using statement.creation_date?

@vdewulf
Copy link
Contributor

vdewulf commented Oct 2, 2018

Tested NOT OK with Triodos bank.
I imported a first CODA without any problem, with 6 transactions.
Then I tried to import the next CODA file. The system told me that the file was already imported.
I tried with another CODA file, it was partially imported (the system told me that some transactions were ignored, in fact the 6 first).

It seems that the unique reference construction should be improved. See houssine's feedback for the technical details.
Here are an extract of two CODA files that I tried to import:
image

'partner_name': line.counterparty_name or None,
'account_number': line.counterparty_number or None,
'note': self.get_st_line_note(line, information_dict),
'unique_import_id': line.ref + line.transaction_ref,
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

we get a problem here testing it with Triodos Coda.
the uniqe_import_id is too weak and led to ignore statement and or statement line import.
in our implementation we build the uniqe_import_id like this statement.coda_seq_number + '-' + statement.old_balance_date + '-' + statement.new_balance_date + '-' + move.ref

we also found that trasanction_ref returned from pycoda is in fact the bank reference from which the payment as been made. This reference is not mandatory and can be empty(N° de référence de la banque. Cette information est strictement informative)

@vdewulf
Copy link
Contributor

vdewulf commented Nov 6, 2018

@sbidoul Hello Stéphane. Is there any news regarding our test results on this PR?
Thanks in advance!

@vdewulf
Copy link
Contributor

vdewulf commented Jul 15, 2019

@sbidoul Hi Stéphane,
Is it possible for you to merge (even if the error we found is not fixed yet)?
So that we can migrate to v12 and propose a fix in another PR.
Thanks for your follow-up!
@houssine78 for information

@sbidoul
Copy link
Member Author

sbidoul commented Jul 15, 2019

@vdewulf I've backported the most changes from v11. Let's see if it's green.

/ocabot merge

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Rebased to 10.0-ocabot-merge-pr-54-by-sbidoul-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 15, 2019
Signed-off-by sbidoul
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 655de5c. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 10.0.

@houssine78
Copy link
Sponsor Contributor

thanks @sbidoul

@sbidoul sbidoul deleted the 10.0-mig-bank_statement_import_coda branch July 16, 2019 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants