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] Add discord.Object support to bank.get_balance #4654

Open
wants to merge 3 commits into
base: V3/develop
Choose a base branch
from

Conversation

goettner
Copy link
Contributor

@goettner goettner commented Dec 3, 2020

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

As detailed in #4391, much of the Core Bank API functions only accepted discord.Member or discord.User input types for routine functions such as get_balance(). This is problematic, given the unnecessary and possibly sensitive information that is included with a Member or User object. This can be improved by changing the calls' input types to enable passing it a discord.Object instead, in order to prevent sensitive information being unnecessarily acquired.

I also added test cases to tests/cogs/test_economy.py, in order to test the ability of these API calls to function correctly with a discord.Object as input, and the necessary infrastructure in pytest/core.py

@Drapersniper
Copy link
Contributor

Drapersniper commented Dec 3, 2020

You've misunderstood my last statement I think, discord objects only have the .id attributes and not the rest of the other attributes that a user or member has, as such special handling is required here.

The remaining bank apis should also accept the discord.Object input.

@goettner
Copy link
Contributor Author

goettner commented Dec 3, 2020

Okay, I see. With that in mind then, how do you propose handling situations where other object members are used? Specifically with this block in bank.get_account() in mind:
Screen Shot 2020-12-03 at 11 20 48 AM

@Drapersniper
Copy link
Contributor

Drapersniper commented Dec 3, 2020

Okay, I see. With that in mind then, how do you propose handling situations where other object members are used? Specifically with this block in bank.get_account() in mind:
Screen Shot 2020-12-03 at 11 20 48 AM

For one discord.Object could only ever replace users and not members, since members have an additional guild attribute. This would mean objects can ever only be used in global banks

For the attribute access, I would personally add placeholders, such as "John doe" for the display name access since that doesn't exist on an Object

@goettner
Copy link
Contributor Author

goettner commented Dec 4, 2020

For the attribute access, I would personally add placeholders, such as "John doe" for the display name access since that doesn't exist on an Object

Pushed some changes based on your feedback, let me know what you think.

@@ -620,12 +630,12 @@ async def get_account(member: Union[discord.Member, discord.User, discord.Object
else:
all_accounts = await _config.all_members(member.guild)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line will and should fail error with discord.Object. Even if it were to succeed with Object, get_default_balance below would throw a runtime error

@Flame442 Flame442 added Category: Core - API - Bank This is related to the core Bank API. Type: Enhancement Something meant to enhance existing Red features. labels Jan 8, 2021
@Jackenmen Jackenmen added this to the 3.5.0 milestone Apr 21, 2021
@Jackenmen Jackenmen modified the milestones: 3.5.0, 3.6.0 Nov 11, 2021
@Flame442 Flame442 added the No Activity When a PR or issue hasn't had activity in a while. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core - API - Bank This is related to the core Bank API. No Activity When a PR or issue hasn't had activity in a while. Type: Enhancement Something meant to enhance existing Red features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants