Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -188,20 +188,24 @@ def _compute_update_schedule(self):
)[0][1],
}

def _pull(self, date_since, date_until):
def _pull(self, date_since, date_until, one_fetch=False):
"""Pull data for all providers within requested period."""
is_scheduled = self.env.context.get("scheduled")
debug = self.env.context.get("account_statement_online_import_debug")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Design concern: This overrides _get_statement_date_since() which providers may use to align the start date to statement boundaries (e.g., start of week/month for weekly/monthly statement creation modes). Bypassing it means the fetched data may not align with the provider's expected statement boundaries.

The _get_statement_date_since method exists to allow providers to customize this behavior -- silently skipping it via a boolean flag breaks that contract.

debug_data = []
for provider in self:
statement_date_since = provider._get_statement_date_since(date_since)
if one_fetch:
statement_date_since = date_since
while statement_date_since < date_until:
# Note that statement_date_until is exclusive, while date_until is
# inclusive. So if we have daily statements date_until might
# be 2020-01-31, while statement_date_until is 2020-02-01.
statement_date_until = (
statement_date_since + provider._get_statement_date_step()
)
if one_fetch:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How checking this by default is compatible with providers that limit fetch to X days per fetch?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can you put any example of that limit? All that I know don't have such limit. And anyway, if you put a big interval, you get in return the message error if so, so you can limit your interval in the wizard. Remember that this is only for the interactive pull wizard.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

E.g. PayPal - one year only, Wise.com - one year only. User may as well select 5 years when importing for the first time on new setup. Such providers should set the step to 1 year making the one_step setting irrelevant.

Obviously you're aiming to fix experience for some specific providers - what step do they set? Since if they set one day but can handle much more so that one_fetch works - those providers' step should be fixed and not an override added.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, and if they do that, an error will be arisen from the Paypal provider, so I don't see any problem. Instead, with the default value that is daily, they will make 365*5 = 1825 requests, taking a lot of time and make them to think that it's hung.

We can put a warning if you prefer saying that the provider may not accept the petition in one block, but what is not sane is to not do it by default.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's use just one of the two threads :D why adding statement_creation_mode like all was not an option?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because you want to do this only once, not for every automatic pull that happens once you do the initial pulling, that is only needed to be updated. Another possible use is any temporary problem that prevented to download an interval, and in such case, you also want only one pull, not one by one. Example: during a week, you have invalid credentials. Once you put the valid ones, having create statements as daily, only last day will be downloaded, so you click on the pull wizard and download the last week. Why would you want to do this by default through 7 requests instead of just one?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's pause for a second to investigate the idea behind statement_creation_mode having day/week/month:
By configuring statement_creation_mode on the provider to day/week/month we control how many statements area created, or in other words - how transactions are grouped.

If I got your argument for one_fetch correctly, it is proposed to solve the API rate limiting. As a side effect it overrides the statement_creation_mode, creating a single statement from all. I definitely see issue with that.
In your example, with daily setting, and one_fetch checked - one statement covering entire period will be generated.

I fully acknowledge the API rate limiting issue that may arise for some providers when you pull yearly data with day granularity. However I strongly disagree with the proposed approach.

As a counter-proposal, I'd suggest:

  • adding _get_statement_date_step_max() (None by default) so that providers like PayPal may set it to 365 days.
  • refactoring the code to use _get_statement_date_step_max (effectively hardcoding one_fetch behavior) for fetch
  • refactoring code to step through the downloaded data according to statement_creation_mode

that would allow to fetch data in minimal amount of requests (one for some providers, couple for providers like PayPal) while respecting the statement grouping granularity of day/week/month

statement_date_until = date_until
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Statement grouping side effect: Setting statement_date_until = date_until here means a single iteration of the while loop, producing one statement for the entire period. This overrides statement_creation_mode (daily/weekly/monthly) -- the user configures daily statements but gets a single multi-day statement when using the wizard with one_fetch checked.

Fetch optimization and statement grouping are separate concerns that should not be coupled through a single flag.

try:
data = provider._obtain_statement_data(
statement_date_since, statement_date_until
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).

import pprint
from datetime import datetime

from odoo import fields, models

Expand All @@ -12,15 +13,13 @@ class OnlineBankStatementPullWizard(models.TransientModel):
_name = "online.bank.statement.pull.wizard"
_description = "Online Bank Statement Pull Wizard"

date_since = fields.Datetime(
string="From",
required=True,
default=fields.Datetime.now,
)
date_until = fields.Datetime(
string="To",
required=True,
default=fields.Datetime.now,
date_since = fields.Date(string="From", required=True, default=fields.Date.today)
Comment thread
alexey-pelykh marked this conversation as resolved.
date_until = fields.Date(string="To", required=True, default=fields.Date.today)
one_fetch = fields.Boolean(
string="One fetch",
help="If checked, retrieve the whole interval in only one request to the "
"provider. This is useful if the provider sets request limits.",
default=True,
Comment thread
pedrobaeza marked this conversation as resolved.
)

def _get_provider(self):
Expand All @@ -35,21 +34,31 @@ def _get_provider(self):
provider = active_record
return provider

def _get_datetimes(self):
"""Return the datetime values for the entered dates, including whole starting
and ending days.
"""
self.ensure_one()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Timezone concern: datetime.combine(self.date_since, datetime.min.time()) produces a naive datetime (no tzinfo). When one_fetch=True, this value is used directly as date_since in _pull, bypassing _get_statement_date_since() which normally handles date alignment. For providers operating in non-UTC timezones, this could cause off-by-one-day boundary issues.

Consider using fields.Datetime.to_datetime() or applying the provider's timezone to ensure consistency with the normal code path.

since = datetime.combine(self.date_since, datetime.min.time())
until = datetime.combine(self.date_until, datetime.max.time())
return since, until

def action_pull(self):
"""Pull statements from provider and then show list of statements."""
provider = self._get_provider()
provider._pull(self.date_since, self.date_until)
since, until = self._get_datetimes()
provider._pull(since, until, one_fetch=self.one_fetch)
action = self.env.ref("account.action_bank_statement_tree").sudo().read([])[0]
action["domain"] = [("journal_id", "=", provider.journal_id.id)]
return action

def action_debug(self):
"""Pull statements in debug and show result."""
provider = self._get_provider().with_context(
active_test=False,
account_statement_online_import_debug=True,
active_test=False, account_statement_online_import_debug=True
)
data = provider._pull(self.date_since, self.date_until)
since, until = self._get_datetimes()
data = provider._pull(since, until, one_fetch=self.one_fetch)
wizard = self.env["online.bank.statement.pull.debug"].create(
{"data": pprint.pformat(data)}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<group name="main">
<field name="date_since" />
<field name="date_until" />
<field name="one_fetch" />
</group>
<footer>
<button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def test_mocked_gocardless(self):
vals = {
"date_since": "2020-10-30",
"date_until": "2020-11-11",
"one_fetch": False,
}
wizard = (
self.env["online.bank.statement.pull.wizard"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def _get_available_services(self):
("ponto", "MyPonto.com"),
]

def _pull(self, date_since, date_until):
def _pull(self, date_since, date_until, one_fetch=False):
"""For Ponto the pulling of data will not be grouped by statement.

Instead we will pull data from the last available backwards.
Expand All @@ -52,7 +52,7 @@ def _pull(self, date_since, date_until):
debug = self.env.context.get("account_statement_online_import_debug")
debug_data = []
data = super(OnlineBankStatementProvider, self - ponto_providers)._pull(
date_since, date_until
date_since, date_until, one_fetch=one_fetch
)
if debug:
debug_data += data
Expand Down