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

feat: allow filtering transactions count by scope #798

Merged
merged 2 commits into from
Jul 25, 2022
Merged

Conversation

sborrazas
Copy link
Contributor

refs #752

@sborrazas sborrazas requested review from thepiwo and jyeshe July 20, 2022 17:28
@sborrazas sborrazas self-assigned this Jul 20, 2022
Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

Nice the pagination already handles decreasing range. Only one usability suggestion.

do: {:ok, DbUtil.gen_to_txi(state, last_gen + 1) - DbUtil.gen_to_txi(state, first_gen)}

def count(_state, _range, _params),
do: {:error, ErrInput.Query.exception(value: "can't query by multiple values")}
Copy link
Member

Choose a reason for hiding this comment

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

It matched with unknown params like ?gen=1-100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, changing the error message


assert 101 =
conn
|> get("/txs/count", scope: "txi:#{first_txi}-#{last_txi}")
Copy link
Member

Choose a reason for hiding this comment

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

We could start using a shorthand for scopes like:
http://localhost:4000/txs/count?gen=5:40 instead of (scope=gen:5-40)

What you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather not introduce two different ways of doing the same thing, this adds complexity to the API. I also think scope filtering (either txi or gen) should be done with a single parameter, this makes it clear to the user what scoping means and makes it impossible to filter by two different things (e.g. what happens when ?gen=0-10&txi=20-30)

We should also revisit this matter once we get rid of txis from the API entirely

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good for the user to allow also the shorthand but it would be an improvement. When both params are used there would be an error handling similar to the one that there was on line 64:
do: {:error, ErrInput.Query.exception(value: "can't query by multiple values")}

Base automatically changed from test-with-store to master July 20, 2022 19:59
Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

Approving with optional shorthand improvement.

@sborrazas sborrazas merged commit cae1dc3 into master Jul 25, 2022
@sborrazas sborrazas deleted the tx-count-scope branch July 25, 2022 14:28
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.

3 participants