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

Fix rank parameter of delegates API endpoint #1988

Closed
Tschakki opened this issue May 2, 2018 · 11 comments

Comments

Projects
6 participants
@Tschakki
Copy link
Member

commented May 2, 2018

Expected behavior

By making an API request to the delegates endpoint, with provided rank param, one should get a response that contains information about the delegate with the specified rank.
Also, I think it should accept numbers, not strings as values and could therefore be refactored.

Actual behavior

The response contains all delegates.
The param expects a string.

Steps to reproduce

Make API call
/api/delegates?rank="5"

Which version(s) does this affect? (Environment, OS, etc...)

1.0.0

@4miners

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@Tschakki Any practical use cases for that endpoint?

@mauliksoneji

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

Hi @Tschakki and @4miners, If there need to be changes made, can you please assign this task to me? I can look into this.

@Tschakki

This comment has been minimized.

Copy link
Member Author

commented May 3, 2018

@4miners ok alternative is just to get rid of that rank param...
One can query the delegates sorted by rank, what is enough for use cases like displaying rankings imho.
But I don't have a strong opinion in wether fixing or removing it.

@MaciejBaj

This comment has been minimized.

Copy link
Member

commented May 9, 2018

@Mastermaulik are you still willing to do the task?

@mauliksoneji

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

Yes, I can work on this issue @MaciejBaj.

As far as I understand, currently there is no rank parameter that we take in the /api/delegates endpoint. We need to change this endpoint to take in rank parameter and return the delegates having the same rank parameter.

The type of rank field is number, so it would look something like: /api/delegates?rank=5.
Is this what is the expected behaviour?

Like @Tschakki said, one can also query api/delegates?orderBy=rank:asc
To get the delegates in the increasing order of rank.

@Tschakki

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

Thx for looking into it @Mastermaulik !

I think the most convenient will be to simply remove it, as this endpoint is not really needed... The same can be achieved with the alternative request api/delegates?orderBy=rank:asc, as you mentioned.

This is just a recommendation, if you want to fix it or already started, go ahead :)

@diego-G

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@Tschakki after reading all your comments, IMHO I think this parameter should be kept. I don't see how it can bother anyone. The use case will arrive with the time.
By itself, I see much more straightforward to request rank=101 than offset=100&limit=1&sort=rank:asc. That's quite clear.
@Mastermaulik if you don't mind, I gotta close the current PR. Would you be eager to fix the endpoint as it was intended at the beginning?

@MaciejBaj MaciejBaj added this to New Issues in Sprint Board 30-05-18 via automation May 31, 2018

@MaciejBaj MaciejBaj removed this from New Issues in Sprint Board 21-05-18 May 31, 2018

@MaciejBaj MaciejBaj moved this from New Issues to Previous Issues in Sprint Board 30-05-18 May 31, 2018

@mauliksoneji

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

Hi @diego-G , I have had an eye surgery recently and am advised to take rest till monday, Please give me time till monday, I will work on the issue then.

@MaciejBaj

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

@nazarhussain please suggest the proper solution.

@MaciejBaj MaciejBaj assigned Tschakki and unassigned diego-G Jun 6, 2018

@nazarhussain

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

@Tschakki I reviewed the discussion as well the relevant code. Our db layer is written in a way to to accept filters fo only persisted database column. While rank is a computed column, so we have to refactor a lot of stuff to make that filter work.

@diego-G Providing composite filter is what we should do, but in current scope. Currently its get more complicated. e.g. If some one request rank=23&offset=3, so logically its an invalid request, as both params can be used together. So we have to an extensive request validation layer to send proper error messages.

For now its seems feasible to just remove the rank filter. Use can still get a specific rank account by providing multiple filter options e.g. offset=100&limit=1&sort=rank:asc. @MaciejBaj

@Tschakki

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

@nazarhussain thanks for clarification!
I reopened @Mastermaulik 's PR, looks good from my side :)

@MaciejBaj MaciejBaj closed this in 6063030 Jun 7, 2018

Version 1.0.0 automation moved this from Open Issues to Closed Issues Jun 7, 2018

Sprint Board 30-05-18 automation moved this from Previous Issues to Closed Issues Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.