feat: add feature to the List Users API to filter users by skill by battuAshita · Pull Request #1097 · anitab-org/mentorship-backend

battuAshita · 2021-05-10T11:02:20Z

Description

Added a new feature to filter both users and verified users based on their skill.
The 'app/api/dao/user.py' and the 'app/api/resources/user.py' files have been updated.

Fixes #759

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 included a test_dao_filter_by_skill.py file to 'tests/users'.
Both the test cases that I have written have been passed successfully.

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
Updated the Swagger UI documentation for the List Users API

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

vj-codes

@battuAshita the lint check is failing, please run black locally and push the changes
Unless lint passes the build action won't be triggered

vj-codes · 2021-05-10T12:56:26Z

@battuAshita one last file and it will pass

codecov · 2021-05-10T13:01:18Z

Codecov Report

Merging #1097 (7f158f9) into develop (8924616) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1097   +/-   ##
========================================
  Coverage    94.94%   94.95%           
========================================
  Files           38       38           
  Lines         2058     2061    +3     
========================================
+ Hits          1954     1957    +3     
  Misses         104      104

Impacted Files	Coverage Δ
app/api/dao/user.py	`100.00% <100.00%> (ø)`
app/api/resources/user.py	`90.36% <100.00%> (ø)`
app/database/models/user.py	`98.57% <100.00%> (ø)`

epicadk · 2021-05-10T14:53:22Z

+        )
+        if skill != "":
+            users_list_query = users_list_query.filter(
+                func.lower(UserModel.skills) == func.lower(skill)


This will only work when the skill exactly matches. Enable FTS and use proter tokenizer (or the postgres equivalent).

Will make the necessary changes and get back :)

Hi @epicadk Please correct me if I'm wrong. Adding FTS would require changes to made in the underlying table right? We need to have a ts_vector column which helps us compare with the given query.

Also, I have a row with skill as "critical thinking"
select * from users where to_tsvector('english', skills) @@ to_tsquery('Criti');
This query did not give me the result whereas
select * from users where skills ilike '%criti%';
This query gave me the result

I feel using something like "ILIKE" would be sufficient here.
Thoughts?

Like with a wildcard at the beginning will not use indexes.
Also porter will match Frustrate to Frustration which would be nice to have since skills isn't an enum.

A good practice is to use Explain Analyze on SQL queries. ( Only for your understanding not in production of course.)

devkapilbansal

Hi @battuAshita please make necessary changes requested below 👇

battuAshita · 2021-05-18T16:25:50Z

I have made changes to the code such that we will be able to filter based on the skill even when they are not exactly matched. Like if we search for "Creative", we will be able to get the users with skill "Creativity". Please review this change :)

battuAshita · 2021-05-29T07:31:24Z

Done :)

devkapilbansal · 2021-09-09T08:44:22Z

There are some conflicts @battuAshita .
Please solve them

battuAshita · 2021-09-09T09:16:43Z

There are some conflicts @battuAshita .
Please solve them

Okay, sure. Will get back :)

battuAshita · 2021-09-09T09:54:06Z

There are some conflicts @battuAshita .
Please solve them

Done.

devkapilbansal

Will test it soon

Some minor changes here 👇

battuAshita · 2021-09-10T17:09:20Z

Made the necessary changes. Please review them.

isabelcosta

This looks good, I just wonder about the "None" keyword added to the documentation.
Other than that, once that is addressed I am happy to approve :)

Thank you for contributing @battuAshita 🤗

isabelcosta · 2021-10-09T15:15:44Z

+        self.need_mentoring = need_mentoring
+        self.available_to_mentor = available_to_mentor


wow so this was just making these values set to false 😱 nice catch!

battuAshita · 2021-10-09T17:15:58Z

Done :)

isabelcosta

Great work! Thank you for addressing my comments @battuAshita 🙌🏾

vj-codes requested changes May 10, 2021

View reviewed changes

vj-codes added the Status: Changes Requested Changes are required to be done by the PR author. label May 10, 2021

battuAshita added 2 commits May 10, 2021 17:50

feat: add feature to the List Users API to filter users by skill

2876e7c

feat: add feature to the List Users API to filter users by skill

fd71462

battuAshita force-pushed the issue#759 branch from ffb7b17 to fd71462 Compare May 10, 2021 12:25

battuAshita added 2 commits May 10, 2021 18:23

feat: add feature to the List Users API to filter users by skill

d6df0d2

feat: add feature to the List Users API to filter users by skill

f60c559

epicadk reviewed May 10, 2021

View reviewed changes

Comment thread app/api/resources/user.py Outdated

epicadk suggested changes May 10, 2021

View reviewed changes

Comment thread app/api/resources/user.py

battuAshita added 2 commits May 11, 2021 19:16

incorporated review comments

e80cdbd

incorporated review comments

424daca

devkapilbansal added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels May 11, 2021

devkapilbansal suggested changes May 11, 2021

View reviewed changes

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels May 11, 2021

battuAshita added 4 commits May 14, 2021 01:13

made changes

458bcc0

made changes

8308492

incorporated changes

e4fbd3f

made necessary changes

c38f715

epicadk reviewed May 15, 2021

View reviewed changes

Comment thread app/api/dao/user.py Outdated

epicadk suggested changes May 18, 2021

View reviewed changes

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

removed fts functionality

3d4c9d7

battuAshita requested a review from isabelcosta as a code owner May 18, 2021 19:06

epicadk suggested changes May 29, 2021

View reviewed changes

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

removed verify-user condition

bee18f0

devkapilbansal added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jun 9, 2021

devkapilbansal requested review from epicadk and vj-codes September 9, 2021 08:44

devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Sep 9, 2021

Merge branch 'develop' into issue#759

fe906b0

devkapilbansal suggested changes Sep 9, 2021

View reviewed changes

Comment thread requirements.txt

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

Comment thread tests/users/test_dao_filter_by_skill.py Outdated

battuAshita added 4 commits September 10, 2021 22:07

Incorporated review comments

7d24370

Incorporated review comments

3333ec0

Removed lint errors

b416b38

Removed lint errors

34b440c

devkapilbansal added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Sep 11, 2021

isabelcosta requested changes Oct 9, 2021

View reviewed changes

Removed error

7f158f9

isabelcosta approved these changes Oct 18, 2021

View reviewed changes

		self.need_mentoring = need_mentoring
		self.available_to_mentor = available_to_mentor

Conversation

battuAshita commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change:

How Has This Been Tested?

Checklist:

Uh oh!

vj-codes left a comment

Choose a reason for hiding this comment

Uh oh!

vj-codes commented May 10, 2021

Uh oh!

codecov Bot commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

epicadk May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

battuAshita May 11, 2021

Choose a reason for hiding this comment

Uh oh!

battuAshita May 11, 2021

Choose a reason for hiding this comment

Uh oh!

epicadk May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devkapilbansal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

battuAshita commented May 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

battuAshita commented May 29, 2021

Uh oh!

devkapilbansal commented Sep 9, 2021

Uh oh!

battuAshita commented Sep 9, 2021

Uh oh!

battuAshita commented Sep 9, 2021

Uh oh!

devkapilbansal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

battuAshita commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isabelcosta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isabelcosta Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

battuAshita commented Oct 9, 2021

Uh oh!

isabelcosta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

battuAshita commented May 10, 2021 •

edited

Loading

codecov Bot commented May 10, 2021 •

edited

Loading

epicadk May 10, 2021 •

edited

Loading

epicadk May 11, 2021 •

edited

Loading

battuAshita commented Sep 10, 2021 •

edited

Loading