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

NUM-384 filters account by their balance #217

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

jdupas22
Copy link
Contributor

@jdupas22 jdupas22 commented Jun 7, 2022

This feature allows us to filters the accounts we get using the /{ledger}/accounts route by their balance

The request now takes two more optionnal parameters, and <balance_operator>

  • balance : int64 that represents the balance volume of the account
  • balance_operator : comparison operator, can be gt, gte, e, lt, lte

I also tried to make the tests easier to read by bracketing them

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #217 (a3e0e05) into main (b11fa82) will increase coverage by 0.45%.
The diff coverage is 93.84%.

@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   67.16%   67.62%   +0.45%     
==========================================
  Files          72       72              
  Lines        3816     3870      +54     
==========================================
+ Hits         2563     2617      +54     
  Misses       1001     1001              
  Partials      252      252              
Impacted Files Coverage Δ
pkg/storage/sqlstorage/transactions.go 82.03% <84.61%> (ø)
pkg/api/controllers/transaction_controller.go 65.03% <90.74%> (ø)
pkg/storage/sqlstorage/accounts.go 84.40% <91.48%> (+3.94%) ⬆️
pkg/api/controllers/account_controller.go 75.55% <92.00%> (+5.69%) ⬆️
pkg/api/controllers/config_controller.go 45.71% <100.00%> (ø)
pkg/ledger/ledger.go 76.96% <100.00%> (ø)
pkg/ledger/stats.go 66.66% <100.00%> (ø)
pkg/opentelemetry/opentelemetrytraces/storage.go 49.65% <100.00%> (ø)
pkg/storage/accounts.go 100.00% <100.00%> (ø)
pkg/storage/sqlstorage/aggregations.go 83.82% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce23843...a3e0e05. Read the comment docs.

@jdupas22 jdupas22 force-pushed the num-384-filter-accounts-by-balance branch from f805184 to e7f5dd9 Compare June 7, 2022 15:22
@jdupas22 jdupas22 marked this pull request as ready for review June 7, 2022 15:40
@jdupas22 jdupas22 requested a review from a team as a code owner June 7, 2022 15:40
@jdupas22 jdupas22 requested review from louhde, gfyrag and antoinegelloz and removed request for a team June 7, 2022 15:40
pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved
pkg/storage/sqlstorage/accounts.go Outdated Show resolved Hide resolved
pkg/storage/sqlstorage/accounts.go Outdated Show resolved Hide resolved
@jdupas22 jdupas22 force-pushed the num-384-filter-accounts-by-balance branch from e26a3c3 to 0da6ddf Compare June 9, 2022 07:16
Copy link
Contributor

@antoinegelloz antoinegelloz left a comment

Choose a reason for hiding this comment

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

What do you think about also adding unit tests in the sqlstorage package for this new feature?

pkg/api/controllers/account_controller.go Outdated Show resolved Hide resolved
pkg/api/controllers/account_controller.go Outdated Show resolved Hide resolved
pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved
pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved
pkg/storage/sqlstorage/accounts.go Show resolved Hide resolved
@jdupas22
Copy link
Contributor Author

jdupas22 commented Jun 9, 2022

@antoinegelloz @gfyrag maybe we try to merge #216 before this one ?

@antoinegelloz
Copy link
Contributor

@antoinegelloz @gfyrag maybe we try to merge #216 before this one ?

Yes we could :) Did you have a look at it already?

@flemzord
Copy link
Member

flemzord commented Jun 9, 2022

@all-contributors please add @jdupas22 for code

@allcontributors
Copy link
Contributor

@flemzord

I've put up a pull request to add @jdupas22! 🎉

@jdupas22 jdupas22 requested a review from gfyrag June 10, 2022 07:29
gfyrag
gfyrag previously approved these changes Jun 14, 2022
pkg/api/controllers/account_controller.go Outdated Show resolved Hide resolved
pkg/storage/sqlstorage/accounts_test.go Outdated Show resolved Hide resolved
pkg/storage/utils.go Outdated Show resolved Hide resolved
pkg/storage/utils.go Outdated Show resolved Hide resolved
pkg/storage/utils.go Outdated Show resolved Hide resolved
pkg/api/controllers/script_controller_test.go Outdated Show resolved Hide resolved
pkg/api/controllers/transaction_controller.go Outdated Show resolved Hide resolved
@jdupas22 jdupas22 force-pushed the num-384-filter-accounts-by-balance branch 4 times, most recently from 010cf49 to e0235ee Compare June 15, 2022 14:05
gfyrag
gfyrag previously approved these changes Jun 15, 2022
pkg/api/controllers/script_controller_test.go Outdated Show resolved Hide resolved
pkg/storage/transactions.go Outdated Show resolved Hide resolved
pkg/storage/transactions.go Outdated Show resolved Hide resolved
pkg/storage/accounts.go Outdated Show resolved Hide resolved
pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved
feat(api_test): add unit tests for balance filtering in get accounts controller

refactor(accounts_balance_filter): convert balance into integer during db request for sqlite, and replace balance_operator if workflow by a switch case

feat(accounts_balance_filter): add parameter validation in controller, panics if parameters are bad in storage, and few fixes in the tests

feat(accounts_balance_filter): fix missing return after responseError in controller, and various test improvements

refactor(accounts_controller): rename constants in camel case to be more standard

refactor(query): dissolve query package into storage, and better handling of enum BalanceOperator
antoinegelloz
antoinegelloz previously approved these changes Jun 16, 2022
pkg/api/controllers/account_controller_test.go Outdated Show resolved Hide resolved
pkg/storage/accounts.go Outdated Show resolved Hide resolved
pkg/api/controllers/account_controller.go Outdated Show resolved Hide resolved
@antoinegelloz antoinegelloz merged commit 85afc37 into main Jun 16, 2022
@antoinegelloz antoinegelloz deleted the num-384-filter-accounts-by-balance branch June 16, 2022 19:11
@gfyrag gfyrag added this to the 1.6.0 milestone Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants