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 bank managers to set the maximum allowed balance in the bank #2926

Merged
merged 10 commits into from Aug 30, 2019
Merged

[Bank] Allow bank managers to set the maximum allowed balance in the bank #2926

merged 10 commits into from Aug 30, 2019

Conversation

Drapersniper
Copy link
Contributor

@Drapersniper Drapersniper commented Aug 8, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

[p]bankset maxbal can be used to set the maximum bank balance

NOTE: This removes the MAX_BALANCE constant from the bank module

To get the maximum allowed balance use the new bank.get_max_balance() method

Drapersniper added 2 commits Aug 8, 2019
`[p]bankset maxbal` can be used to set the maximum bank balance

Signed-off-by: Guy <guyreis96@gmail.com>
`[p]bankset maxbal` can be used to set the maximum bank balance

Signed-off-by: Guy <guyreis96@gmail.com>
`[p]bankset maxbal` can be used to set the maximum bank balance

Signed-off-by: Guy <guyreis96@gmail.com>
@Drapersniper Drapersniper requested a review from Twentysix26 as a code owner Aug 8, 2019
Signed-off-by: Guy <guyreis96@gmail.com>
@Flame442 Flame442 added the Type: Feature label Aug 9, 2019
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Aug 9, 2019

Lets merge this one AFTER #2925, these 2 conflict, once 2925 is merged I will update this one accordingly.

Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
mikeshardmind pushed a commit that referenced this issue Aug 27, 2019
* Fixed `[p]local start`
Playlist picker auto selects if theres just 1 playlist found
`[p]queue cleanself` added

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

* Black.... you are supposed to trigger before commits

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

* Added `BalanceTooHigh` to the docstrings of `bank.transfer_credits()`

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

* Bring this in line with #2926 to reduce conflicts,`is_global()` already is called inside `get_currency_name` and as such it does not need to be called outside

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

@Drapersniper Drapersniper commented Aug 27, 2019

Conflicts have been resolved, this should be ready to be tested/merged now

@Tobotimus
Copy link
Member

@Tobotimus Tobotimus commented Aug 27, 2019

The upper limit on the balance was put in place to prevent an integer overflow on MongoDB. Specifically, this means the upper limit should always be 2^64 - 1 (although it's probably 2^32 - 1 on 32-bit systems running Mongo, Mongo isn't recommended on 32-bit systems)

@mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Aug 27, 2019

@Tobotimus The limit is still bound to that here as an upper limit (not made clear in the description)

redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
Copy link
Member

@Flame442 Flame442 left a comment

The default bank balance can be higher than the max balance.

changelog.d/2926.breaking.1.rst Outdated Show resolved Hide resolved
redbot/core/bank.py Outdated Show resolved Hide resolved
@@ -550,14 +550,16 @@ def __init__(self, bot: Red):
"Slot cooldown: {slot_time}\n"
"Payday amount: {payday_amount}\n"
"Payday cooldown: {payday_time}\n"
"Amount given at account registration: {register_amount}"
"Amount given at account registration: {register_amount}\n"
"Maximum allowed balance: {maximum_bal}"
Copy link
Member

@Flame442 Flame442 Aug 28, 2019

Choose a reason for hiding this comment

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

Should this be under economyset or bankset? The command to change it is under bankset.

Copy link
Contributor Author

@Drapersniper Drapersniper Aug 29, 2019

Choose a reason for hiding this comment

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

I've added it to both now , i think given that economyset sets the payday values etc its nice to haver it there.

redbot/cogs/bank/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
Copy link
Collaborator

@Cog-CreatorsBot Cog-CreatorsBot left a comment

Changes requested by Flame#2941

@Flame442 Flame442 added the QA: Changes Requested label Aug 28, 2019
@Flame442 Flame442 self-assigned this Aug 28, 2019
Drapersniper and others added 3 commits Aug 28, 2019
… and `[p]bank balance` will set the users balance to max balance if the bank maxbal is lower than the previous user balance

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

@Flame442 Flame442 left a comment

Looking much better, the only other thing is that the payday and starting balance values can be set higher than the max balance if the max balance is changed to lower than either of those values. It isn't really an issue since the balance will be set to where it should be at some point anyway, however it is a small inconsistency. If you can think of a good way to fix it, fix it. If not, don't stress about it.

@@ -766,6 +768,10 @@ def _invalid_amount(amount: int) -> bool:
"""
amount = int(amount)
max_bal = await get_max_balance(guild)
Copy link
Member

@Flame442 Flame442 Aug 29, 2019

Choose a reason for hiding this comment

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

I think raising an error would be better than silently changing the value. Make sure you also change the command to respond to whatever error is raised.

Copy link
Contributor Author

@Drapersniper Drapersniper Aug 29, 2019

Choose a reason for hiding this comment

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

Looking much better, the only other thing is that the payday and starting balance values can be set higher than the max balance if the max balance is changed to lower than either of those values. It isn't really an issue since the balance will be set to where it should be at some point anyway, however it is a small inconsistency. If you can think of a good way to fix it, fix it. If not, don't stress about it.

As discussed in discord
We have 2 options:

  1. Set it the payday amount and initial balance amount with the bankset maxbal command if the old value is higher than the current max

  2. Change the values in bankset and economyset help message to the max value if they are higher than the current max value

My issue with 1 is that since this is in the Bank cog it shouldn't edit settings for the Economy Cog, with 2 it's technically False values

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

@Cog-CreatorsBot Cog-CreatorsBot left a comment

Approved by Flame#2941

@Flame442 Flame442 added QA: Passed and removed QA: Changes Requested labels Aug 29, 2019
@mikeshardmind mikeshardmind merged commit e04eed4 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 bank-changes branch Aug 30, 2019
mikeshardmind pushed a commit that referenced this issue Sep 22, 2019
* Removes `MAX_BALANCE` from bank, user `bank.get_max_balance()` now
`[p]bankset maxbal` can be used to set the maximum bank balance

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

* Pushed the Fix for new issue introduced by #2926

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

* Changelogs

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

* Rename changelog to a misc

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

* Address Flame's review

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

* Correct an outdated reference in the docs

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

* Reword docstring for the RuntimeError

Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA: Passed Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants