Skip to content

[IMP] pivot: insert pivot with real date #5943

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

Closed
wants to merge 2 commits into from

Conversation

LucasLefevre
Copy link
Collaborator

Purpose

Currently, when you group a pivot by day and insert it in a spreadsheet, the generated pivot functions look like

=PIVOT.VALUE(2,"amount_total","date_order:day","06/01/2023")

"06/01/2023" is ambiguous. Is it the 6th January 2023 or the 1st June 2023 ? It turns out it's always interpreted as a US date (mm/dd/yyyy): 1st June 2023. Even if the spreadsheet locale is FR (dd/mm/yyyy). If the locale is FR, it's inconsistent then: if you type "06/01/2023" in a cell, it's interpreted with the FR locale as 6th January 2023, but specifically in PIVOT functions, it's interpreted as 1st June 2023. The reason is historic: before the introduction of locales, everything was US, always. When we introduced locales, we left the way it was interpreted in PIVOT functions to avoid breaking existing spreadsheet when changing the locale)

Alternatives

  • Interpret the string in PIVOT functions with the spreadsheet locale. This is a breaking change though. We could upgrade spreadsheets for simple cases, but not if the date comes from a reference to another cell, with an arbitrary complex construction with functions. Changing the locale would also break all pivots with hard-coded date string.
  • Support only "real" dates in PIVOT functions (DATE(2023, 6, 1) instead of the hardcoded strings). It means no more ambiguity and no inconsistencies anymore. But it's a breaking change and we can't upgrade all existing spreadsheets (same issue as above). The benefit compared to the alternative above is that dates that we would not be properly upgraded would now result in errors, allowing the user to spot the issue and fix it.
  • Insert functions with string dates with the year first "2023/06/01". Those dates are always interpreted as yyyy/mm/dd which makes them unambiguous, no matter the spreadsheet locale. If the user manually types a PIVOT function with "06/01/2023", we keep the current behavior and interpret it has US. This ensure there's no breaking change.
  • Same as above but insert functions with DATE(2023, 6, 1). Preserve the current behavior and interpret hard-coded date strings as US.

The last two options are preferred to avoid any breaking change. We choose the last one to encourage users to use DATE(...) and avoid hard-coded date strings in formulas, which don't work well when changing the spreadsheet locale.

Task: 3973659

Description:

description of this task, what is implemented and why it is implemented that way.

Task: TASK_ID

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Mar 17, 2025

Pull request status dashboard

Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

woop woop

