Extend close time logic to differentiate between the claimant and other users by ChrisLovering · Pull Request #1470 · python-discord/bot

ChrisLovering · 2021-03-16T20:14:05Z

This extends the closing logic for help channels to closed based on the claimant's last message time and last message in general.

This results in the help channel closing time being reduced if only others are chatting, and the claimant has already left.

Thanks to laundmo, Darr, Griff, LX and Hemlock for the group coding on this PR :D

This allows us to configure the idle time allowed for claiments seperate from tohers.

As we have complicated this logic, we now don't specify exactly how long until the channel will close.

…er users.

laundmo

I was there in VC watching this be implemented and tested, all seems good to me.

Shivansh-007

Few nitpicks:

MarkKoz · 2021-03-17T19:49:31Z

+    claimant_last_message_time += timedelta(minutes=constants.HelpChannels.idle_minutes_claimant)
+
+    # The further away closing time is what we should use.
+    closing_time = max(claimant_last_message_time, last_message_time)


This breaks the priority that deleted_idle_minutes has because idle_minutes_claimant is configured to be greater than deleted_idle_minutes by default.

If _message.is_empty(channel) returns True, this indicates that there isn't a message in the channel. So both cache's would be empty. This would mean we enter the if statement and return before this code is hit.

That is incorrect because the cache doesn't get cleared when the channel goes dormant. is_empty doesn't mean the channel has no messages at all; it basically means the channel's latest message is the bot's embed.

I added code to clear both caches when the channel is unclaimed in a recent commit, so would that mean it's now correct?
https://github.com/python-discord/bot/pull/1470/files#diff-c31098978b8d838eadb52b2bf23494072cc66686e738d7f99c4f59e6495bfe6fR380-R381

Maybe, but is there a way to fix it without relying on the cache being cleared?

How do you feel about returning early, within the if await _message.is_empty(channel): block?

MarkKoz · 2021-03-17T20:06:24Z

Has it been decided to "take the hit" when the bot is offline and can't cache messages that get sent?

laundmo · 2021-03-17T23:33:21Z

@MarkKoz might need to add a condition to the old method fallback, that checks for bot uptime smaller than idle_minutes_claimant so that there is at least that much time after a restart after which channels wont be unduly closed.

that is, if i understood correctly what you were referring to.

Issue with this breaking other functionality have been brought up.

MarkKoz · 2021-03-18T00:23:49Z

@MarkKoz might need to add a condition to the old method fallback, that checks for bot uptime smaller than idle_minutes_claimant so that there is at least that much time after a restart after which channels wont be unduly closed.

that is, if i understood correctly what you were referring to.

What I was referring to is the cache not having the latest times due to the bot going offline and more messages being sent in channels. Upon going online again, the bot may prematurely close channels due to the outdated cache leading to the perception that the channels have been inactive.

I don't understand what you're proposing.

laundmo · 2021-03-18T08:50:08Z

@MarkKoz as it stands currently, it will fall back to the old behavior if the cache isn't available (empty/ returns None)

I am proposing extending the fallback conditions to always use the old method for a while after startup, as it does not rely on cache. This way, instead of channels being closed prematurely they might take a bit longer to close.

The unanswered cache was previously just a boolen of whether a non-claimant every replied to a help channel. With us now needing to know the time at which a non-claimant messaged in a given channel, we infer the answered status from this cache instead.

Also updates the doc string to reflect this new behaviour.

MarkKoz · 2021-03-18T20:53:46Z

@MarkKoz as it stands currently, it will fall back to the old behavior if the cache isn't available (empty/ returns None)

I am proposing extending the fallback conditions to always use the old method for a while after startup, as it does not rely on cache. This way, instead of channels being closed prematurely they might take a bit longer to close.

Yes, that could work. It'd have to reschedule it for the maximum time.

…ther than if both are empty

ChrisLovering · 2021-03-18T21:32:11Z

@MarkKoz as it stands currently, it will fall back to the old behavior if the cache isn't available (empty/ returns None)
I am proposing extending the fallback conditions to always use the old method for a while after startup, as it does not rely on cache. This way, instead of channels being closed prematurely they might take a bit longer to close.

Yes, that could work. It'd have to reschedule it for the maximum time.

