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: closed trades timezone consistency #454

Merged

Conversation

uhliksk
Copy link
Contributor

@uhliksk uhliksk commented Aug 4, 2022

Description

  • Fix data inconsistencies caused by using various timezone settings on client and server side. On the server side dates will be always stored in UTC format regardless of docker container timezone.
  • Fix wrong periods of time reported on client in client's timezone is other than UTC. Client will report it's timezone to server to get proper range of data from database based on local timezone.
  • Fix wrong first day of week reported on client with locale set to other day than sunday. Client will report it's locale to server to get proper range of data from database based on the locale.

Related Issue

#449

Motivation and Context

Closed trades were reporting different values on different places in application when day, week or month were selected as reported period of time.

How Has This Been Tested?

Docker container is using UTC time in default settings thus the change will not affect default configurations. In configurations with custom docker container time zone there may be temporary change in order of transactions stored in database if UTC time predeces the local time as the key was previously also generated including the time zone data.

Tested using custom TZ variable to change default docker container time zone. On the client side tested on system with time zone other than UTC.

Screenshots (if appropriate):

@uhliksk uhliksk changed the title Fix closed trades timezone consistency fix: closed trades timezone consistency Aug 4, 2022
@habibalkhabbaz
Copy link
Contributor

habibalkhabbaz commented Aug 5, 2022

Thanks @uhliksk
It seems a good PR!

However, I think the tests are failing. Are you planned to fix them? If not, just let me know and I will be happy to fix and to keep things easier for @chrisleekr when he have time to check.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 5, 2022

@habibalkhabbaz Oops, I didn't checked the tests. You can fix them, of course. Thank you.

@habibalkhabbaz
Copy link
Contributor

habibalkhabbaz commented Aug 5, 2022

Hello @uhliksk
I've finished fixing the test but unfortunately, I can't commit due to permission.
I don't know if this can be done from your side or from @chrisleekr side

remote: Permission to uhliksk/binance-trading-bot.git denied to habibalkhabbaz.

@chrisleekr
Copy link
Owner

Hello @uhliksk I've finished fixing the test but unfortunately, I can't commit due to permission. I don't know if this can be done from your side or from @chrisleekr side

remote: Permission to uhliksk/binance-trading-bot.git denied to habibalkhabbaz.

@habibalkhabbaz I think @uhliksk should add you as a collaborator in his forked repository.

@chrisleekr
Copy link
Owner

@uhliksk Thanks for your contributions.

As soon as @habibalkhabbaz commits tests, I will review and merge in.

@chrisleekr chrisleekr linked an issue Aug 6, 2022 that may be closed by this pull request
@chrisleekr chrisleekr added the bug Something isn't working label Aug 6, 2022
@chrisleekr chrisleekr self-requested a review August 6, 2022 13:20
@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 6, 2022

Hello @habibalkhabbaz

I've sent you an invite for uhliksk/binance-trading-bot.

@habibalkhabbaz
Copy link
Contributor

Thanks @uhliksk @chrisleekr
Tests has been added.

@chrisleekr
Copy link
Owner

chrisleekr commented Aug 7, 2022

@uhliksk @habibalkhabbaz

Hi guys,

I pushed some changes.
40dc23e
Please review and let me know what you think.

So the principle I get from this PR is,

  1. Anything saved into the database should be a UTC Date instance.
    i.e.
moment().utc().toDate()
  1. Slack message will be the docker container's local date time.
    i.e.
moment().format()
  1. Override data actionAt should be UTC ISO string format.
    i.e.
moment().toISOString()

Did I understand correctly? Let me know if I understand wrong.

And one suggestion for @uhliksk
It looks like you have not installed husky properly because it has forced lint rule for branch name.
You must see this error when you commit or push

Branch name lint fail! Branch "fix-closed-trades-timezone-consistency" must contain a separator "/".

And I also found some lint is not applied. Are you using VSCode?
If not, prefer to use VSCode as the project is optimised for it.

Check the doc - https://github.com/chrisleekr/binance-trading-bot/blob/master/DEVELOPMENT.md#branch-naming-conventions

Thanks for your contribution again. :)
If you guys - @uhliksk @habibalkhabbaz - are ok with the last changes, I will merge in.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 7, 2022

@chrisleekr Thank you for the suggestion. I'll install and use VSCode.

@habibalkhabbaz
Copy link
Contributor

@chrisleekr
I see it's fine to merge in if @uhliksk not have any issue.

@uhliksk
Copy link
Contributor Author

uhliksk commented Aug 7, 2022

@chrisleekr I see it's fine to merge in if @uhliksk not have any issue.

No issue. Fine to merge for me.

@chrisleekr
Copy link
Owner

Very good.
I will merge in.

@uhliksk @habibalkhabbaz

@chrisleekr chrisleekr merged commit 8160e25 into chrisleekr:master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent reporting for periods of time
3 participants