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.

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

Added toggle for Verify, new default behaviour for welcome message and k!setup #232

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

jplam123
Copy link

@jplam123 jplam123 commented Jul 29, 2021

Summary

close #86
This PR, adds a toggle for verification using k!verifyDM .

Added new default behaviour for welcome messages, they do not happen by default unless one is updated with a welcome message.

Added k!setup command, this blocks all possible sensitive commands until the command is called in the guild by an admin

Checklist

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

  • Have you tested the changes? (pytest & dpytest)
  • Have you followed PEP-8 for naming and styling?
  • Has your code been properly documented with RestructuredText docstrings?
  • Have you added your changes to CHANGELOG.md under the [Unreleased] heading?
  • If your code added new bot commands, have you updated documentation.json?

  • All of this code is your own, or follows the author repo's licence.

jplam123 and others added 25 commits July 23, 2021 13:21
test: added test for introcog without message
Fixed: SQL Insertion error, if statement was set up wrong

Signed-off-by: Jonathan <[email protected]>
@jplam123 jplam123 added the good first issue Good for newcomers label Jul 29, 2021
@jplam123 jplam123 requested a review from Kaspiaan July 29, 2021 13:10
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #232 (4b1ef73) into master (1b3210f) will increase coverage by 0.99%.
The diff coverage is 95.65%.

❗ Current head 4b1ef73 differs from pull request most recent head 0646e2f. Consider uploading reports for the commit 0646e2f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   82.26%   83.26%   +0.99%     
==========================================
  Files          26       26              
  Lines        6452     6973     +521     
