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

[IMP] account_statement_import_camt: look for currency under Ntry #460

Conversation

StefanRijnhart
Copy link
Member

Encountered in a Camt.054 statement: currency listed under
/BkToCstmrDbtCdtNtfctn/Ntfctn/Ntry/Amt/@ccy

Encountered in a Camt.054 statement: currency listed under
/BkToCstmrDbtCdtNtfctn/Ntfctn/Ntry/Amt/@ccy
@StefanRijnhart StefanRijnhart added this to the 14.0 milestone Apr 11, 2022
@TeoGoddet
Copy link
Contributor

See #426 and #388
Issue #387 also

Copy link

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

Because of the added test I would favor this PR instead of #388 388
EDIT: the #426 seems also complete... I haven't enough skills on this addon to know which one is better. Some PSC members could check?

@anedisi
Copy link

anedisi commented Apr 11, 2022

@sebalix those two are independent, this should come on top of that @TeoGoddet from #426

@StefanRijnhart
Copy link
Member Author

I'll have a look at #426 tomorrow to see if it obsoletes this change.

@StefanRijnhart
Copy link
Member Author

When I run #426 with the test from this PR, the test fails on

2022-04-12 06:47:22,585 1852340 ERROR 14camt odoo.addons.account_statement_import_camt.tests.test_import_bank_statement: ERROR: TestParser.test_parse_camt054
Traceback (most recent call last):
  File "//home/odoo/14.0/parts/bank-statement-import/account_statement_import_camt/tests/test_import_bank_statement.py", line 50, in test_parse_camt054
    self._do_parse_test("test-camt054", "golden-camt054.pydata")
  File "//home/odoo/14.0/parts/bank-statement-import/account_statement_import_camt/tests/test_import_bank_statement.py", line 27, in _do_parse_test
    res = self.parser.parse(data.read())
  File "//home/odoo/14.0/parts/bank-statement-import/account_statement_import_camt/models/parser.py", line 335, in parse
    statement = self.parse_statement(ns, node)
  File "//home/odoo/14.0/parts/bank-statement-import/account_statement_import_camt/models/parser.py", line 286, in parse_statement
    transactions.extend(self.parse_entry(ns, entry_node, result["currency"]))
KeyError: 'currency'

When I apply the single line fix from this PR, all tests succeed again. So the PRs are unrelated.

Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

:LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@yvaucher
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-460-by-yvaucher-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit cade3fb into OCA:14.0 Apr 21, 2022
@OCA-git-bot
Copy link
Contributor

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

LudLaf pushed a commit to Tecnativa/bank-statement-import that referenced this pull request May 4, 2022
Signed-off-by yvaucher
@StefanRijnhart
Copy link
Member Author

Port to 15: #466

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.

7 participants