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

[Bank] Allow Bot Owner/Guild Owners to remove invalid users from the bank #2845

Merged
merged 34 commits into from Aug 30, 2019
Merged

[Bank] Allow Bot Owner/Guild Owners to remove invalid users from the bank #2845

merged 34 commits into from Aug 30, 2019

Conversation

Drapersniper
Copy link
Contributor

@Drapersniper Drapersniper commented Jul 8, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Sinbad Please dont kill me for using a protected member of Config

Add 2 new commands removedead which clears all users who are in the database but are no longer in guild

and memberdelete which complete delete a users bank account

Both can only be run by guild owners

@Drapersniper Drapersniper requested a review from palmtree5 as a code owner Jul 8, 2019
@Drapersniper Drapersniper changed the title [Bank] Allow guild owners to remove remove members from bank (+ clean bank from users who are no longer in server) [Bank] Allow guild owners to remove members from bank (+ clean bank from users who are no longer in server) Jul 8, 2019
@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Jul 8, 2019

I'm not sure this is a great idea to add prior to proper bulk interfaces in config.

If we do decide to add this as is, it will at minimum need a # FIXME: use-config-bulk-update (or some similar to ensure someone goes back for it later). I'd recommend an additional comment warning against blindly copying the method which this is done with as well to prevent people from assuming this is safe in all cases (it very much isn't).

Have not tested this, though the logic itself should be fine from a brief glance.

…plemented

Added a brief warning to warn devs not to use the `_get_base_group` as it can mess up their config files
Removed a redundant existence check
@Flame442 Flame442 self-assigned this Jul 9, 2019
@Flame442 Flame442 added the Type: Feature label Jul 9, 2019
Copy link
Member

@Flame442 Flame442 left a comment

I still think this needs some more discussion, however I have reviewed the code as-is so it can be improved.

Additional comments that don't apply to a particular line:
-All of the places you use member=member, you might want to use member=member.display_name instead so that it will use nicknames.
-It would be nice if this supported global banks and had an extra check that the invoker is a bot owner when the bank is global.

redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 10, 2019

-It would be nice if this supported global banks and had an extra check that the invoker is a bot owner when the bank is global.

I agree but how would this work? Since Global bank means the balance works across servers.

Or do you mean that the cleanup member command should be be available to the bot owner?

@Flame442
Copy link
Member

@Flame442 Flame442 commented Jul 10, 2019

If the bank is global, accounts will be tied to Users (rather than Members). Bot owners should be able to delete a user's account when the bank is global and possibly clear all users that no longer share a guild with the bot.

Renamed `bank_local_clean` to `bank_prune`
Added support for global banks to `bank_prune`
`bank_prune` will now raise `BankPruneError` if trying to prune a local bank and `guild` is not supplied

Renamed `cleanup` subgroup to `prune`
`prune` subgroup will have 3 commands:
  `user`   :  Deletes the bank account for the specified member : Accepts `Union[discord.Member, discord.User, int]`
  `global` :  Prune global bank accounts for all users who no longer share a server with the bot
  `local`  :  Prune local bank accounts for all users who are no longer in the guild

Changed check for `prune` subgroup to be `@check_global_setting_admin()`
[p]bank prune local  : Can be run by Guild owners only
[p]bank prune global : Can be run by Bot Owner only
[p]bank prune user   : Can be run by Admins, Guild owners and Bot Owner
@Drapersniper Drapersniper requested a review from Twentysix26 as a code owner Jul 11, 2019
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 11, 2019

Global support has been added, also reworked perms and names:

[p]bank prune local : Can be run by Guild owners only
[p]bank prune global : Can be run by Bot Owner only
[p]bank prune user : Can be run by Admins, Guild owners and Bot Owner

Copy link
Member

@Flame442 Flame442 left a comment

