8000 fix: Chart date format on x-axis is inaccurate by shariquerik · Pull Request #18191 · frappe/frappe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Chart date format on x-axis is inaccurate #18191

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 21, 2022
Merged

fix: Chart date format on x-axis is inaccurate #18191

merged 6 commits into from
Sep 21, 2022

Conversation

shariquerik
Copy link
Member

Issue:
The date format of some dates on the x-axis of the chart is inaccurate as you can see in the below screenshots.

Before:
image

After:
image

@shariquerik shariquerik requested review from a team and phot0n and removed request for a team September 20, 2022 08:16
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Sep 20, 2022
@shariquerik shariquerik added frappe-support and removed add-test-cases Add test case to validate fix or enhancement labels Sep 20, 2022
@phot0n
Copy link
Contributor
phot0n commented Sep 20, 2022 •
edited
Loading

format_date was added there to show dates according to system settings as previously dashboard charts showed dates in a constant format (ref: #17151)

Keeping day_first=True would start parsing things wrongly as it will fail to show correct dates for formats such as mm/dd/yyyy

@shariquerik
Copy link
Member Author
shariquerik commented Sep 20, 2022 •
edited
Loading

Keeping day_first=True would start parsing things wrongly as it will fail to show correct dates for formats such as mm/dd/yyyy

I have tested with all the formats, it is working fine
image

day_first=True is only used to get the correct format of the date_string used in the format_date method. After that, the format is changed based on the system date format set.

@shariquerik
Copy link
Member Author

Unit tests are failing I will check them.

@phot0n
Copy link
Contributor
phot0n commented Sep 20, 2022 •
edited
Loading

day_first=True is only used to get the correct format of the date_string used in the format_date method. After that, the format is changed based on the system date format set.

ah okay yes, it should work fine as we do format it to dd-mm-yy before sending it to format_date function (in the get_period function)

frappe/frappe/utils/dateutils.py

Lines 144 to 145 in c204fa7

"Daily": date.strftime("%d-%m-%y"),
"Weekly": date.strftime("%d-%m-%y"),

@phot0n
Copy link
Contributor
phot0n commented Sep 20, 2022 •
edited
Loading

Reason why this mess is happening is because parser lib, defaults to mdy format and only starts considering other formats if the first element crosses 12 or 31

https://github.com/dateutil/dateutil/blob/7c5c98959bd0e9eea3c55fdf78bef3ae5b8c995e/src/dateutil/parser/_parser.py#L550-L563

@codecov
Copy link
codecov bot commented Sep 20, 2022 •
edited
Loading

Codecov Report

Merging #18191 (a253290) into develop (c204fa7) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #18191      +/-   ##
===========================================
- Coverage    62.40%   62.18%   -0.22%     
===========================================
  Files          744      744              
  Lines        68396    68882     +486     
  Branches      5962     5962              
===========================================
+ Hits         42680    42833     +153     
- Misses       22137    22470     +333     
  Partials      3579     3579
Flag Coverage Δ
server-postgres 66.78% <100.00%> (-0.01%) ⬇️

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

phot0n
phot0n reviewed Sep 21, 2022
View reviewed changes
@phot0n
Copy link
Contributor
phot0n commented Sep 21, 2022

Unrelated tests failing. merging - hopefully things don't break 🤞

@phot0n phot0n merged commit 5cfcc4a into frappe:develop Sep 21, 2022
@sagarvora
Copy link
Contributor

We did this in India Compliance a while ago. The date we were getting was in the dd-mm-yyyy format, but dateutil parsed it in mm-dd-yyyy format.

https://github.com/resilient-tech/india-compliance/blob/116092d8b100468bb4fa3532c5b8919218925e61/india_compliance/gst_india/utils/__init__.py#L358

Guess we can use the Frappe API now. Thanks @shariquerik.

@mergify mergify bot mentioned this pull request Sep 22, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 22, 2022
@shariquerik @mergify
fix: Chart date format on x-axis is inaccurate (#18191)
a5144fb 
(cherry picked from commit 5cfcc4a)

# Conflicts:
#	frappe/utils/data.py
@mergify mergify bot mentioned this pull request Sep 22, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 22, 2022
@shariquerik @mergify
fix: Chart date format on x-axis is inaccurate (#18191)
7cd2b1b 
(cherry picked from commit 5cfcc4a)
ankush pushed a commit that referenced this pull request Sep 28, 2022
@mergify @shariquerik
fix: Chart date format on x-axis is inaccurate (#18191) (#18207)
f940c7b 
(cherry picked from commit 5cfcc4a)

Co-authored-by: Shariq Ansari <30859809+shariquerik@users.noreply.github.com>
frappe-pr-bot pushed a commit that referenced this pull request Oct 4, 2022
@frappe-bot
chore(release): Bumped to Version 13.41.4
f560344 
## [13.41.4](v13.41.3...v13.41.4) (2022-10-04)

### Bug Fixes

* Chart date format on x-axis is inaccurate ([#18191](#18191)) ([a5144fb](a5144fb))
* Chart filter not working if not operator is used (backport [#18194](#18194)) ([#18264](#18264)) ([a04436f](a04436f))
* handle isatty correctly (backport [#18277](#18277)) ([#18279](#18279)) ([3516dc4](3516dc4))
* only allow verified_command on GET requests ([#18235](#18235)) ([#18237](#18237)) ([3d2d07b](3d2d07b))
* remove return statement from override_email_send hook logic ([909fe12](909fe12))
* throw exception if backup failed ([#18230](#18230)) ([#18254](#18254)) ([96bae57](96bae57))
* typeError ([281d14d](281d14d))

### Performance Improvements

* use `modified` instead of `creation` in scheduler ([#18234](#18234)) ([#18239](#18239)) ([847b30e](847b30e))
frappe-pr-bot pushed a commit that referenced this pull request Oct 4, 2022
@frappe-bot
chore(release): Bumped to Version 14.10.0
ea29632 
# [14.10.0](v14.9.0...v14.10.0) (2022-10-04)

### Bug Fixes

* add translate function to some strings (backport [#18188](#18188)) ([#18242](#18242)) ([c929fe6](c929fe6))
* Chart date format on x-axis is inaccurate ([#18191](#18191)) ([#18207](#18207)) ([f940c7b](f940c7b))
* Chart filter not working if not operator is used (backport [#18194](#18194)) ([#18265](#18265)) ([775ed71](775ed71))
* **Data Import:** don't validate empty values ([#17923](#17923)) ([#18275](#18275)) ([26bfbf0](26bfbf0))
* dont assume issingle exists ([#18236](#18236)) ([#18252](#18252)) ([6164921](6164921))
* geolocation ([#18250](#18250)) ([#18282](#18282)) ([3e70643](3e70643))
* handle isatty correctly ([#18277](#18277)) ([#18278](#18278)) ([7ce396e](7ce396e))
* ignore child tables when init-ing parent doc ([84a58b3](84a58b3))
* **minor:** More button in list view ([#18283](#18283)) ([#18284](#18284)) ([99b8bbd](99b8bbd))
* only allow verified_command on GET requests ([#18235](#18235)) ([#18238](#18238)) ([d974143](d974143))
* report generation - frappe monitor ([#18259](#18259)) ([#18260](#18260)) ([d860e20](d860e20))
* throw exception if backup failed ([#18230](#18230)) ([#18255](#18255)) ([0f71fd2](0f71fd2))

### Features

* Added Subscription Banner for remotely logging into FrappeCloud dashboard from site ([#18263](#18263)) ([68f315d](68f315d))

### Performance Improvements

* use `modified` instead of `creation` in scheduler ([#18234](#18234)) ([#18240](#18240)) ([6245b91](6245b91))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@phot0n phot0n phot0n approved these changes

Assignees
No one assigned
Labels
backport version-14-hotfix backport to version 14 frappe-support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shariquerik @phot0n @sagarvora
0