Allow helpers to edit their own nomination reason#1841
Conversation
ChrisLovering
force-pushed
the
allow-helpers-to-edit-their-own-nomination-reason
branch
from
a5badc8 to
3b57421
Compare
Xithrius
added
a: backend
ChrisLovering
force-pushed
the
allow-helpers-to-edit-their-own-nomination-reason
branch
from
3b57421 to
3adf7f9
Compare
jchristgit
left a comment
jchristgit
left a comment
There was a problem hiding this comment.
Yup. Yup. Yup. Yup. Yup.
There was a problem hiding this comment.
The implementation looks good on the technical side, but I find the command structure confusing.
It's not clear to me semantically why the command group is the one which accepts the narrower option for a target. In other commands, when a group accepts arguments, it's the opposite - meaning, it accepts the wider definition of a certain argument (e.g infraction search).
While this is easier to write as code, technically speaking it's not much more problematic to replicate the same behavior by letting helpers invoke the edit reason subcommand and raising the appropriate error if another actor is specified. Or, to make the help embed more friendly to helpers, to do the opposite of what you did, and make the group take the wider definition of a target and let helpers use tp edit reason specifically. Still, in that case the functionality of the group command in this way will seem odd to me as the group includes editing end reasons as well.
Also, something that is missing is the ability to raise the help embed for the group. Currently, you make the target mandatory, which raises an error even when you would expect only the help embed to show up.
edit: Thinking about the comment below, I'm probably more in favor of the first approach of keeping the group without arguments, as in the second approach they will see the group taking arguments but not be able to use them?
|
Additionally, I'd argue we should allow the |
ChrisLovering
force-pushed
the
allow-helpers-to-edit-their-own-nomination-reason
branch
2 times, most recently
from
f27a1c6 to
2d1ddc6
Compare
ChrisLovering
dismissed
jchristgit’s stale review
Changed considerably since then.
|
I have changed this back to one command, and change the interface slightly to allow for both helpers and mods to run the command, with validation being ran to ensure non-mod helpers can't edit other user's commands. This also improves the interface for mods wanting to edit the reason for the own nomination of a user, by not needing them to look up the nomination ID. |
This change will allow helpers to run the edit reason command in the Talentpool cog. To ensure that non-mod helpers can only edit their own reasons the interface for the reason command has been changed slightly. If nominee_or_nomination_id is a member or user, then the command edits the currently active nomination for that person. If it's an int, then use this to look up that nomination ID to edit. If no nominator is specified, assume the invoker is editing their own nomination reason. Otherwise, edit the reason from that specific nominator. Raise a permission error if a non-mod staff member invokes this command on a specific nomination ID, or with an nominator other than themselves.
Co-authored-by: Bluenix <bluenixdev@gmail.com>
ChrisLovering
force-pushed
the
allow-helpers-to-edit-their-own-nomination-reason
branch
from
2d1ddc6 to
77f76cb
Compare
wookie184
left a comment
wookie184
left a comment
There was a problem hiding this comment.
We could limit this command to only be run in the nominations channel by helpers, although not really important. This all seems to work well 👍
| await ctx.send(f":x: Maxiumum allowed characters for the reason is {REASON_MAX_CHARS}.") | ||
| await ctx.send(f":x: Maximum allowed characters for the reason is {REASON_MAX_CHARS}.") | ||
| return | ||
| if isinstance(target, Member): |
There was a problem hiding this comment.
The command can resolve to a user. What happens if you try to nominate someone off-server?
There was a problem hiding this comment.
It'll just work with the code below, so I'll swap this around and have it check isinstance of int and then an else to catch Member & User
By swapping the isinstance to check for int, the else block now catches the case where target is Member or User, this allows for editting the nomination reason of members that are off server.
mbaruh
left a comment
mbaruh
left a comment
There was a problem hiding this comment.
I'm not sure it makes sense to nominate someone outside the server at all, but that's out of scope for this PR. Looks good otherwise 👍
jchristgit
deleted the
allow-helpers-to-edit-their-own-nomination-reason
branch
Xithrius
removed
the
s: needs review
6 participants
Closes #1617
This change will allow helpers to run the edit reason command in the Talentpool cog.
To ensure that non-mod helpers can only edit their own reasons the interface for the reason command has been changed slightly.
If nominee_or_nomination_id is a member or user, then the command edits the currently active nomination for that person.
If it's an int, then use this to look up that nomination ID to edit.
If no nominator is specified, assume the invoker is editing their own nomination reason.
Otherwise, edit the reason from that specific nominator.
Raise a permission error if a non-mod staff member invokes this command on a
specific nomination ID, or with an nominator other than themselves.