What about clearing the caches on init, so the bot uses the fallback on startup, until the caches have had time to populate?
Another open is populating the cache's on init too.

…ibility

MarkKoz · 2021-03-25T22:05:50Z

    if claimed_timestamp:
-        claimed = datetime.utcfromtimestamp(claimed_timestamp)
-        return datetime.utcnow() - claimed
+        claimed = datetime.fromtimestamp(claimed_timestamp)


discord.py still returns times in UTC; they're just naïve. It uses utcfromtimestamp. Sorry if my previous comment was misleading.

…ession has yet to be answered.

datetime.min cannot be converted to a timestamp as it's pre-epoch. Instead wait until we actuall need it and then create the correct datetime object depending on teh cache contents.

…time objects.

MarkKoz · 2021-03-26T21:09:51Z

I'm trying this out with the following config:

    idle_minutes_claimant: 2
    idle_minutes_others: 1
    deleted_idle_minutes: 1

I simply claim a channel, and it correctly schedules it to close in 2 minutes. However, when the closing task runs, it calculates a wrong time. The log timestamps on the left are in UTC-7. Converted to UTC the times would be around 2021-03-26 20:58:06. Therefore, the time it calculates is off by 7 hours. It's clearly an issue with timezones somewhere.

2021-03-26 13:58:06 | bot.exts.help_channels._channel | TRACE | #help-burrito (800698210392997918) should be closed at 2021-03-27 03:58:05.915000 due to claimant timeout.
...
2021-03-26 13:58:06 | bot.exts.help_channels._cog | INFO | #help-burrito (800698210392997918) is still active; scheduling it to be moved after 25199 seconds.
...
2021-03-26 13:58:06 | bot.utils.scheduling.HelpChannels | TRACE | Waiting 25199 seconds before awaiting coroutine for #800698210392997918.

Previously we were using `utcfromtimestamp()` which would compensate the timestamp when converting to UTC even though the timestamp itself was in UTC: >>> datetime.utcnow() datetime.datetime(2021, 3, 26, 22, 8, 47, 441603) >>> a = datetime.utcnow().timestamp() 1616821624.207364 >>> a = datetime.utcfromtimestamp(a) datetime.datetime(2021, 3, 27, 5, 7, 4, 207364) By switching to `fromtimestamp()` this avoids that behaviour.

It has some API changes, so it's best to update now before the project starts using the library more.

Fix issues converting timestamps to datetimes and vice-versa. The main culprit id `datetime.timestamp()`, which always assumes naïve objects are in local time. That behaviour conflicts with discord.py, which returns naïve objects in UTC rather than local time. Switching from `utcfromtimestamp` to `fromtimestamp` was incorrect since the latter also assumes the timestamp is in local time.

jb3

Code looks good and functionality works, just a few documentation things.

