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

Allow users to opt-out of overdrafting #87

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

YtvwlD
Copy link
Member

@YtvwlD YtvwlD commented Jan 20, 2020

To achieve this there's a new user attribute: can_overdraw.
This is enabled by default for all new and existing users.
If this is set to false, this user's balance can go below zero.
Drinks are hidden from the user page when their price is higher then the user's balance and the user has can_overdraw disabled.

This is not yet fully exposed via the API - the attribute is returned but can't be changed and the validation is still being performed. This has to be added before merging this PR.

To achieve this, there's a new user attribute: can_overdraw.
This is enabled by default for all new and existing users.
If this is set to false, this user's balance can go below zero.
As a first step this throws validation errors
which somehow works but is not very user-friendly.
Also, this is not yet fully exposed via the API.
This way we can create new alerts in other places that
don't disappear by themselves.
@YtvwlD YtvwlD requested a review from nomaster January 20, 2020 08:36
Copy link
Member

@nomaster nomaster left a comment

Choose a reason for hiding this comment

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

Thanks for the good idea and implementation! How do we deal with the API standard? Is this a "proprietary" addition?

@@ -28,4 +28,4 @@ if (navigator.userAgent.match(/(ipod|iphone|ipad)/i)) {
}

//hide notification bars after a few seconds
$('.alert').delay(10000).fadeOut('slow');
$('.flash').delay(10000).fadeOut('slow');
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to #88? General UI changes shouldn't happen in this PR.

@@ -1,4 +1,4 @@
- flash.each do |level, message|
.alert(class="alert-#{level}")
.flash.alert(class="alert-#{level}")
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -25,17 +45,25 @@ class UserTest < ActiveSupport::TestCase
end

test "should pay" do
assert users(:one).payment(rand(500)), "Failed to pay"
assert users(:one).payment(rand(99)), "Failed to pay"
Copy link
Member

Choose a reason for hiding this comment

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

why the change to rand(99)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants