refactor: datetime.now() to datetime.utcnow()

[mtreacy002]

Description

This PR is to replace all occurrences of datetime.now() too datetime.utcnow()

Fixes #555

Type of Change:

• Code

Code/Quality Assurance Only

• Refactoring datetime.now() to datetime.utcnow()

How Has This Been Tested?

I tested this by running python run.py and register/login which worked as normal. I also tested this by running unittests resulted in all tests passed.

Checklist:

• My PR follows the style guidelines of this project
• I have performed a self-review of my own code or materials

Code/Quality Assurance Only

• My changes generate no new warnings
• existing unit tests pass locally with my changes

Additional Notes
Some part of the codes were reformatted through this PR using VSCode auto-enabled pylint.

[mtreacy002]

@isabelcosta, in this PR I only replace the occurrences of datetime.now(). I didn't change the time.time() in register user and its related cron job and test. Let me know if you want this to be change as well.

[robotjellyzone]

hmm.... apart from adding utcnow() , i can see that you have done the formatting of most of the returning messages with the response then I think you can change the title of the issue or this PR to something relevant to it or you can also elaborate about this in the description of this PR as well so, that it can easily help anyone to understand exactly what all you have done in this PR ... [just a suggestion,] BTW, its a greatly needed change in the PR 👍

[mtreacy002]

Oopss.. I think this must be the autopep or something I accidentally set to 'ON' in my VSCode IDE. I'll fix it and push again.. Nice catch, @robotjellyzone. Thanks again. 😉

[mtreacy002]

mmm. I think it's the Prettier on VSCode doing this auto-prettying stuff. hehehe.. should I just add a little explanation on the PR that I did a little formatting a bit, rather than changing it back to how it was. Will it make any different if I reverse it back? I think the difference is only from one line to two lines. @robotjellyzone, what do you think?

[robotjellyzone]

I think you should mention this in your description of this PR but I think then you have to go through the various other files as well to make sure this consistency should be maintained with other response messages as well so, maybe let's just wait for @isabelcosta to suggest !!

[mtreacy002]

Alright, I think before I change anything, I'll wait for @isabelcosta to give her thoughts then. 😉

[mtreacy002]

I found the culprit. I set both Python > Linting: Lint On Save and Python > Linting: Pylint Enabled to True. Mmm... I actually quite like these options to be on, just to make sure the codes abide the Linting standards. I'll probably mention this is PR as additional note and if need to, for the rest of the files we can schedule some code reformatting period (once a month or every 2-3 month?) just to make sure code quality is kept in check. It'll be similar to what I did with PR#478 last time, but in smaller scale as opposed to the whole project (probably folder by folder). Anyway, we'll wait for @isabelcosta, or perhaps @ramitsawhney27 can also give us advise on this 😆 .

[isabelcosta]

code reformatting period (once a month or every 2-3 month?)

This is interesting definitely! However, the real solution I think would be, doing a refactor on the style, and ensure that from a point in time, contributors are required to run a linting tool that does such code formatting. Just like we started talking for black tool. I am just not sure about which tool to use 🤔

[mtreacy002]

Thanks, @ramitsawhney27 for giving your approval. On the discussion I have with @robotjellyzone, can you pls share your thoughts on this? I really appreciate your support 😉

[isabelcosta]

I understand there was some reformating in the PR which is good but at the same time some are not related to this issue. I feel I can overlook this and start a conversation on what tool to use to have this issue solved, and have everyone using the same, in this way minimizing these reformats.

Other than that the code seems nice :) Thank you so much for this contribution @mtreacy002 🎉

[isabelcosta]

code reformatting period (once a month or every 2-3 month?)

This is interesting definitely! However, the real solution I think would be, doing a refactor on the style, and ensure that from a point in time, contributors are required to run a linting tool that does such code formatting. Just like we started talking for black tool. I am just not sure about which tool to use 🤔

[isabelcosta]

@robotjellyzone if you were to ignore the formatting, would you approve this PR?
Can you add your review to the PR, please?

[robotjellyzone]

@robotjellyzone if you were to ignore the formatting, would you approve this PR?
Can you add your review to the PR, please?

Yeah sure @isabelcosta ! Everything is totally fine & perfect. You did a great job @mtreacy002 👍 , I just thought that it would be great if @mtreacy002 just elaborate about these formatting in the description as well [this will improve PR review for every new reviewer] as its always good to know what all this PR is doing . [even if its doing a small change, as no change is small at all in open source :)]