jb3 · 2021-03-30T19:29:18Z

    """
    Report stats for a completed help session channel `channel_id`.

    Set `is_auto` to True if the channel was automatically closed or False if manually closed.


This docstring needs updating with closed_on and the potential values.

jb3

Looks good to me!

jb3

Actual approval now, .

ChrisLovering added 4 commits March 16, 2021 17:21

Split out help channel idle time constants

6b9c954

This allows us to configure the idle time allowed for claiments seperate from tohers.

Set a reasonable default for idle_minutes_others.

ce86169

Update help availible footer

243d465

As we have complicated this logic, we now don't specify exactly how long until the channel will close.

Extend close time logic to differentiate between the claimant and oth…

b3c66a6

…er users.

ChrisLovering requested review from Akarys42 and MarkKoz as code owners March 16, 2021 20:14

laundmo added a: help channels Related to the help channel system p: 1 - high High Priority labels Mar 16, 2021

laundmo previously approved these changes Mar 16, 2021

View reviewed changes

Improve trace message.

9503a2e

Shivansh-007 suggested changes Mar 17, 2021

View reviewed changes

Comment thread bot/exts/help_channels/_message.py Outdated

Comment thread bot/exts/help_channels/_cog.py Outdated

Comment thread bot/exts/help_channels/_channel.py Outdated

Xithrius added the t: feature New feature or request label Mar 17, 2021

ChrisLovering added 2 commits March 17, 2021 18:33

Improve availible channel embed footer

61e17c8

Improve docs strings and variable names within the help channel system

12a73f2

MarkKoz requested changes Mar 17, 2021

View reviewed changes

ChrisLovering added 4 commits March 18, 2021 20:08

Update function name to describe new behaviour.

5003059

Also updates the doc string to reflect this new behaviour.

Delete cached claimant message time.

9a3c946

Move return behaviour comments to in-line rather than docstring.

6a954de

ChrisLovering added 3 commits March 18, 2021 21:17

If the channel is empty, determine closing time based on last message.

e962aa5

Determine closing time from last message if either cache is empty, ra…

3fe9a39

…ther than if both are empty

More descriptive comment for when an if block is entered

72eb71d

laundmo reviewed Mar 18, 2021

View reviewed changes

Comment thread bot/exts/help_channels/_channel.py Outdated

ChrisLovering added 3 commits March 25, 2021 20:30

update help channel on_message docstring to reflect unchanged respons…

ef38220

…ibility

Only fetch claimant id once

93b9d41

Change help channel caching logic to use timezone naive stamps

3114b48

MarkKoz requested changes Mar 25, 2021

View reviewed changes

ChrisLovering added 3 commits March 25, 2021 22:15

Create utc datetime objects from timestamps

bae3d6a

Refactor help channel update message cache function for clearer flow

f228659

Refactor if block within help channel system to be more readable

d7eb48a

MarkKoz requested changes Mar 25, 2021

View reviewed changes

Comment thread bot/exts/help_channels/_cog.py Outdated

Comment thread bot/exts/help_channels/_stats.py

ChrisLovering added 4 commits March 25, 2021 23:24

Reset a channel's non-claimant cache on claim, to indicate that the s…

a3a5fc4

…ession has yet to be answered.

Convert to timestamp before posting to redis

a951789

Fix error when converting non claimant message

fea068f

datetime.min cannot be converted to a timestamp as it's pre-epoch. Instead wait until we actuall need it and then create the correct datetime object depending on teh cache contents.

Add 1 second due to POSIX timestamps being lower resolution than date…

519398b

…time objects.

ChrisLovering and others added 5 commits March 26, 2021 21:57

Use correct constant for each type of help session user

a732e0a

Update arrow to 1.0.3

07e3b45

It has some API changes, so it's best to update now before the project starts using the library more.

HelpChannels: refactor get_closing_time

feb9b22

MarkKoz approved these changes Mar 30, 2021

View reviewed changes

jb3 requested changes Mar 30, 2021

View reviewed changes

Enumerate all possible values for closed_on in docstring

96037aa

jb3 reviewed Mar 30, 2021

View reviewed changes

jb3 approved these changes Mar 30, 2021

View reviewed changes

laundmo approved these changes Mar 30, 2021

View reviewed changes

Change to an Enum for possible closing reasons

2621f24

jb3 approved these changes Mar 30, 2021

View reviewed changes

Fix copy & paste error in closing reason enum

57fb66d

jb3 merged commit 07fb4ed into main Mar 30, 2021

jb3 deleted the help-channel-closing-delay-changes branch March 30, 2021 20:28

kwzrd mentioned this pull request Apr 13, 2021

Branding: correct broken timestamp persistence & improve cosmetics #1523

Merged

Uh oh!

Conversation

ChrisLovering commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laundmo left a comment

Choose a reason for hiding this comment

Uh oh!

Shivansh-007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarkKoz Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

ChrisLovering Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

MarkKoz Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

ChrisLovering Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkKoz Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

ChrisLovering Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarkKoz commented Mar 17, 2021

Uh oh!

laundmo commented Mar 17, 2021

Uh oh!

MarkKoz commented Mar 18, 2021

Uh oh!

laundmo commented Mar 18, 2021

Uh oh!

MarkKoz commented Mar 18, 2021

Uh oh!

Uh oh!

ChrisLovering commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarkKoz Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarkKoz commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jb3 left a comment

Choose a reason for hiding this comment

Uh oh!

jb3 Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jb3 left a comment

Choose a reason for hiding this comment

Uh oh!

jb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

ChrisLovering commented Mar 16, 2021 •

edited

Loading

ChrisLovering Mar 18, 2021 •

edited

Loading

ChrisLovering commented Mar 18, 2021 •

edited

Loading

MarkKoz commented Mar 26, 2021 •

edited

Loading