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

Feature: add admin currencies fetch filters #2678

Merged
merged 4 commits into from
Nov 23, 2020

Conversation

oyershov
Copy link

@oyershov oyershov commented Sep 3, 2020

No description provided.

@@ -112,7 +112,12 @@ class Currencies < Grape::API
get '/currencies' do
admin_authorize! :read, Currency

search = Currency.ransack(type_eq: params[:type])
ransack_params = Helpers::RansackBuilder.new(params)
.eq(:type, :deposit_enabled, :withdrawal_enabled, :visible)
Copy link
Member

Choose a reason for hiding this comment

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

you should add new params in params section: check line 108 and example you can see at line 189

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

no, should be like:
deposit_enabled: {
type: { value: Boolean, message: 'admin.currency.non_boolean_deposit_enabled' },
default: true,
desc: -> { API::V2::Admin::Entities::Currency.documentation[:deposit_enabled][:desc] }
}
for all params that you added

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -86,6 +86,38 @@
expect(result.dig(0, :code)).to eq 'eur'
end

it 'list of deposit_enabled currencies' do
Copy link
Member

Choose a reason for hiding this comment

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

add negative cases, please

Copy link
Author

Choose a reason for hiding this comment

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

done

app/api/v2/admin/entities/currency.rb Outdated Show resolved Hide resolved
spec/api/v2/admin/adjustments_spec.rb Outdated Show resolved Hide resolved
@@ -112,7 +112,12 @@ class Currencies < Grape::API
get '/currencies' do
admin_authorize! :read, Currency

search = Currency.ransack(type_eq: params[:type])
ransack_params = Helpers::RansackBuilder.new(params)
.eq(:type, :deposit_enabled, :withdrawal_enabled, :visible)
Copy link
Member

Choose a reason for hiding this comment

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

no, should be like:
deposit_enabled: {
type: { value: Boolean, message: 'admin.currency.non_boolean_deposit_enabled' },
default: true,
desc: -> { API::V2::Admin::Entities::Currency.documentation[:deposit_enabled][:desc] }
}
for all params that you added

@@ -86,6 +86,70 @@
expect(result.dig(0, :code)).to eq 'eur'
end

it 'list of deposit enabled currencies' do
Copy link
Member

Choose a reason for hiding this comment

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

after you will add params declaration you need to negative cases like if you will send params with the wrong type, check pls line 153:
it 'returns error in case of invalid type' do
api_get '/api/v2/admin/currencies', params: { type: 'invalid' }, token: token
expect(response).to have_http_status 422
end

expect(response).to be_successful

result = JSON.parse(response.body)
expect(result.size).to eq Currency.coins.select { |c| c['visible'] == false }.count
Copy link
Member

Choose a reason for hiding this comment

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

just FYI you can use Currency.coins.where(visible: false)

@mnaichuk mnaichuk added PR: QA and removed PR: Review labels Sep 4, 2020
@calj
Copy link
Member

calj commented Oct 9, 2020

please rebase

@oyershov oyershov force-pushed the feature/admin-currency-filters branch from e8723c4 to 06a44ba Compare October 16, 2020 12:11
@calj calj merged commit 5d1a59d into openware:master Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants