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

[16.0][MIG] account_statement_import_online_ponto: Migration to 16.0 #554

Merged

Conversation

NL66278
Copy link
Contributor

@NL66278 NL66278 commented Jan 27, 2023

This PR includes the changes from #548

If that base PR is merged, this one can be rebased. Conversely when this branch is merged PR 548 can be closed.

@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch 2 times, most recently from ca1412a to 3db5d62 Compare January 30, 2023 16:26
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch from 1474034 to 49a3590 Compare February 14, 2023 12:30
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch 9 times, most recently from 5ce26cf to 6ac2db2 Compare March 16, 2023 00:05
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch 2 times, most recently from 8c59283 to 6c86e3a Compare March 16, 2023 23:34
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch from 6c86e3a to 5a9b71c Compare April 4, 2023 13:25
@StefanRijnhart
Copy link
Member

/ocabot migration account_statement_import_online_ponto

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Apr 19, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 19, 2023
13 tasks
@NL66278 NL66278 changed the title [16.0][MIG] account_statement_import_online_ponto: Migration to 16.0 [WIP][16.0][MIG] account_statement_import_online_ponto: Migration to 16.0 Jun 14, 2023
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch 2 times, most recently from 2cbefb1 to 178a565 Compare September 15, 2023 09:57
Comment on lines 150 to 181
def reset_transactions(date_since):
"""Reset values for new statement."""
statement_date_since = self._get_statement_date_since(date_since)
statement_date_until = (
statement_date_since + self._get_statement_date_step()
)
statement_lines = []
return statement_date_since, statement_date_until, statement_lines

lines = sorted(lines, key=itemgetter("transaction_datetime"))
(
statement_date_since,
statement_date_until,
statement_lines,
) = reset_transactions(lines[0]["transaction_datetime"])
for line in lines:
line.pop("transaction_datetime")
vals_line = self._ponto_get_transaction_vals(line)
if vals_line["date"] >= statement_date_until:
self._create_or_update_statement(
(statement_lines, {}), statement_date_since, statement_date_until
)
(
statement_date_since,
statement_date_until,
statement_lines,
) = reset_transactions(statement_date_until)
statement_lines.append(vals_line)
# Handle lines in last statement.
self._create_or_update_statement(
(statement_lines, {}), statement_date_since, statement_date_until
)
Copy link
Member

Choose a reason for hiding this comment

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

After testing this module with real data, we noticed that a few lines were retrieved from ponto but were not imported in odoo. The reason seems to be that the date of the lines not always falls back into the date range (date_since - date_until) and so they are filtered out. I had difficulties to fix this method so I ended up with rewriting this method and simplifying the code this way:

    def _ponto_store_lines(self, lines):
        """Store transactions retrieved from Ponto in statements.
        """
        lines = sorted(lines, key=itemgetter("transaction_datetime"))

        # Group statement lines by date per period (date range)
        grouped_periods = {}
        for line in lines:
            date_since = line["transaction_datetime"]
            statement_date_since = self._get_statement_date_since(date_since)
            statement_date_until = statement_date_since + self._get_statement_date_step()
            if (statement_date_since, statement_date_until) not in grouped_periods:
                grouped_periods[(statement_date_since, statement_date_until)] = []

            line.pop("transaction_datetime")
            vals_line = self._ponto_get_transaction_vals(line)
            grouped_periods[(statement_date_since, statement_date_until)].append(vals_line)

        # For each period, create or update statement lines
        for period, statement_lines in grouped_periods.items():
            (statement_date_since, statement_date_until) = period

            self._create_or_update_statement(
                (statement_lines, {}), statement_date_since, statement_date_until
            )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@astirpe Hi, could you make a PR along these lines to my branch? Just to be sure I updated things like intended. Anyway, thank you for your contribution!

@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch from 17bd08f to 3d49a70 Compare September 25, 2023 10:22
@NL66278 NL66278 changed the title [WIP][16.0][MIG] account_statement_import_online_ponto: Migration to 16.0 [16.0][MIG] account_statement_import_online_ponto: Migration to 16.0 Sep 25, 2023
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Suggestions:

Copy link
Contributor

@ThijsvOers ThijsvOers left a comment

Choose a reason for hiding this comment

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

I have tested the Module:

  1. created MyPonto account
  2. added Odoo integration
  3. in Odoo Added online bank sync. Important notice: use Clientid from Myponto in field Login in Odoo
  4. Tested with Daily, weekly and Monthly statement, all lines were retreived complete (this was an issue before the lastest changes)
  5. Field Partner is now also filled in in odoo, this makes the reconcile process better.

@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch 2 times, most recently from d00405d to bc9cada Compare September 29, 2023 11:33
NL66278 and others added 12 commits October 18, 2023 09:40
1. Find partner if ther already is a bank account with the right IBAN;
2. Store (and display) raw import data to help in problem determination;
3. Make it easy to extend the parsing of import data;
4. Apply some clean coding principles.
Currently translated at 100.0% (16 of 16 strings)

Translation: bank-statement-import-14.0/bank-statement-import-14.0-account_statement_import_online_ponto
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-14-0/bank-statement-import-14-0-account_statement_import_online_ponto/ca/
The 2 modules account_statement_import_online and
account_statement_import depend on account_statement_import_base (and
not on each other) and share common code, in particular a hook to update
the statement line. So we can now have reconciliation modules that use
this hook and therefore work both on file import and online import. More
details on OCA#481.

Improve bank statement line form view and journal form view.
Separate retrieval of data from ponto (buffer data) and
creation of statements.
Add debug messages
Set raw_data in bank statement lines
With Ponto we can only retrieve data backwards in time. Therefore
we override the normal _pull method to create statements in descending
order of statement date.
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch from 07d0791 to 56880d1 Compare October 18, 2023 07:40
@NL66278 NL66278 force-pushed the 16.0-mig-account_statement_import_online_ponto branch from 56880d1 to bda6315 Compare October 18, 2023 07:54
@NL66278
Copy link
Contributor Author

NL66278 commented Oct 18, 2023

@victoralmau I did the rebase and a check. Found one deprecated text in the help for a field. Everything in order now.

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Code review OK

@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). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-554-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7d48088 into OCA:16.0 Oct 18, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

victoralmau pushed a commit to Tecnativa/bank-statement-import that referenced this pull request Apr 4, 2024
Signed-off-by pedrobaeza
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.