Skip to content

Consider server timezone on _get_timezone_offset instead of django's settings #886

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

iwalucas
Copy link

@iwalucas iwalucas commented May 7, 2025

DatabaseScheduler class consider's django timezone calculations to offset hour, but beat uses server's so that creates a situation where some tasks are excluded from running hours

Report: #885

@auvipy auvipy requested review from Copilot and auvipy May 7, 2025 11:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the _get_timezone_offset method so that it leverages the server's timezone rather than Django's timezone settings.

  • Removed dependency on django.utils.timezone
  • Obtains the server time via aware_now() and retrieves its timezone using ZoneInfo

Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.17%. Comparing base (87c0597) to head (70d34ac).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #886      +/-   ##
==========================================
+ Coverage   88.07%   88.17%   +0.09%     
==========================================
  Files          32       32              
  Lines        1006     1006              
  Branches      104      104              
==========================================
+ Hits          886      887       +1     
  Misses        101      101              
+ Partials       19       18       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

I was thinking if you can add some additional tests to verify the changes, and check if it creates any regresion

Copy link

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Suggest a minor rephrasing in the code.

server_tz = timezone.get_current_timezone()
server_time = aware_now()
# Use server_time.tzinfo directly if it is already a ZoneInfo instance
server_tz = server_time.tzinfo if isinstance(server_time.tzinfo, ZoneInfo) else ZoneInfo(str(server_time.tzinfo))
Copy link

@jennifer-richards jennifer-richards May 7, 2025

Choose a reason for hiding this comment

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

The tzinfo class does not define __str__. It would be better to use server_time.tzinfo.tzname() which is guaranteed to exist.

There's no guarantee ZoneInfo will know what to do with the tzname(), though, so this won't work with obscure timezone implementations. (Edit: or with obscure zones not known to ZoneInfo. Either of these cases will probably break Django, so I don't think that this is a problem, just pointing it out.)

@auvipy
Copy link
Member

auvipy commented May 8, 2025

@alirafiei75 can you please have another look

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.

3 participants