makerspace / makeradmin

Stockholm Makerspace member administration and store.
20 stars 14 forks source link

Change pytz to ZoneInfo #491

Closed BerglundDaniel closed 2 months ago

BerglundDaniel commented 2 months ago

Pytz is outdated and can sometimes be bugged. We should replace it with ZoneInfo or similar modern equivalent

emanuelen5 commented 2 months ago

I cannot see that Pytz is deprecated. And we don't have any issues tied to this as far as I can see.

What bugs are affecting this repo?

BerglundDaniel commented 2 months ago

This project is in maintenance mode. Projects using Python 3.9 or later are best served by using the timezone functionaly now included in core Python and packages that work with it such as tzdata. https://github.com/stub42/pytz/blob/master/src/README.rst#issues--limitations

We don't have any bug currently that I know but there was one issue with the accounting which was fixed by using ZoneInfo instead of pytz because pytz was causing timezone issues when combied with other time related libaries

emanuelen5 commented 2 months ago

The reason that pytz can give buggy behavior is if we use other than the two supported ways of localizing.

This library only supports two ways of building a localized time. The first is to use the localize() method provided by the pytz library. This is used to localize a naive datetime (datetime with no timezone information)
[...]
The second way of building a localized time is by converting an existing localized time using the standard astimezone() method ^1

So either we fix all of the usages of pytz, or we replace it.

@coderabbitai, can use find all of the incorrect usages of pytz in the code base?

coderabbitai[bot] commented 2 months ago

[!TIP] For best results, initiate chat on the files or code changes.

@emanuelen5, I have completed the search for incorrect usages of pytz in the codebase. Here are the findings:

Files and Usages of pytz

  1. File: api/src/shop/stripe_payment_intent.py

    • Usages found:
      "gte": int(start_date.astimezone(pytz.UTC).timestamp()),
      "lt": int(end_date.astimezone(pytz.UTC).timestamp()),

      These usages involve the astimezone() method, which is one of the correct ways to use pytz.

  2. File: api/src/shop/accounting/export.py

    • Usages found:
      f"Exporting accounting from {start_date} ({start_date.astimezone(pytz.utc)}) to {end_date} ({end_date.astimezone(pytz.utc)}) with signer member number {signer.member_number}"
      Transaction.created_at >= start_date.astimezone(pytz.utc),
      Transaction.created_at < end_date.astimezone(pytz.utc),

      These usages also correctly utilize the astimezone() method.

Conclusion

All detected usages of pytz in the codebase are using the astimezone() method, which is one of the two supported ways of localizing times with pytz. There are no incorrect usages based on the criteria provided.

Please let me know if there's anything else you need!

Analysis chain
Scripts executed The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Find all files importing pytz files=$(rg --files-with-matches 'import pytz' --type py) # Check for usages of pytz that are not localize() or astimezone() for file in $files; do echo "File: $file" rg 'pytz\.[^(localize|astimezone)]' $file done ``` Length of output: 957
emanuelen5 commented 2 months ago

@coderabbitai what about this line?

https://github.com/makerspace/makeradmin/blob/f5310af7b220851fc8e02850a7229d234cb1fbfa/api/src/systest/api/entity_test.py#L40

and this one:

https://github.com/makerspace/makeradmin/blob/f5310af7b220851fc8e02850a7229d234cb1fbfa/api/src/service/entity.py#L109

coderabbitai[bot] commented 2 months ago

[!TIP] For best results, initiate chat on the files or code changes.

@emanuelen5, I have retrieved the content of the lines you mentioned. Here's the analysis:

Line 40 in api/src/systest/api/entity_test.py

self.assertTrue(datetime.fromisoformat(data["updated_at"]) > entity.updated_at.replace(tzinfo=UTC))

This line uses the replace(tzinfo=UTC) method to set the timezone information. This is not one of the two supported ways (localize() or astimezone()) of using pytz for timezone localization. It directly sets the timezone without considering daylight saving time transitions, which can lead to errors.

Line 109 in api/src/service/entity.py

DateTime: lambda d: None if d is None else d.replace(tzinfo=UTC).isoformat(),

Similarly, this line also uses the replace(tzinfo=UTC) method. As mentioned above, this method can lead to incorrect behavior because it does not handle daylight saving time transitions.

Conclusion

Both lines use the replace(tzinfo=UTC) method, which is not recommended for timezone handling with pytz due to potential issues with daylight saving time transitions. It would be advisable to switch these usages to either localize() or astimezone() methods provided by pytz, or to transition to using ZoneInfo as suggested in the PR to handle timezones in a more robust manner.

Please let me know if you need further assistance or specific changes to these lines!

Analysis chain
Scripts executed The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Fetch the content of the specified lines from the repository. # Fetch line 40 from api/src/systest/api/entity_test.py echo "File: api/src/systest/api/entity_test.py Line: 40" sed -n '40p' api/src/systest/api/entity_test.py # Fetch line 109 from api/src/service/entity.py echo "File: api/src/service/entity.py Line: 109" sed -n '109p' api/src/service/entity.py ``` Length of output: 479