[16.0][IMP] account_statement_import_online: Pull wizard with dates and one fetch

[pedrobaeza]

This commit improves the provider pull wizard in 2 ways:

• It asks for dates instead of datetimes, as at the end, the minimum pull unit is the whole day, and it was very confusing for the user to select times without knowing that behind the scenes, the whole day will be pulled. Now, the user selects only the day. Quicker and cleaner...
• It includes a check "One fetch", that instructs the pull method to ask to the provider the whole interval in one shot. This way, you don't consume the request rate limits for the providers with them, or only due to speed purposes. It's checked by default, as it's the more natural way of fetching an interval on demand.

@Tecnativa TT55399

[OCA-git-bot]

Hi @alexey-pelykh,
some modules you are maintaining are being modified, check this out!

[alexey-pelykh]

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

[pedrobaeza]

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.

[alexey-pelykh]

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.

[pedrobaeza]

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.

[alexey-pelykh]

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

[pedrobaeza]

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?

[alexey-pelykh]

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

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[victoralmau]

IMO this change is coherent.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[alexey-pelykh]

Review: account_statement_import_online Pull Wizard Improvements

Thanks for the contribution, @pedrobaeza. The Date-vs-Datetime UX improvement is valuable. However, the one_fetch mechanism raises architectural concerns that merit further discussion before merging.

What works well

• Date fields instead of Datetime: Good UX improvement. Users selecting dates is more intuitive when the minimum pull granularity is a full day.
• _get_datetimes() helper: Clean conversion method with proper min.time() / max.time() boundaries.
• Ponto override propagation: The _pull signature change is correctly forwarded through super() in the Ponto provider.

Design concerns with one_fetch

1. Bypasses _get_statement_date_since: When one_fetch=True, the method overrides statement_date_since = date_since at line 194, skipping the provider-specific logic in _get_statement_date_since() that may adjust the start date (e.g. for providers that need alignment to statement boundaries). This silent override could produce unexpected results for providers that rely on that adjustment.

2. Bypasses _get_statement_date_step grouping: When one_fetch=True, statement_date_until = date_until at line 208 means a single call to _obtain_statement_data for the entire period. The resulting statement will cover the full date range regardless of statement_creation_mode (daily/weekly/monthly). This is a side effect -- the user configures daily statements but gets one big statement when pulling interactively.

3. Timezone concern in _get_datetimes(): The method produces naive datetimes via datetime.combine(date, datetime.min.time()). When one_fetch=True, the normal path through _get_statement_date_since (which handles timezone-aware date alignment) is skipped. This could cause subtle off-by-one-day issues for providers in non-UTC timezones.

4. Default True is risky: Defaulting one_fetch=True means the wizard will by default bypass the carefully designed step-based iteration for all providers, including future ones that may depend on it. A provider author implementing _get_statement_date_step to return 30 days (because their API limits requests to 30-day windows) would find that the wizard silently ignores their constraint.

Alternative approach suggestion

As discussed in the earlier review thread, a cleaner architecture might be:

• Add _get_statement_date_step_max() returning None by default (unlimited)
• Providers with API limits (PayPal, Wise) override it to their max window
• The fetch loop uses _get_statement_date_step_max for fetching but still creates statements per statement_creation_mode

This separates the two concerns: fetch granularity (how many API calls) vs statement grouping (how transactions are organized).

Test coverage

The GoCardless test is updated to set one_fetch=False, preserving existing behavior. However, there is no test exercising the one_fetch=True path to verify it actually works correctly (single fetch, statement boundaries, etc.).

Verdict

The Date field change is ready to merge on its own. The one_fetch mechanism needs design refinement to avoid bypassing provider-specific constraints and statement grouping configuration. Consider splitting this into two PRs: one for the Date UX improvement, and one for the fetch optimization after the architectural approach is agreed upon.

Review posted via CorporateHub OCA review campaign

[alexey-pelykh]

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.

[alexey-pelykh]

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.

[alexey-pelykh]

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.