Use aware objects when storing and reading UTC timestamps

[DevilXD]

Type

• Bugfix
• Enhancement
• New feature

Description of the changes

.utcfromtimestamp() takes local timezone into account when converting a timestamp back into a datetime object: https://cdn.discordapp.com/attachments/160386989819035648/725707305320185876/unknown.png
Considering that the timestamp that ends up being stored is in UTC, this is probably not what was intended.

Ref of where the value is stored:

Red-DiscordBot/redbot/cogs/mod/kickban.py

Line 445 in 6328403

await self.config.member(user).banned_until.set(unban_time.timestamp())

There appears to be one other place where .utcfromtimestamp() is used - I modified that one accordingly too:

Red-DiscordBot/redbot/core/bank.py

Line 136 in ad97918

return datetime.datetime.utcfromtimestamp(time)

[Jackenmen]

Hmm, the actual issue here is with how we create the timestamp - datetime.timestamp() assumes that naive datetime instances (such as the one generated by datetime.utcnow()) represents local time and therefore generates incorrect POSIX timestamp that is relative to local timezone.

I don't think we can really solve that though, when we already have existing data, so I guess this might be needed middle ground 😐

[DevilXD]

datetime.timestamp() assumes that naive datetime instances (such as the one generated by datetime.utcnow()) represents local time (...)

This is not really the case though? datetime.utcnow() always generates a UTC timestamp object, no matter the timezone. The .timestamp() method converts that to a POSIX timestamp, and datetime.fromtimestamp() converts it back - also no matter the timezone. You should always operate on UTC timestamps internally, only optionally using timezone conversions when displaying the information to the user - otherwise you're exposing yourself to lots of potential bugs, such as the stored time intervals changing when you change the system timezone, among many others.

My recommendation would be to stick to UTC stamps, and as explained, only deal with timezones when it's time (duh) to display it to the user.

[Jackenmen]

This is not really the case though? datetime.utcnow() always generates a UTC timestamp object, no matter the timezone. The .timestamp() method converts that to a POSIX timestamp, and datetime.fromtimestamp() converts it back - also no matter the timezone.

Well, not really. .timestamp() assumes the time is local when the datetime object is naive. And datetime.utcnow() returns a naive datetime object that is not in local timezone.

[DevilXD]

This is technically ready to go. Aware datetime objects are now used in places where they are (or can end up) being stored in the config, along with UTC conversion back.

One thing I'd like to point out tho - the audio cog has lots of datetime.utcnow() usage, and probably lots of timestamp conversions too. Because the internals of that cog appear to be too complicated for me, I haven't touched anything there. I'm not sure if its in the scope of this PR, but if someone knowledgeable about that would / could point me to the right places, I'll add the changes as needed.

[Jackenmen]

A very late review, sorry for that.

Aside from review comments:

• The message.created_at in redbot/cogs/filter/filter.py:L376 was missed
• Per your comment, I checked Audio's usage of utcnow() and it shouldn't need any changes, so this PR doesn't need to touch it.

[Jackenmen]

Looks good to me, thanks for contributing these fixes for the timezone madness 😄