==========================================
+ Hits         5308     5806     +498     
- Misses       1144     1167      +23     
Flag Coverage Δ
unittests 83.26% <95.65%> (+0.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
KoalaBot.py 77.00% <25.00%> (-4.53%) ⬇️
cogs/IntroCog.py 84.49% <84.00%> (-0.83%) ⬇️
tests/test_ReactForRole.py 89.17% <88.52%> (-0.01%) ⬇️
tests/test_TwitchAlert.py 95.87% <91.17%> (-0.47%) ⬇️
cogs/TextFilter.py 91.90% <91.66%> (+0.28%) ⬆️
cogs/Verification.py 68.04% <91.66%> (+2.33%) ⬆️
tests/test_TextFilter.py 96.29% <96.15%> (-0.18%) ⬇️
cogs/Announce.py 96.53% <100.00%> (+0.14%) ⬆️
cogs/BaseCog.py 62.29% <100.00%> (+0.95%) ⬆️
cogs/ColourRole.py 89.14% <100.00%> (+0.25%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b3210f...0646e2f. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2021

This pull request introduces 1 alert when merging af59bbb into 85c3be5 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 30, 2021

This pull request introduces 1 alert when merging 721174e into 85c3be5 - view on LGTM.com

new alerts:

  • 1 for Unused import

{
"command": "setup",
"parameters": []
"description": "Allows access to configure the bot, once legal terms are agreed"
Copy link
Member

Choose a reason for hiding this comment

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

please make sure the colon is kept in there, also need to add back in the comma on the line above (52)

Choose a reason for hiding this comment

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

Done

{
"command": "verifyDM",
"parameters": ["toggle"]
"description" "Toggle the verify DM for a guild, that sends the user a list of emails to verify"
Copy link
Member

Choose a reason for hiding this comment

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

please make sure the colon is there between "description" and "Toggle ...", also need to add back in the comma on the line above (347)

Choose a reason for hiding this comment

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

Done


sql_create_guild_dm_email_list_status_table = """
CREATE TABLE IF NOT EXISTS GuildDMEmailListStatus(
guild_id integer NOT NULL PRIMARY KEY,
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be changed to text later down the line. Same thing on 136

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
guild_id integer NOT NULL PRIMARY KEY,
guild_id text NOT NULL PRIMARY KEY,

sql_create_guild_setup_table = """
CREATE TABLE IF NOT EXISTS GuildSetupStatus(
guild_id integer NOT NULL PRIMARY KEY,
accepted_setup BOOLEAN NOT NULL CHECK (accepted_setup IN (0, 1))
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth DEFAULT ing this?

guild_id = ?"""
self.db_execute_commit(sql_update_dm_email_list_status, args=[toggle, guild_id])

def remove_dm_email_list_status(self, guild_id):
Copy link
Member

Choose a reason for hiding this comment

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

If possible can you create tests for these new methods

Choose a reason for hiding this comment

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

Done

@lgtm-com
Copy link

lgtm-com bot commented Aug 4, 2021

This pull request introduces 1 alert when merging ea4bb34 into d666525 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 4, 2021

This pull request introduces 1 alert when merging 7850f15 into d666525 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2021

This pull request introduces 2 alerts when merging b5f2a40 into 1b3210f - view on LGTM.com

new alerts:

  • 2 for Unused import

@JayDwee
Copy link
Member

JayDwee commented Aug 5, 2021

Please remove the unused imports specified by LGTM

@Jaddison011
Copy link

Please remove the unused imports specified by LGTM

Done

Copy link
Member

@JayDwee JayDwee left a comment

Choose a reason for hiding this comment

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

Couple things to change, also need to check if setup is required for all our commands.
Also need to actually make the terms and conditions, as we currently only have a privacy policy.
Also need to check if privacy policy should be agreed to within setup. But in the meantime there are some things to change

@@ -3,6 +3,12 @@ All notable changes to KoalaBot will be documented in this file.
A lot of these commands will only be available to administrators

## [Unreleased]
### 29-07-2021
Copy link
Member

Choose a reason for hiding this comment

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

Changelog should be in the same format as previously. Use subheadings for different Cogs/ base KoalaBot, and 'other' for anything else mainly dev related

@@ -97,6 +97,13 @@ def is_admin(ctx):
return ctx.author.guild_permissions.administrator or is_dpytest


def terms_agreed(ctx):
Copy link
Member

Choose a reason for hiding this comment

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

When this fails, a 'list index out of range' error is given, either displayed in console, or to the user. This should not be visible.
image
image

@@ -160,6 +167,16 @@ async def on_command_error(ctx, error):
elif isinstance(error, commands.CommandOnCooldown):
await ctx.send(embed=error_embed(description=f"{ctx.author.mention}, this command is still on cooldown for "
f"{str(error.retry_after)}s."))
elif isinstance(error, commands.CheckFailure):
if database_manager.fetch_guild_setup_status(ctx.guild.id) == 0:
await ctx.send(embed=error_embed(description="In order to use this command. You must agree to the Terms & Conditions " \
Copy link
Member

Choose a reason for hiding this comment

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

This error doesn't seem to show up. Instead nothing is sent to the user. needs fixing

"For legal documents relating to this, please view the following link: http://legal.koalabot.uk/ " \
"Use k!setup to agree"
for channel in guild.text_channels:
if channel.permissions_for(guild.me).send_messages:
Copy link
Member

Choose a reason for hiding this comment

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

if it doesn't have access, what happens? Also this would be very infuriating to most people as when joining KoalaBot may post in a public announcement channel. Instead it would be best to DM the user that invited Koala, or just wait to send this message when a Koala command is sent in any channel, and respond with this instead

KoalaBot.logger.info(f"KoalaBot joined new guild, id = {guild.id}, name = {guild.name}.")
await self.send_setup_message(guild)
Copy link
Member

Choose a reason for hiding this comment

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

move to the failed terms agreed check

Comment on lines +69 to +70
DBManager.insert_setup_status(guild.id)
DBManager.update_guild_setup_status(guild.id)
Copy link
Member

Choose a reason for hiding this comment

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

can these be added to a pytest fixture?

@@ -129,13 +131,123 @@ def create_base_tables(self):
welcome_message text
);"""

sql_create_guild_setup_table = """
CREATE TABLE IF NOT EXISTS GuildSetupStatus(
guild_id integer NOT NULL PRIMARY KEY,
Copy link
Member

Choose a reason for hiding this comment

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

Change IDs to text


sql_create_guild_dm_email_list_status_table = """
CREATE TABLE IF NOT EXISTS GuildDMEmailListStatus(
guild_id integer NOT NULL PRIMARY KEY,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
guild_id integer NOT NULL PRIMARY KEY,
guild_id text NOT NULL PRIMARY KEY,

Comment on lines +141 to +146
sql_create_guild_dm_email_list_status_table = """
CREATE TABLE IF NOT EXISTS GuildDMEmailListStatus(
guild_id integer NOT NULL PRIMARY KEY,
dm_email_list_status BOOLEAN NOT NULL CHECK (dm_email_list_status IN (0, 1))
);
"""
Copy link
Member

Choose a reason for hiding this comment

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

should this be moved to Verification.py?

removal of line as per @JayDwee's suggestion

Co-authored-by: Jack Draper <[email protected]>
@Kaspiaan Kaspiaan assigned Kaspiaan and unassigned Jaddison011 Oct 18, 2021
@Kaspiaan Kaspiaan assigned Copper76 and unassigned Kaspiaan Oct 18, 2021
@VirajShah18 VirajShah18 marked this pull request as draft November 9, 2021 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove introductory DM messages when joining servers
5 participants