We auto create role #244
We auto create role #244
Conversation
Signed-off-by: Jonathan <[email protected]>
Signed-off-by: Jonathan <[email protected]>
Signed-off-by: Jonathan <[email protected]>
Signed-off-by: Jonathan <[email protected]>
Signed-off-by: Jonathan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #244 +/- ##
==========================================
- Coverage 82.53% 82.41% -0.13%
==========================================
Files 27 27
Lines 6534 6630 +96
==========================================
+ Hits 5393 5464 +71
- Misses 1141 1166 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
This pull request introduces 2 alerts when merging cc977eb into 29abb7b - view on LGTM.com new alerts:
|
Signed-off-by: Jonathan <[email protected]>
|
This pull request introduces 1 alert when merging 19fbbe1 into 29abb7b - view on LGTM.com new alerts:
|
Signed-off-by: Jonathan <[email protected]>
|
This pull request introduces 1 alert when merging 2a09754 into 29abb7b - view on LGTM.com new alerts:
|
Signed-off-by: Jaddison011 <[email protected]>
Signed-off-by: Jaddison011 <[email protected]>
|
This pull request introduces 1 alert when merging 6075450 into 29abb7b - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4df82a6 into 29abb7b - view on LGTM.com new alerts:
|
Signed-off-by: Jonathan <[email protected]>
…aBot into WEAutoCreateRole Signed-off-by: Jonathan <[email protected]>
|
This pull request introduces 1 alert when merging 87dfcf2 into 29abb7b - view on LGTM.com new alerts:
|
| @@ -44,6 +50,13 @@ def verify_is_enabled(ctx): | |||
|
|
|||
| return result or (str(ctx.author) == KoalaBot.TEST_USER and KoalaBot.is_dpytest) | |||
|
|
|||
|
|
|||
| def check_if_role_exists(guild, university): | |||
There was a problem hiding this comment.
Please add a docstring above this, even though it's self explanitory it's still worth having one
| self.DBManager.db_execute_commit(verified_table) | ||
| self.DBManager.db_execute_commit(non_verified_table) | ||
| self.DBManager.db_execute_commit(role_table) | ||
| self.DBManager.db_execute_commit(re_verify_table) | ||
| drop_universities = "DROP TABLE Universities" |
There was a problem hiding this comment.
Just wondering why you drop the table before re-creating it each time?
| @@ -166,6 +191,9 @@ async def enable_verification(self, ctx, suffix=None, role=None): | |||
| :param role: the role to give users with that email verified (e.g. @students) | |||
| :return: | |||
| """ | |||
| role = role | |||
| if not suffix: | |||
| await ctx.send("hi") | |||
There was a problem hiding this comment.
Also just wondering the point in this considering the case of suffix not beign provided would be caught afterwards anyway
| role_id = f"<@&{str(role.id)}>" | ||
| await self.verify_university(ctx, email_suffix, role_id, university) | ||
|
|
||
| def insert_university_csv(self): |
There was a problem hiding this comment.
Please include a test for this if possible
There was a problem hiding this comment.
Please make sure all methods have tests where possible, also noticed some stuff I'm not sure the use behind such as dropping the table each time the init is called regardless of anythign else, also a random ctx.send("hi"), not sure if that was left over from debugging or I might be missing something.
|
To add onto the testing portion, have a look at the automated tests made on python 3.7 as some of the new tests failed. Another issue is the code coverage but this can be rectified by ensuring your tests cover as much as possible. |
Summary
Checklist
CHANGELOG.mdunder the[Unreleased]heading?documentation.json?