const date = toNumber(normalizedValue, DEFAULT_LOCALE);
return `"${formatValue(date, { locale: DEFAULT_LOCALE, format: "mm/dd/yyyy" })}"`;
const jsDate = toJsDate(normalizedValue, DEFAULT_LOCALE);
return `DATE(${jsDate.getFullYear()}, ${jsDate.getMonth() + 1}, ${jsDate.getDate()})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: there's no space at all inside generated PIVOT.VALUE formulas ... except here. We should stay consistent.

Purpose
-------
Currently, when you group a pivot by day and insert it in a spreadsheet,
the generated pivot functions look like

=PIVOT.VALUE(2,"amount_total","date_order:day","06/01/2023")

"06/01/2023" is ambiguous. Is it the 6th January 2023 or the 1st June 2023 ?
It turns out it's always interpreted as a US date (mm/dd/yyyy): 1st June 2023.
Even if the spreadsheet locale is FR (dd/mm/yyyy). If the locale is FR, it's
inconsistent then: if you type "06/01/2023" in a cell, it's interpreted with
the FR locale as 6th January 2023, but specifically in PIVOT functions,
it's interpreted as 1st June 2023. The reason is historic: before the
introduction of locales, everything was US, always. When we introduced
locales, we left the way it was interpreted in PIVOT functions to avoid
breaking existing spreadsheet when changing the locale)

Alternatives

- Interpret the string in PIVOT functions with the spreadsheet locale.
  This is a breaking change though. We could upgrade spreadsheets for simple
  cases, but not if the date comes from a reference to another cell, with an
  arbitrary complex construction with functions. Changing the locale would
  also break all pivots with hard-coded date string.
- Support only "real" dates in PIVOT functions (DATE(2023, 6, 1) instead of
  the hardcoded strings). It means no more ambiguity and no inconsistencies
  anymore. But it's a breaking change and we can't upgrade all existing
  spreadsheets (same issue as above). The benefit compared to the
  alternative above is that dates that we would not be properly upgraded
  would now result in errors, allowing the user to spot the issue and fix it.
- Insert functions with string dates with the year first "2023/06/01". Those
  dates are always interpreted as yyyy/mm/dd which makes them unambiguous,
  no matter the spreadsheet locale. If the user manually types a PIVOT
  function with "06/01/2023", we keep the current behavior and interpret it
  has US. This ensure there's no breaking change.
- Same as above but insert functions with DATE(2023, 6, 1). Preserve the
  current behavior and interpret hard-coded date strings as US.

The last two options are preferred to avoid any breaking change. We choose
the last one to encourage users to use DATE(...) and avoid hard-coded date
strings in formulas, which don't work well when changing the spreadsheet
locale.

Task: 3973659
Introducing "le bug de l'an 3000".

Currently, =PIVOT.VALUE(1, "measure", "date:year", DATE(2025,1,1)) produces an
unexpected result because DATE(2025,1,1) evaluates to 45658, which is
mistakenly interpreted as the year 45658—a time when we’ll all be long gone.

To fix this, we now assume that any numeric value above 3000 is an invalid
year and should be treated as a full date.

Task: 3973659
@LucasLefevre LucasLefevre force-pushed the master-bye-date-strings-lul branch from a8bdcd6 to 7930e9f Compare March 21, 2025 09:21
@VincentSchippefilt
Copy link
Collaborator

robodoo r+

@robodoo
Copy link
Collaborator

robodoo commented Apr 3, 2025

@LucasLefevre @VincentSchippefilt because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@LucasLefevre
Copy link
Collaborator Author

robodoo rebase-ff

@robodoo
Copy link
Collaborator

robodoo commented Apr 3, 2025

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Apr 3, 2025
Purpose
-------
Currently, when you group a pivot by day and insert it in a spreadsheet,
the generated pivot functions look like

=PIVOT.VALUE(2,"amount_total","date_order:day","06/01/2023")

"06/01/2023" is ambiguous. Is it the 6th January 2023 or the 1st June 2023 ?
It turns out it's always interpreted as a US date (mm/dd/yyyy): 1st June 2023.
Even if the spreadsheet locale is FR (dd/mm/yyyy). If the locale is FR, it's
inconsistent then: if you type "06/01/2023" in a cell, it's interpreted with
the FR locale as 6th January 2023, but specifically in PIVOT functions,
it's interpreted as 1st June 2023. The reason is historic: before the
introduction of locales, everything was US, always. When we introduced
locales, we left the way it was interpreted in PIVOT functions to avoid
breaking existing spreadsheet when changing the locale)

Alternatives

- Interpret the string in PIVOT functions with the spreadsheet locale.
  This is a breaking change though. We could upgrade spreadsheets for simple
  cases, but not if the date comes from a reference to another cell, with an
  arbitrary complex construction with functions. Changing the locale would
  also break all pivots with hard-coded date string.
- Support only "real" dates in PIVOT functions (DATE(2023, 6, 1) instead of
  the hardcoded strings). It means no more ambiguity and no inconsistencies
  anymore. But it's a breaking change and we can't upgrade all existing
  spreadsheets (same issue as above). The benefit compared to the
  alternative above is that dates that we would not be properly upgraded
  would now result in errors, allowing the user to spot the issue and fix it.
- Insert functions with string dates with the year first "2023/06/01". Those
  dates are always interpreted as yyyy/mm/dd which makes them unambiguous,
  no matter the spreadsheet locale. If the user manually types a PIVOT
  function with "06/01/2023", we keep the current behavior and interpret it
  has US. This ensure there's no breaking change.
- Same as above but insert functions with DATE(2023, 6, 1). Preserve the
  current behavior and interpret hard-coded date strings as US.

The last two options are preferred to avoid any breaking change. We choose
the last one to encourage users to use DATE(...) and avoid hard-coded date
strings in formulas, which don't work well when changing the spreadsheet
locale.

Task: 3973659
Part-of: #5943
Signed-off-by: Vincent Schippefilt (vsc) <[email protected]>
robodoo pushed a commit that referenced this pull request Apr 3, 2025
Introducing "le bug de l'an 3000".

Currently, =PIVOT.VALUE(1, "measure", "date:year", DATE(2025,1,1)) produces an
unexpected result because DATE(2025,1,1) evaluates to 45658, which is
mistakenly interpreted as the year 45658—a time when we’ll all be long gone.

To fix this, we now assume that any numeric value above 3000 is an invalid
year and should be treated as a full date.

closes #5943

Task: 3973659
Signed-off-by: Vincent Schippefilt (vsc) <[email protected]>
@robodoo robodoo added the 18.3 label Apr 3, 2025
@robodoo robodoo closed this Apr 3, 2025
@fw-bot fw-bot deleted the master-bye-date-strings-lul branch April 10, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants