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.

Sign up for GitHub

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

Jump to bottom

Add support for Currency#enabled (aka #visible, closes #818) #855

Merged
merged 1 commit into from May 24, 2018
Merged

Add support for Currency#enabled (aka #visible, closes #818) #855

merged 1 commit into from May 24, 2018

Conversation

shal
Copy link

@shal shal commented Apr 12, 2018

No description provided.

@yivo yivo added the PR: WIP label Apr 12, 2018
@shal shal requested a review from yivo April 12, 2018 17:37
@shal shal changed the title [WIP] Add support field Currency#visible (#818) [WIP] Add support field Currency#visible (closes #818) Apr 13, 2018
@yivo yivo changed the title [WIP] Add support field Currency#visible (closes #818) Add support field Currency#visible (closes #818) Apr 17, 2018
yivo
yivo suggested changes Apr 17, 2018
View reviewed changes
Copy link

@yivo yivo left a comment •
edited

Choose a reason for hiding this comment

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

Check app/api VERY carefully.

Use this scope to fetch enabled accounts:
2018-04-17--11-27-18
2018-04-17--11-27-14

Visible ony:
2018-04-17--11-25-24
2018-04-17--11-24-28
2018-04-17--11-24-11
2018-04-17--11-23-40
2018-04-17--11-22-57

app/api:
2018-04-17--11-21-33
2018-04-17--11-21-29
2018-04-17--11-20-53
2018-04-17--11-20-34
2018-04-17--11-19-57
2018-04-17--11-19-51

@yivo yivo modified the milestones: 1.7 - Multi-Fiat support and display currency, 1.8 - Support for ERC20 tokens Apr 18, 2018
yivo
yivo suggested changes Apr 24, 2018
View reviewed changes
spec/factories/currencies.rb Outdated
@@ -66,5 +66,15 @@
transaction_url_template: 'https://bithomp.com/explorer/#{address}',
deposit_confirmations: 1
end

trait :invisible do
Copy link

Choose a reason for hiding this comment

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

@ashanaakh This is not good and should be removed. We seed currencies and markets before every example so it is not possible to use factories for currencies or markets as usually. Please use before & after callback for the spec.

spec/api/management_api_v1/withdraws_spec.rb Outdated
let(:account) { member.accounts.with_currency(currency).first }

before { account.update!(balance: 1.2) }

after { Currency.destroy(invisible_currency) }
Copy link

Choose a reason for hiding this comment

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

@ashanaakh This should be removed too.

spec/api/management_api_v1/withdraws_spec.rb Outdated
@@ -107,6 +129,12 @@ def request
expect(account.reload.balance).to eq(1.2 - amount)
expect(account.reload.locked).to eq amount
end

it 'validates currency visibility' do
Copy link

Choose a reason for hiding this comment

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

@ashanaakh Better to use validates visibility of currency

spec/api/management_api_v1/withdraws_spec.rb Outdated
@@ -84,10 +97,19 @@ def request
amount: amount,
rid: Faker::Bitcoin.address }
end
let :bad_data do
Copy link

Choose a reason for hiding this comment

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

@ashanaakh Use context 'invisible currency' and before after changing visibility instead of producing lot of magic lets.

Copy link

Choose a reason for hiding this comment

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

context 'invisible currency' do
  before { currency.update!(visible: false) }
  after { currency.update!(visible: true) }
  it '...' do
    # ...
  end
end

spec/api/api_v2/solvency_spec.rb Outdated
@@ -19,6 +24,13 @@
expect(response.code).to eq '422'
end

it 'should validate currency visibility' do
Copy link

Choose a reason for hiding this comment

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

@ashanaakh Don't use should, just it 'validates visibility of currency'.

@shal shal changed the title Add support field Currency#visible (closes #818) [WIP] Add support field Currency#visible (closes #818) Apr 24, 2018
@yivo yivo removed this from the 1.8 - Support for ERC20, Event API milestone May 3, 2018
@yivo yivo changed the title [WIP] Add support field Currency#visible (closes #818) Add support field Currency#visible (closes #818) May 3, 2018
@shal
Copy link
Author

shal commented May 5, 2018

@yivo rebased on master.

@yivo
Copy link

yivo commented May 14, 2018

@ashanaakh Please, change base to 1-8-stable.

@shal shal changed the base branch from master to 1-8-stable May 14, 2018 14:43
@shal
Copy link
Author

shal commented May 14, 2018

@yivo Done 😄

yivo
yivo suggested changes May 15, 2018
View reviewed changes
Copy link

@yivo yivo left a comment

Choose a reason for hiding this comment

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

Looks good.

spec/api/management_api_v1/withdraws_spec.rb Outdated
before { Currency.first.update!(visible: false) }
after { Currency.first.update!(visible: true) }
it 'validates visibility of currency' do
params = {
Copy link

Choose a reason for hiding this comment

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

@ashanaakh In this call we don't need amount and rid since is returns collection.

spec/api/management_api_v1/deposits_spec.rb Outdated
params = {
uid: member.authentications.first.uid,
currency: Currency.first.code,
amount: 0.1
Copy link

Choose a reason for hiding this comment

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

@ashanaakh No need for field amount for this call.

app/models/currency.rb Outdated
@@ -106,8 +105,8 @@ def as_json(*)
end

def summary
Copy link

Choose a reason for hiding this comment

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

@ashanaakh This is used in admin panel on dashboard page. Admin should see all accounts.

@yivo
Copy link

yivo commented May 16, 2018 •
edited

@ashanaakh Yesterday we discussed this with QA. We found some bugs with address generation. I will submit them later. Also we found that name enabled fits much better than visible. Please, do visible -> enabled including database migration & admin panel.

@pkucherenk0 pkucherenk0 mentioned this pull request May 16, 2018
Closed
@shal shal changed the title Add support field Currency#visible (closes #818) Add support field Currency#enabled (closes #818) May 22, 2018
yivo
yivo suggested changes May 23, 2018
View reviewed changes
app/api/api_v2/deposits.rb
@@ -12,7 +12,7 @@ class Deposits < Grape::API

Copy link

Choose a reason for hiding this comment

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

2018-05-23--16-06-06

2018-05-23--15-59-29

2018-05-23--15-59-24

2018-05-23--15-57-53

2018-05-23--15-57-44

2018-05-23--15-57-34

2018-05-23--15-57-16

2018-05-23--15-55-37

@yivo yivo mentioned this pull request May 23, 2018
Closed
@yivo yivo changed the title Add support field Currency#enabled (closes #818) Add support for Currency#enabled (aka #visible, closes #818) May 24, 2018
@yivo yivo added PR: Approved and removed PR: QA labels May 24, 2018
@mod mod merged commit e156dd8 into openware:1-8-stable May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yivo yivo yivo approved these changes

Assignees
No one assigned
Labels
PR: Approved v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shal @yivo @mod