Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Warnings]Make it possible to add reason with unwarn #3490

Merged
merged 11 commits into from Feb 14, 2020
Merged

[Warnings]Make it possible to add reason with unwarn #3490

Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
83dfae2
Unwarn now able to have a reason
Dav-Git Feb 2, 2020
9b6c7fd
black
Dav-Git Feb 2, 2020
d583621
Update string to say Run instead of Do
Dav-Git Feb 3, 2020
67b1c7a
Make error for unregistered reason less agressive
Dav-Git Feb 3, 2020
0820a41
Removing unneccessary lines and not putting the user input through th…
Dav-Git Feb 3, 2020
92b3bb0
Because black hates me, here black formatting with adjusted line length
Dav-Git Feb 3, 2020
3859642
Trigger CI
Dav-Git Feb 7, 2020
bc94009
Now always sendes a message when an invalid reason is passed, not onl…
Dav-Git Feb 9, 2020
f8c2540
That should do the trick
Dav-Git Feb 9, 2020
d99c11f
Correct
Dav-Git Feb 9, 2020
0620dde
Make Reason optional
Dav-Git Feb 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 36 additions & 6 deletions redbot/cogs/warnings/warnings.py
View file
Open in desktop
Expand Up @@ -198,7 +198,7 @@ async def reasonlist(self, ctx: commands.Context):
for r, v in registered_reasons.items():
if await ctx.embed_requested():
em = discord.Embed(
title=_("Reason: {name}").format(name=r), description=v["description"]
title=_("Reason: {name}").format(name=r), description=v["description"],
)
em.add_field(name=_("Points"), value=str(v["points"]))
msg_list.append(em)
Expand Down Expand Up @@ -226,7 +226,9 @@ async def actionlist(self, ctx: commands.Context):
if await ctx.embed_requested():
em = discord.Embed(title=_("Action: {name}").format(name=r["action_name"]))
em.add_field(name=_("Points"), value="{}".format(r["points"]), inline=False)
em.add_field(name=_("Exceed command"), value=r["exceed_command"], inline=False)
em.add_field(
name=_("Exceed command"), value=r["exceed_command"], inline=False,
)
em.add_field(name=_("Drop command"), value=r["drop_command"], inline=False)
msg_list.append(em)
else:
Expand Down Expand Up @@ -371,7 +373,8 @@ async def warnings(self, ctx: commands.Context, user: Union[discord.Member, int]
description=user_warnings[key]["description"],
)
await ctx.send_interactive(
pagify(msg, shorten_by=58), box_lang=_("Warnings for {user}").format(user=user)
pagify(msg, shorten_by=58),
box_lang=_("Warnings for {user}").format(user=user),
)

@commands.command()
Expand Down Expand Up @@ -400,13 +403,16 @@ async def mywarnings(self, ctx: commands.Context):
description=user_warnings[key]["description"],
)
await ctx.send_interactive(
pagify(msg, shorten_by=58), box_lang=_("Warnings for {user}").format(user=user)
pagify(msg, shorten_by=58),
box_lang=_("Warnings for {user}").format(user=user),
)

@commands.command()
@commands.guild_only()
@checks.admin_or_permissions(ban_members=True)
async def unwarn(self, ctx: commands.Context, user: Union[discord.Member, int], warn_id: str):
async def unwarn(
self, ctx: commands.Context, user: Union[discord.Member, int], warn_id: str, reason: str,
Dav-Git marked this conversation as resolved.
Show resolved Hide resolved
):
"""Remove a warning from a user."""

guild = ctx.guild
Expand All @@ -422,6 +428,29 @@ async def unwarn(self, ctx: commands.Context, user: Union[discord.Member, int],
if user_id == ctx.author.id:
return await ctx.send(_("You cannot remove warnings from yourself."))

custom_allowed = await self.config.guild(ctx.guild).allow_custom_reasons()
guild_settings = self.config.guild(ctx.guild)
reason_type = None
async with guild_settings.reasons() as registered_reasons:
if reason.lower() not in registered_reasons:
msg = _("That is not a registered reason.")
if custom_allowed:
reason_type = {"description": reason}
elif (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually stating the valid reasons, when custom_allowed is disallowed and reason.lower() not in registered_reasons would be more user friendly (something along the lines of "Please use one of the following: ...")

Right now if reason.lower() not in registered_reasons and not custom_allowed this just fails silently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look at this when I return home later today

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would then require to be edited in the warn command as well. My knowledge in string formatting is limited (everyone tells me my code is to slow) that I'd rather leave that up to someone else as it also doesn't really fit in this PR in my opinion. If you really want to see this included I'll need some guidance via discord.

I'm guessing that reason_lower is a list of strings that in one way ore another can be concatinated and passed to an embed which is then sent to the user. If we exceed the embed limit with the amount of registered reasons that's going to require some logic I don't know how to code and I'm not really sure if it's worth it at that point. Leaving it with just the instructions on how to enable custom warnings seems reasonable to me.

ctx.guild.owner == ctx.author
or ctx.channel.permissions_for(ctx.author).administrator
or await ctx.bot.is_owner(ctx.author)
):
msg += " " + _(
"Run `{prefix}warningset allowcustomreasons true` to enable custom "
"reasons."
).format(prefix=ctx.prefix)
return await ctx.send(msg)
else:
reason_type = registered_reasons[reason.lower()]
if reason_type is None:
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should return a user friendly message when a valid reason is not present, just silently doing nothing is very confusing imo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If another QA/Core-dev disagrees let me know and i'll review this as it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree we shouldn't fail silently, though I don't think we should list all registered reasons, especially not in scope of this PR.


member_settings = self.config.member(member)
current_point_count = await member_settings.total_points()
await warning_points_remove_check(self.config, ctx, member, current_point_count)
Expand All @@ -433,14 +462,15 @@ async def unwarn(self, ctx: commands.Context, user: Union[discord.Member, int],
await member_settings.total_points.set(current_point_count)
user_warnings.pop(warn_id)
try:
reason_msg = "{description}".format(description=reason_type["description"])
Copy link
Member

@Jackenmen Jackenmen Feb 9, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be (though this part might not even exist after allowing any reason):

Suggested change
reason_msg = "{description}".format(description=reason_type["description"])
reason_msg = reason_type["description"]

await modlog.create_case(
self.bot,
ctx.guild,
ctx.message.created_at,
"unwarned",
member,
ctx.message.author,
None,
reason_msg,
until=None,
channel=None,
)
Expand Down