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

Add processing step to the date format string so that we get Day of Month #27557

Conversation

adam-james-v
Copy link
Contributor

Fixes: #27469

Since the frontend sends a date-style string with various differences from how Java's Date Formatter expects things, we need to post-process some bits. The 'D' -> 'd' string replacement was missed in the post processing fn, and so in some cases it was missed altogether, causing this bug.

Existing tests in metabase.pulse.render.datetime-test and metabase.pulse.render.table-test should already catch this issue. Prior to the change they would fail, but they now pass after adding this PR's changes.

image

…onth

Since the frontend sends a date-style string with various differences from how Java's Date Formatter expects things,
we need to post-process some bits. The 'D' -> 'd' string replacement was missed in the post processing fn, and so in
some cases it was missed altogether, causing this bug.

Existing tests in `metabase.pulse.render.datetime-test` and `metabase.pulse.render.table-test` should already catch
this issue. Prior to the change they would fail, but they now pass after adding this PR's changes.
@adam-james-v adam-james-v added the backport Automatically create PR on current release branch on merge label Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 65.46% // Head: 65.21% // Decreases project coverage by -0.25% ⚠️

Coverage data is based on head (70a3188) compared to base (b232d6f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27557      +/-   ##
==========================================
- Coverage   65.46%   65.21%   -0.26%     
==========================================
  Files        3192     3188       -4     
  Lines       92720    92696      -24     
  Branches    11777    11769       -8     
==========================================
- Hits        60699    60451     -248     
- Misses      27152    27391     +239     
+ Partials     4869     4854      -15     
Flag Coverage Δ
back-end 85.42% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/metabase/pulse/render/datetime.clj 87.67% <100.00%> (+0.34%) ⬆️
...se/containers/DataPicker/PanePicker/PanePicker.tsx 0.00% <0.00%> (-100.00%) ⬇️
...iners/DataPicker/DataTypePicker/DataTypePicker.tsx 0.00% <0.00%> (-100.00%) ⬇️
...ers/DataPicker/RawDataPicker/RawDataPickerView.tsx 0.00% <0.00%> (-97.44%) ⬇️
...base/containers/DataPicker/DataPickerContainer.tsx 3.70% <0.00%> (-96.30%) ⬇️
...ataPicker/RawDataPicker/RawDataPickerContainer.tsx 0.00% <0.00%> (-88.89%) ⬇️
...ners/DataPicker/CardPicker/CardPickerContainer.tsx 0.00% <0.00%> (-88.00%) ⬇️
...tabase/containers/DataPicker/useDataPickerValue.ts 0.00% <0.00%> (-86.96%) ⬇️
...ontainers/DataPicker/CardPicker/CardPickerView.tsx 0.00% <0.00%> (-84.22%) ⬇️
.../metabase/containers/DataPicker/DataPickerView.tsx 0.00% <0.00%> (-76.48%) ⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adam-james-v adam-james-v enabled auto-merge (squash) January 6, 2023 21:48
@deploysentinel
Copy link

deploysentinel bot commented Jan 7, 2023

No failed tests 🎉

@adam-james-v adam-james-v merged commit 8fa8b18 into master Jan 9, 2023
@adam-james-v adam-james-v deleted the bugfix-static-viz-days-incorrectly-rendered-as-day-of-year-27469 branch January 9, 2023 23:10
github-actions bot pushed a commit that referenced this pull request Jan 9, 2023
…onth (#27557)

Since the frontend sends a date-style string with various differences from how Java's Date Formatter expects things,
we need to post-process some bits. The 'D' -> 'd' string replacement was missed in the post processing fn, and so in
some cases it was missed altogether, causing this bug.

Existing tests in `metabase.pulse.render.datetime-test` and `metabase.pulse.render.table-test` should already catch
this issue. Prior to the change they would fail, but they now pass after adding this PR's changes.
adam-james-v added a commit that referenced this pull request Jan 10, 2023
…onth (#27557) (#27600)

Since the frontend sends a date-style string with various differences from how Java's Date Formatter expects things,
we need to post-process some bits. The 'D' -> 'd' string replacement was missed in the post processing fn, and so in
some cases it was missed altogether, causing this bug.

Existing tests in `metabase.pulse.render.datetime-test` and `metabase.pulse.render.table-test` should already catch
this issue. Prior to the change they would fail, but they now pass after adding this PR's changes.

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong date format in static viz for tables
2 participants