Skip to content

feat: Add to List Users API the ability to filter users by location #848

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
Closed
wants to merge 3 commits into from
Closed

feat: Add to List Users API the ability to filter users by location #848

wants to merge 3 commits into from

Conversation

ashokrayal
Copy link
Contributor

Description

Fixes #760 Add to List Users API the ability to filter users by location

Type of Change:

  • Code

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

I have updated location for few users then I tried to search from /users and /users/verified endpoints and It was giving expected results.

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@codecov
Copy link

codecov bot commented Sep 8, 2020 •
edited
Loading

Codecov Report

Merging #848 (62d68cc) into develop (1643e7e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #848   +/-   ##
========================================
  Coverage    95.99%   96.00%           
========================================
  Files           96       96           
  Lines         5293     5306   +13     
========================================
+ Hits          5081     5094   +13     
  Misses         212      212
Impacted Files Coverage Δ
app/api/dao/user.py 85.08% <100.00%> (ø)
app/api/resources/user.py 89.04% <100.00%> (+0.10%) ⬆️
tests/test_data.py 100.00% <100.00%> (ø)
tests/users/test_api_list_users.py 99.30% <100.00%> (+0.05%) ⬆️

@ashokrayal
Copy link
Contributor Author

Hi @paritoshsinghrahar,

Can you please review my PR?

Thanks

@vj-codes vj-codes mentioned this pull request Sep 15, 2020
Open
5 tasks
@paritoshsinghrahar
Copy link

Hi @paritoshsinghrahar,

Can you please review my PR?

Thanks

Sure. I'll get back to you.

@paritoshsinghrahar
Copy link

@ashokrayal On an initial review looks good. Great Effort, though.

@ashokrayal ashokrayal changed the title Issue 760 feat: Add to List Users API the ability to filter users by location Sep 24, 2020
@ashokrayal
Copy link
Contributor Author

Hi @paritoshsinghrahar @isabelcosta, @vj-codes,
Can someone review this PR?

@paritoshsinghrahar paritoshsinghrahar requested review from isabelcosta and SanketDG and removed request for isabelcosta September 29, 2020 02:45
paritoshsinghrahar
paritoshsinghrahar previously approved these changes Sep 29, 2020
View reviewed changes
Copy link

@paritoshsinghrahar paritoshsinghrahar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed it again. Seems legit and great. LGTM!!
@isabelcosta, please review.

@paritoshsinghrahar paritoshsinghrahar added the Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. label Sep 29, 2020
SanketDG
SanketDG previously approved these changes Sep 29, 2020
View reviewed changes
@SanketDG
Copy link
Contributor

@isabelcosta This looks good,

But before merging any of the filter PRs, I'd be willing to have a discussion on what the best approach is to implement this or refactor this later. Currently, the function and the parameters suffers from all kinds of code smell, so I am open to all kinds of ideas on how to implement filtering in a clean way.

@pbgurav pbgurav mentioned this pull request Oct 19, 2020
Open
@rpattath rpattath added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. Help Wanted Extra attention is needed. and removed Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. labels Oct 21, 2020
@rpattath
Copy link
Member

@isabelcosta @SanketDG does this PR need further discussion before being tested and merged?

@rpattath rpattath removed the Help Wanted Extra attention is needed. label Oct 27, 2020
@isabelcosta
Copy link
Member

@SanketDG I agree with you. Will wait for the test and then merge.
But as part of this, can you create an issue for creating a cleaner way for filtering data? If not, I can, just let me know :)
Thank you so much for bringing this up!

devkapilbansal
devkapilbansal suggested changes Nov 15, 2020
View reviewed changes
Copy link
Member

@devkapilbansal devkapilbansal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ashokrayal can you please solve the merge conflicts

@ashokrayal
Copy link
Contributor Author

I will surely do it today

@SanketDG
Copy link
Contributor

Here are some basic ideas to abstract away filtering into a common piece of code (so that it doesn't end up having 20 arguments in the function signature)

  • Use **kwargs as a container for all filter operations.
  • Loop through kwargs and check which "keys" are fields of UserModel.
  • Do the standard filter query using these key value pairs from **kwargs

@rpattath rpattath removed the Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. label Nov 24, 2020
@rpattath rpattath added the Status: Needs Review PR needs an additional review or a maintainer's review. label Nov 24, 2020
@vj-codes
Copy link
Member

Putting on hold as the best approach is not decided yet

@vj-codes vj-codes added Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Feb 21, 2021
@isabelcosta
Copy link
Member

@vj-codes could you create, in case it does not exist yet, an issue to implement filtering in a generic way.

vj-codes
vj-codes reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@vj-codes vj-codes left a comment

Choose a reason for hiding this comment

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

Closing due to inactivity, thank you for your contribution!
Follow up issue will be made.

@vj-codes vj-codes closed this Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vj-codes vj-codes vj-codes left review comments

@devkapilbansal devkapilbansal devkapilbansal requested changes

@SanketDG SanketDG SanketDG left review comments

@paritoshsinghrahar paritoshsinghrahar paritoshsinghrahar left review comments

Assignees
No one assigned
Labels
Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add to List Users API the ability to filter users by location
7 participants
@ashokrayal @paritoshsinghrahar @SanketDG @rpattath @isabelcosta @vj-codes @devkapilbansal