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 : UTC based timezone #2177

Merged
merged 11 commits into from
Feb 8, 2024
Merged

Fix : UTC based timezone #2177

merged 11 commits into from
Feb 8, 2024

Conversation

Preetam-Das26
Copy link
Collaborator

Purpose of PR?:

Fixes #2122

Does this PR introduce a breaking change?
NO

If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

  • Bug fix. Fixes #
  • New feature (Non-API breaking changes that adds functionality)
  • PR Title follows the convention of <type>: <subject>
  • Commit has unit tests

@ReimarBauer
Copy link
Member

please remove the pytest.log

@ReimarBauer
Copy link
Member

on "future" fixes you may be want to create a branch for an issue you work on. git -b name

our stable branch means released on conda-forge.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

remove accidantly committed files, line added to /python-flake8.yml

@Preetam-Das26
Copy link
Collaborator Author

Ok sure

@Preetam-Das26
Copy link
Collaborator Author

@ReimarBauer Kindly review the recent changes made .

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

Thx

@ReimarBauer ReimarBauer self-requested a review February 8, 2024 07:59
@ReimarBauer
Copy link
Member

not seen that I had to enable tests ;) let us see what they tell.

instance/mscolab.db Outdated Show resolved Hide resolved
.github/workflows/python-flake8.yml Outdated Show resolved Hide resolved
self.fm.modify_user(user_query, attribute="confirmed_on", value=confirm_time)
self.fm.modify_user(user_query, attribute="confirmed", value=True)
user_query = User.query.filter_by(id=user.id).first()
assert user_query.confirmed is True
assert user_query.confirmed_on == confirm_time
assert user_query.confirmed_on.replace(tzinfo=None) == confirm_time.replace(tzinfo=None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change hides a bug.

When I revert this I get the following error:

FAILED tests/_test_mscolab/test_file_manager.py::Test_FileManager::test_modify_user - assert datetime.datetime(2024, 2, 9, 8, 49, 39, 968400) == datetime.datetime(2024, 2, 9, 8, 49, 39, 968400, tzinfo=datetime.timezone.utc)

This means that the database query returns a naive datetime object, which is then not equal to the timezone aware object. I think the database query should return an aware datetime object instead, but I am not yet sure how to make it do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I didn't realise this situation. I will look into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can also be handled in a follow-up PR because it is not exactly related to the initial issue that is addressed here. But I would prefer if this line stays as is (i.e. assert user_query.confirmed_on == confirm_time), and the test be marked as xfail instead to acknowledge that this is an issue.

@matrss
Copy link
Collaborator

matrss commented Feb 8, 2024

I also just noticed that we have a bunch of calls to datetime.datetime.utcnow(), which is even worse than .now() without a timezone. We should address those as well, but that could also be a follow-up PR.

@matrss matrss merged commit 0a04998 into Open-MSS:develop Feb 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use UTC based timestamps
4 participants