After deleting my bank account with [p]bank prune user Flame yes, [p]bank balance returns this error (I'm not cropping it because I don't know how much is useful and I'm too tired to check):

Exception in command 'bank balance'
Traceback (most recent call last):
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\discord\ext\commands\core.py", line 63, in wrapped
    ret = await coro(*args, **kwargs)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\cogs\economy\economy.py", line 157, in balance
    bal = await bank.get_balance(user)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\core\bank.py", line 139, in get_balance
    acc = await get_account(member)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\core\bank.py", line 492, in get_account
    all_accounts = await _conf.all_users()
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\core\config.py", line 1057, in all_users
    return await self._all_from_scope(self.USER)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\core\config.py", line 987, in _all_from_scope
    ret[int(k)] = data
ValueError: invalid literal for int() with base 10: 'name'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\discord\ext\commands\bot.py", line 860, in invoke
    await ctx.command.invoke(ctx)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\redbot\core\commands\commands.py", line 570, in invoke
    await super().invoke(ctx)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\discord\ext\commands\core.py", line 1127, in invoke
    await ctx.invoked_subcommand.invoke(ctx)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\discord\ext\commands\core.py", line 698, in invoke
    await injected(*ctx.args, **ctx.kwargs)
  File "c:\Users\Flame\Desktop\Random\TestingRedbot\reddev\VENV\lib\site-packages\discord\ext\commands\core.py", line 72, in wrapped
    raise CommandInvokeError(exc) from exc
discord.ext.commands.errors.CommandInvokeError: Command raised an exception: ValueError: invalid literal for int() with base 10: 'name'

This same error appears for other commands that interact with the bank, such as [p]payday. I can't really test this effectively with this happening.

redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
redbot/core/errors.py Show resolved Hide resolved
Cog-CreatorsBot
Cog-CreatorsBot previously requested changes Jul 11, 2019
Copy link
Collaborator

@Cog-CreatorsBot Cog-CreatorsBot left a comment

Changes requested by Flame#2941

-Flame

@Drapersniper Drapersniper requested a review from tekulvw as a code owner Jul 12, 2019
@mikeshardmind mikeshardmind self-requested a review Jul 12, 2019
Copy link
Member

@Flame442 Flame442 left a comment

Looking a lot better.

redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
Fixed [p]bank prune user, When run via DM it will now return an error message to the user (Thanks jack1142)
@Drapersniper Drapersniper changed the title [Bank] Allow guild owners to remove members from bank (+ clean bank from users who are no longer in server) [Bank] Allow Bot Owner/Guild Owners to remove invalid users from the bank Jul 12, 2019
Flame442
Flame442 previously approved these changes Jul 12, 2019
Copy link
Member

@Flame442 Flame442 left a comment

All of the things I wanted to see fixed have been fixed. Thanks for working with me.

@mikeshardmind mikeshardmind dismissed Cog-CreatorsBot’s stale review Jul 12, 2019

Per request in discord until a command is added for this.

…ordBot into removedead

Signed-off-by: Draper <guyreis96@gmail.com>

# Conflicts:
#	redbot/cogs/economy/economy.py
#	redbot/core/bank.py
#	redbot/core/config.py
Signed-off-by: Draper <guyreis96@gmail.com>
Copy link
Member

@Flame442 Flame442 left a comment

New changes requested for the changelog.

changelog.d/2845.feature.rst Outdated Show resolved Hide resolved
changelog.d/2845.feature.rst Outdated Show resolved Hide resolved
changelog.d/2845.misc.rst Outdated Show resolved Hide resolved
changelog.d/2845.feature.rst Outdated Show resolved Hide resolved
changelog.d/2845.misc.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@Cog-CreatorsBot Cog-CreatorsBot left a comment

Changes requested by Flame#2941

Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Copy link
Collaborator

@Cog-CreatorsBot Cog-CreatorsBot left a comment

Approved by Flame#2941

@mikeshardmind mikeshardmind merged commit 7959e0c into Cog-Creators:V3/develop Aug 30, 2019
1 check passed
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone Aug 30, 2019
@Drapersniper Drapersniper deleted the removedead branch Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants