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 deposits and withdraws history to one API endpoint #2207

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Add deposits and withdraws history to one API endpoint #2207

merged 1 commit into from
Nov 12, 2019

Conversation

ymasiuk
Copy link

@ymasiuk ymasiuk commented May 9, 2019

No description provided.

mnaichuk
mnaichuk requested changes May 9, 2019
View reviewed changes
app/api/v2/account/history.rb Outdated
desc: 'Sorting order'

optional :time_from,
type: { value: Integer, message: 'account.history.non_integer_time_from' },
Copy link
Member

Choose a reason for hiding this comment

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

add
allow_blank: { value: false, message: 'account.history.empty_time_from' },

app/api/v2/account/history.rb Outdated
desc: 'An integer represents the seconds elapsed since Unix epoch.'

optional :time_to,
type: { value: Integer, message: 'account.history.non_integer_time_to' },
Copy link
Member

Choose a reason for hiding this comment

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

allow_blank: { value: false, message: 'market.trade.empty_time_to' },

app/api/v2/account/history.rb Outdated
type: { value: Integer, message: 'account.history.non_integer_time_to' },
desc: 'An integer represents the seconds elapsed since Unix epoch.'

optional :page,
Copy link
Member

Choose a reason for hiding this comment

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

rewrite :page and :limit like in https://github.com/rubykube/peatio/blob/e9ccb2abedd1a018cd2aae02bf601531fe4b0b2d/app/api/v2/market/named_params.rb#L47

app/api/v2/account/history.rb Outdated
ORDER BY updated_at #{params[:order_by].upcase}"

result = ActiveRecord::Base.connection.exec_query(sql).to_hash
.tap { |q| q.delete_if { |t| t['currency_id'] != params[:currency] } if params[:currency] }
Copy link
Member

Choose a reason for hiding this comment

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

please use 'select' or 'reject', cause delete_if change collection.
and you can avoid using tap
.select { ... }
.select { ... }
.tap { |q| present paginate(q) }

@ymasiuk ymasiuk force-pushed the feature/deposits-withdraws branch 2 times, most recently from 30c2f0f to e746305 Compare May 13, 2019 09:09
ysv
ysv suggested changes May 14, 2019
View reviewed changes
Copy link

@ysv ysv left a comment

Choose a reason for hiding this comment

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

The main idea is described here
https://gist.github.com/ysv/6c4444c4a76db1a8068be8bae4e459da

@ymasiuk ymasiuk force-pushed the feature/deposits-withdraws branch from e746305 to a990750 Compare July 16, 2019 09:05
calj
calj reviewed Jul 16, 2019
View reviewed changes
config/database.yml Outdated
@@ -10,7 +10,7 @@ default: &default

development:
<<: *default
database: peatio_development
database: peatio_production
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you want to commit that 😅

ysv
ysv suggested changes Jul 19, 2019
View reviewed changes
app/api/v2/account/history.rb Outdated

end
get "/history" do
sql = "SELECT currency_id, amount, fee, address, aasm_state, NULL AS note, txid, updated_at, type FROM deposits WHERE member_id=#{current_user.id} \
Copy link

Choose a reason for hiding this comment

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

Looks like ORDER BY is applied only to second SELECT
Could you prepare spec for this scenario

@mod
Copy link

mod commented Jul 19, 2019

SELECT FROM (SELECT UNION SELECT) order by q1.updated_at DESC

@ysv ysv closed this Aug 15, 2019
@mod mod reopened this Oct 17, 2019
@ysv ysv added PR: WIP and removed PR: Frozen labels Oct 17, 2019
@ymasiuk ymasiuk changed the base branch from master to 2-2-stable October 17, 2019 12:29
@ysv ysv added PR: Approved and removed PR: WIP labels Oct 18, 2019
@ysv
Copy link

ysv commented Oct 18, 2019 •
edited
Loading

@ymasiuk we need the same for master once QA approves this one

@pkucherenk0
Copy link

This pr is working, but I would add filter:

by type: Deposit/Withdraw
by state: accepted/succeed/processing/...
Might deprecate api account/deposits, account/withdraws eventually.

Have similar feature/bug to mentioned above api endpoints.
If some currency is visible:false we will be able to see it in the transaction history without filters, but when we will add filter by this currency we will see an error account.transactions.currency_doesnt_exist

@pkucherenk0
Copy link

pkucherenk0 commented Nov 11, 2019 •
edited
Loading

Filtering by currency returned "errors": [ "server.internal_error" ]
Selection_007
{"level":"ANY","time":"2019-11-11 15:12:46","message":"#<ActiveRecord::StatementInvalid: Mysql2::Error: Unknown column 'currencies.enabled' in 'where clause': SELECT `currencies`.`id` FROM `currencies` WHERE `currencies`.`enabled` = TRUE>"}

@mod mod merged commit 8b4942f into openware:2-2-stable Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calj calj calj left review comments

@ysv ysv ysv approved these changes

@mnaichuk mnaichuk mnaichuk approved these changes

Assignees
No one assigned
Labels
PR: Approved v2.2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ymasiuk @mod @ysv @pkucherenk0 @calj @mnaichuk