[isabelcosta]

@robotjellyzone you are totally right :) 👏 PR description needs to be clear about changes done here, @mtreacy002 can you please update it to reflect that?

[mtreacy002]

@isabelcosta and @robotjellyzone, I had changed the PR description to include the reformatting, is this ok? If you think I need to elaborate further, can you please explain into how much details you want this elaboration should be (can you give me example, please)? Thanks beforehand

[robotjellyzone]

I think its fine now @mtreacy002 👍 what do you think @isabelcosta ? also, i think that a separate issue needs to be created to ensure consistency in the entire code or can you do the formatting of the entire code in this PR itself @mtreacy002 [as you are already describing this] or should create a new issue for that?

[mtreacy002]

I think its fine now @mtreacy002 👍 what do you think @isabelcosta ? also, i think that a separate issue needs to be created to ensure consistency in the entire code or can you do the formatting of the entire code in this PR itself @mtreacy002 [as you are already describing this] or should create a new issue for that?

I can do the reformatting of whole project again using black tools like last time, @robotjellyzone. @isabelcosta, what do you think of this? Let me know if you want me to do it in this PR or open a separate issue for it. 😉

[ramitsawhney27]

In my opinion, it's good that you've improved the format structure, generally good to have linters like autopep on. As for formatting the entire project, I think that's a different task, for which feel free to open the issue. I think we can do without having it in this PR, and we can also do without changing the existing PR's format.

[mtreacy002]

In my opinion, it's good that you've improved the format structure, generally good to have linters like autopep on. As for formatting the entire project, I think that's a different task, for which feel free to open the issue. I think we can do without having it in this PR, and we can also do without changing the existing PR's format.

Thanks, @ramitsawhney27 for sharing your thoughts 😃. @isabelcosta, we probably can bring this up to our coding team meeting to discuss about having a routine schedule for code refactoring.

[robotjellyzone]

Yeah, thanks @ramitsawhney27 👍 so, a new issue would be a lot better idea. Now, I think this PR is ready to merge but before that testing needs to be done [as label said] @isabelcosta @mtreacy002 . great job 🎉 !!

[isabelcosta]

The test here that comes to my mind, is simply creating a user, calling some endpoints and see if the time is correct?! 🤷‍♀️ @anitab-org/qa-team

[robotjellyzone]

The test here that comes to my mind, is simply creating a user, calling some endpoints and see if the time is correct?! 🤷‍♀️ @anitab-org/qa-team

I am also thinking about its testing part as I am not sure how should I ensure that it's storing it in UTC maybe I should try to print those values in my terminal/console as since it will be storing time in our zone ie UTC so, we can try to print it to ensure this [not sure though] !! let me check this 🤔 💭

[rpattath]

@mtreacy002 @isabelcosta any endpoints apart from GET user (screenshot below) that can be tested for this PR?

[robotjellyzone]

Hi @rpattath i think you need to test this for mentorship relation and task creation as the changes have been done in those places only [for making utc time in mentorship relation ]

[rpattath]

@mtreacy002 I am seeing this for request creation datetime. I created the request just a few minutes before the time on the left hand side of the screenshot. On the right hand side of the screenshot you see the output of the request creation_date timestamp from the swagger UI. That does not look correct to me.

[mtreacy002]

@rpattath , can you tell me how you get the time on the left?

• did you grab it from the database or by adding print function on the code/grabbing the value from a running debugger before save_data function?
• Also, what is the actual time you recorded when you did this (I mean from the one you wrote down, so, not computer processed time - pls also tell me your UTC offset - Your '+' or '-' from UTC).
• I'm also suspecting that this might be caused by the Float data type we're using when dealing with Date. It's known for loosing precision which could lead to this discrepancy (notice on the top of each screenshot, the left has no decimal trails but the one on the right has decimal trails).
  I think using Numeric data type with precision declared (e.g. Numeric(16,6)) is better for consistency that way data is recorded/retieved in the same value.
  cc @isabelcosta and @ramitsawhney27.

[rpattath]

@rpattath , can you tell me how you get the time on the left?

* Is it from the database or from adding print function on the code before save data function?

@mtreacy002 I am using https://www.unixtimestamp.com/index.php to convert float timestamp to datetime format. That website by default prints the current timestamp on the left.

