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

fix: consider user selected timezone while exporting the reports #707

Conversation

metapraveen
Copy link
Contributor

problem:
Time in CSV exported reports are always in UTC timezone irrespective of user
selected timezone in the UI. There are use case when time in the exported
reports should be other timezone than UTC.

fix:
plywood passes the user selected timezone to format function here
https://github.com/implydata/plywood/blob/master/src/datatypes/dataset.ts#L343
We can use that to get time in the user selected timezone. The time in reports
will also have information about timezone because of ISO format.

@adrianmroz-allegro adrianmroz-allegro added the enhancement New feature label Feb 16, 2021
@metapraveen metapraveen force-pushed the consider-user-selected-timezone-while-exporting branch from 79532d4 to 62f82c0 Compare February 16, 2021 15:21
Copy link
Contributor

@adrianmroz-allegro adrianmroz-allegro left a comment

Choose a reason for hiding this comment

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

I'm very picky when it comes to naming constants :)

We will discuss internally if we want this change, give us some time.

src/common/utils/time/time.ts Outdated Show resolved Hide resolved
src/common/utils/time/time.ts Outdated Show resolved Hide resolved
@adrianmroz-allegro
Copy link
Contributor

FYI:

> moment().format("YYYY-MM-DDTHH:mm:ss.sssZ")
"2021-02-16T17:10:28.2828+01:00"

problem:
Time in CSV exported reports are always in UTC timezone irrespective of user
selected timezone in the UI. There are use case when time in the exported
reports should be other timezone than UTC.

fix:
plywood passes the user selected timezone to format function here
https://github.com/implydata/plywood/blob/master/src/datatypes/dataset.ts#L343
We can use that to get time in the user selected timezone. The time in reports
will also have information about timezone because of ISO format.
@metapraveen metapraveen force-pushed the consider-user-selected-timezone-while-exporting branch from 62f82c0 to 2d6e191 Compare February 17, 2021 09:24
@metapraveen
Copy link
Contributor Author

FYI:

> moment().format("YYYY-MM-DDTHH:mm:ss.sssZ")
"2021-02-16T17:10:28.2828+01:00"

Used this format because before it used to return .toISOString(). So whenever user looks at exported reports he can know on which timezone this report is for.

@adrianmroz-allegro
Copy link
Contributor

Used this format because before it used to return .toISOString(). So whenever user looks at exported reports he can know on which timezone this report is for.

Sorry, I put this comment here for fellow reviewers, so they can see what output your change would produce :)

But have you tried importing this format into excel? Does excel/google sheets correctly parse this format?

@metapraveen
Copy link
Contributor Author

metapraveen commented Feb 17, 2021

But have you tried importing this format into excel? Does excel/google sheets correctly parse this format?

Yes, google sheet correctly parse this format

@adrianmroz-allegro adrianmroz-allegro merged commit 9eb7d93 into allegro:master Feb 24, 2021
@metapraveen metapraveen deleted the consider-user-selected-timezone-while-exporting branch February 24, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants