Merge infraction edit commands#457
Conversation
Discord.py's internals use the __func__ attribute of special methods (cog_command_error, cog_check, cog_before_invoke, cog_after_invoke). Therefore the methods must be bound methods rather than static so that the attribute exists.
Both the duration and the reason can be edited with the new command. * Remove try-except; the default error handler is already adequate * Remove the new reason from the confirmation message * Simplify humanisation of the timestamp in the confirmation message * Add a converter to support permanent durations
MarkKoz
added
a: moderation
Your solution looks smart. I'm debating with myself whether or not I should even add, so take this suggestion with a pinch of salt. Your current code does not give feedback when the duration was not changed. If we look at this from a human factors engineering perspective, this means we also don't get feedback about not having changed the duration when we most need it: When we tried to change duration, but made a mistake in the duration string. Obviously, the absence of feedback could be seen as feedback itself, but with the above perspective in mind, we now have to rely on human memory for the actor to spot that something that should have been there is not. Since we don't edit the duration of an infraction often, this increases the risk of the actor missing the fact that the duration feedback is absent. A solution would be to always indicate if the duration field was touched, change or not. This would introduce slightly more noise in the confirmation message, but since we infrequently alter infractions and it would be only a couple of words, I'd say that's not a big problem. That said, we're not designing a nuclear power plant or oil refinery, so scrutiny up to that point is probably over the top. Feel free to ignore it.
We could think of adding more validators to the serializer in the back-end. We already have a couple of them and I already proposed two other ones in python-discord/site#260. |
SebastiaanZ
left a comment
SebastiaanZ
left a comment
There was a problem hiding this comment.
One small datetime string thing.
Not explicitly. But if the "confirmation message" that is sent afterwards does not mention the duration but only the reason, then one can assume the duration did not change. Just want to be clear: are you saying that is not adequate? If that's the case, then sure I can agree to this:
|
|
Yeah he's saying that in this section:
A little convoluted to read, but the meaning is there haha. I agree explicit feedback is best for now. |
The format used is %Y-%m-%d %H:%M.
|
I've tested my changes and all is good. 👍 |
The watchchannel ABC defined its own private utility function to format ISO datetime strings to something more human-readable. I have removed this private utility function and replaced the calls to it with calls to the new `format_infraction` utility function defined in bot.utils.time. In addition, I've changed the utility function to use `dateutil` to parse the datetime string, since `dateutil.parser.isoparse` supports the strings our API generates out of the box. With the built-in `datetime.datetime.fromisoformat`, we needed to prepare the string by slicing of the `Z` timezone indicator.
4 participants
Closes #349.
Also made the format of infraction timestamps consistent.
Some things to note:
!infraction edit 1337 50(missing a unit for the duration) will make the reason50and expiration unchanged.infraction editcan be used.