* Also, what is the actual time you recorded when you did this (I mean from the one you wrote down, so, not computer processed time - pls also tell me your UTC offset - Your '+' or '-' from UTC).

4:42pm my time is 8:42pm UTC
The time I created the request was 5/17/2020 @ 7:45pm UTC (maybe + 2-3 minutes). I did not record the exact time.

* I'm also suspecting that this might be caused by the Float data type we're using when dealing with Date. It's known for loosing precision which could lead to this discrepancy (notice on the top of each screenshot, the left has no decimal trails but the one on the right has decimal trails).
  I think using Numeric data type with precision declared (e.g. Numeric(16,6)) is better for consistency that way data is recorded/retieved in the same value.
  cc @isabelcosta and @ramitsawhney27.

[mtreacy002]

So, as far as this PR concern, we did get the time recorded in UTC. The thing is the Float data type creates discrepancy from the data recorded to the data retrieved. I think we should open another issue for this so we can deal with it separately.

What do you think @isabelcosta, @ramitsawhney27, @robotjellyzone and @rpattath?

[SanketDG]

The floating value cannot cause discrepancy, I am not sure if that is the case.

@rpattath Apologies, but if you have some bandwidth, can you re-test this again just to be sure? There shouldn't be such a large discrepancy if you say you launched the request some minutes ago.

[rpattath]

@SanketDG @mtreacy002 I am still seeing the same. I created the request at 05/18/2020 @ 10:42pm (UTC)

[rpattath]

So, as far as this PR concern, we did get the time recorded in UTC. The thing is the Float data type creates discrepancy from the data recorded to the data retrieved. I think we should open another issue for this so we can deal with it separately.

What do you think @isabelcosta, @ramitsawhney27, @robotjellyzone and @rpattath?

@mtreacy002 do you have any suggestions on how to test the recorded datetime with this existing issue? At this point I am not sure how to test this PR.

[mtreacy002]

Hi @Roshni, @robotjellyzone.
So, I've just tested the user register to check if utcnow() does what it is supposed to do: record the date/time in UTC+0.
Here's what I did:

• run python run.py on debug mode.
• register a new user (noted down that I registered on May 20th 07:39PM AEST (UTC+10) and verified email not long after)
• check the data recorded inside database (using sqlite browser)

