Fixes infraction display duration off-by-one discrepancy#2234
Conversation
- Refactored tests for new time duration arguments
- Changed `duration` parameter names to `duration_or_expiry` to more accurately reflect options for help
- Updated docstring to be more explicit on parameter fields
- Corrected datetime patching
- Added new usage of `last_applied` time for duration calculation in `apply_infraction`
ionite34
added
a: frontend
ionite34
changed the title
| def round_datetime(dt: datetime.datetime) -> datetime.datetime: | ||
| """ | ||
| Round a datetime object to the nearest second. | ||
|
|
||
| Resulting datetime objects will have microsecond values of 0, useful for delta comparisons. | ||
| """ | ||
| if dt.microsecond >= 500000: | ||
| dt += datetime.timedelta(seconds=1) | ||
| return dt.replace(microsecond=0) |
There was a problem hiding this comment.
Can you give an example case where the representation is improved with this function? Most deltas are in the hours or days so I'm not sure what called for this addition.
There was a problem hiding this comment.
So that was the rationale for this change, since the minimum accuracy of our humanize_delta is 1 second, the rounding will trim off microseconds via rounding.
The original issue was that the humanize_delta call matches time units until the last unit is exhausted, which, in the case of infractions, due to code and network latency, the delta between inserted_at (calculated at entry insertion server side) and expires_at (calculated at original command invocation) will at least be some microseconds off. In some extreme cases, this will be up to 10s or more.
In most cases, the entries are within 1 second in delta, but this is still enough to cause the 59 minutes 59 seconds display for the original 1h infraction, for example. This rounding will enable most legacy entries without a historical last_applied entry (currently defaults to their original inserted_at value at the site side) to most likely still display a whole unit of time.
In essence, this new function and its usage in humanize_delta will cause a previous 59 minutes, 59.9 seconds duration to round as 1 hour which should be desired behavior considering the minimum precision is 1 second.
This generic application, whilst not strictly necessary for infractions given the new last_applied field, will make future and existing usages of humanize_delta less likely to cause similar errors for local datetime deltas which may be off by some milliseconds due to code latency.
| payload = { | ||
| "actor": ctx.author.id, # Don't use ctx.message.author; antispam only patches ctx.author. | ||
| "hidden": hidden, | ||
| "reason": reason, | ||
| "type": infr_type, | ||
| "user": user.id, | ||
| "active": active, | ||
| "dm_sent": dm_sent | ||
| "dm_sent": dm_sent, | ||
| "last_applied": current_time.isoformat(), | ||
| } |
There was a problem hiding this comment.
Just to make sure I understand, you're supplying last_applied, but what sets inserted_at? I thought the goal was to make the two identical in the beginning. Is it done on the site's end?
There was a problem hiding this comment.
Currently, the site is setting inserted_at when the entry is inserted. The goal of the new attribute last_applied is to provide a datetime that is calculated on command run at the bot side, which should remove some of the server / network latency issues. In addition, the last_applied time is actually calculated together with the delta, meaning that the absolute delta of last_applied and expires_at is exactly equal to the infraction (for example 1 hour) down to the microsecond.
- Used arrow.utcnow to reduce complexity and import
- Changed `duration` parameter names to `duration_or_expiry` to more accurately reflect options for help
- Updated docstring to be more explicit on parameter fields
- Corrected datetime patching
- Added new usage of `last_applied` time for duration calculation in `apply_infraction`
- Used arrow.utcnow to reduce complexity and import
Given API updates, the fallback is not needed
- Used new method of dict subset comparison instead of datetime patching for better compat. with argument types
Corrected test case to use `datetime.utcnow()` to be consistent with target
- Added new infraction delta calculations to updated infractions. - Updates of infraction durations now also update the `last_applied` field. - `inserted_at` is now sent by the bot client to denote the original unmodified infraction application time
ionite34
force-pushed
the
infraction-durations
branch
from
fb028d1 to
6a87a38
Compare
|
DM and resend DM style has been modified in dcd946b pursuant to discussions. New style:
|
minalike
left a comment
minalike
left a comment
There was a problem hiding this comment.
Mina test, mina like. 👍🏻
4 participants
Closes #2217, Closes #2130
Overview
last_appliedtime field for infractions.last_appliedmodel attribute in the merged PR Add a field to track the time an infraction was last applied site#751Test Example
Related Fixes and improvements
1. Ban command help message
bancommand help message did not accurately reflect its ability to accept expiry ISO times2. Indicator for updated infraction durations
3. DM now displays whether the infraction duration has been edited
Design and API Notes
Expiryto use a new aliasDurationOrExpirypost_infractionsmethod of _utils.py has an updated parameter name and type. Associated usages have also been updated.timedeltaordatetimemarking the absolute expiry date. Originally theexpires_atparameter only accepted adatetimemarking the expiry date.