note that the email verification date is the one using datetime.utcnow() whereas register date is using time.time() (which is also a UTC+0 by default) so, to show registration time (time.time() not too far out from email verification date, I convert it from epoch to human readable (left screen in screenshot below). On the right side is the result of GET user. Notice that the value return for both (registration date and email verification date is still on UTC+0).

• As far as this PR concern (recording data in UTC+0), it is complete and the test has proved it.

HOWEVER, the next step needs to be taken so that when a user retrieve the data (which is recorded in UTC+0), they must see it in their own timezone (converted). Because this action (retrieving data) can be done by anyone, can be the user related to the data or other users who can be located anywhere with different Timezone (UTC offset). So, like in the example test above, I'm in UTC+10, when I retrieve the data, I need to see it in my own timezone (May 20th 07:39PM AEST). And If an admin located in Los Angeles retrieve the user data I just created, they need to see it as May 20th 02:39AM PDT. But in the database, we must record it on its natural UTC+0 format (May 20th 09:39AM).

Hope this makes sense.

@isabelcosta and @ramitsawhney27, should we open few issues to deal with this on Mentorship System repos?

• First issue is to add Timezone field in User registration form on Android frontend
• Second issue is to add Timezone column in User table on Mentorship Backend
• Third issue is to add function to convert UTC+0 to the user's timezone (user=whoever retrieves the data).

In my BridgeInTech DB design, I've taken this into consideration by having timezone column on users_extension table.

[mtreacy002]

I'm gonna give one function of the mentorship relation (e.g. send request) a test now. will update you all soon.

[mtreacy002]

So, here's the test I did on Mentorship Relation send request.

Steps:

• registered 2nd user (testtwo)
• logged in as 2nd user and send request to the first user I created previously (testone). I wrote down that I sent the request on May 20th 9:09PM AEST (UTC+10).
• checked in the database and confirm the data is recorded in UTC+0 (I expected to see it's recorded in May 20th 11:09AM UTC+0). Now, let's see what we have:

• convert creation date from epoch timestamp to human readable format and GET /mentorship_relations/pending (still login as user 2). Notice that we get a WRONG time recorded (May 20th 1:09AM UTC+0 which is equal to May 20th 11:09AM UTC+10)!!

What went wrong here?

Upon further investigation I found out that we have an extra step in the code to record creation_date in the mentorship relation (app/api/dao/mentorship_relation.py line 113) which is using .timestamp() after datetime.utcnow()

where as in the email verification date for user register (app/api/dao/user.py line 349), we've only using datetime.utcnow() without .timestamp().

Now, this is a good pick up, @rpattath. This means we have to check all codes dealing with date if they're using .timestamp() to make sure they get recorded correctly. There must be a reason why we need to use this extra step .timestamp() in our code previously. But whatever the reason is, we need to make sure it is used/done properly to represent the actual event.

Can you please open an issue for this bug (.timestamp()), @rpattath?

[mtreacy002]

Ok, so I ran the send request from develop branch (with datetime.now().timestamp()) and did the same step above (send request). The date was recorded correctly as in UTC+0 (sent on May 20th 10:30PM UTC+10 and database recorded it as May 20th 12:20PM UTC+0).

This is explained in the python docs that .timestamp() returns posix timestamp with float value similar to what is returned by time.time() we used in register method to record register_date.

@isabelcosta and @ramitsawhney27. I think we need to decide which of the three methods to record date in UTC+0 we're going to use throughout the project (for consistency):

• time.time()
• datetime.now().timestamp()
• datetime.utcnow()

I haven't look closely to every single code where they are being used to see if using only one option out of the three will be a possible solution.

[rpattath]

So, here's the test I did on Mentorship Relation send request.

Steps:

* registered 2nd user (testtwo)

* logged in as 2nd user and send request to the first user I created previously (testone). I wrote down that I sent the request on May 20th 9:09PM AEST (UTC+10).

* checked in the database and confirm the data is recorded in UTC+0 (I expected to see it's recorded in May 20th 11:09AM UTC+0). Now, let's see what we have:

* convert `creation date` from epoch timestamp to human readable format and GET /mentorship_relations/pending (still login as user 2). Notice that we get a WRONG time recorded (May 20th 1:09AM UTC+0 which is equal to May 20th 11:09AM UTC+10)!!

What went wrong here?

Upon further investigation I found out that we have an extra step in the code to record creation_date in the mentorship relation (app/api/dao/mentorship_relation.py line 113) which is using .timestamp() after datetime.utcnow()

where as in the email verification date for user register (app/api/dao/user.py line 349), we've only using datetime.utcnow() without .timestamp().

Now, this is a good pick up, @rpattath. This means we have to check all codes dealing with date if they're using .timestamp() to make sure they get recorded correctly. There must be a reason why we need to use this extra step .timestamp() in our code previously. But whatever the reason is, we need to make sure it is used/done properly to represent the actual event.

Can you please open an issue for this bug (.timestamp()), @rpattath?

Opened this issue and moving this PR to Needs Review.

[vj-codes]

@mtreacy002 please solve the merge conflicts

[vj-codes]

@mtreacy002 hey can you please resolve the conflicts , as this PR already has 2 approving reviews after conflicts are solved it can be directly tested:)

[codecov]

Codecov Report

Merging #556 (c1c8a9f) into develop (4295459) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #556   +/-   ##
========================================
  Coverage    92.50%   92.50%           
========================================
  Files           38       38           
  Lines         2014     2014           
========================================
  Hits          1863     1863           
  Misses         151      151           

Impacted Files	Coverage Δ
app/api/dao/mentorship_relation.py	96.11% <100.00%> (ø)
app/api/dao/task.py	95.45% <100.00%> (ø)
app/api/dao/user.py	85.77% <100.00%> (ø)
app/database/models/task_comment.py	95.00% <100.00%> (ø)
app/schedulers/complete_mentorship_cron_job.py	100.00% <100.00%> (ø)

[mtreacy002]

@mtreacy002 hey can you please resolve the conflicts , as this PR already has 2 approving reviews after conflicts are solved it can be directly tested:)

Done, @vj-codes 😉

[devkapilbansal]

LGTM 👍

Tested locally and working fine. All instances of datetime.now() are replaced by datetime.utcnow()

[isabelcosta]

Looks good 😄
Thank you for updating this PR @mtreacy002 🙌🏾