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: add feature to the List Users API to filter users by skill #1097

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions app/api/dao/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ def get_user_by_username(username: str):
def list_users(
user_id: int,
search_query: str = "",
skill: str = "",
page: int = DEFAULT_PAGE,
per_page: int = DEFAULT_USERS_PER_PAGE,
is_verified=None,
Expand All @@ -159,6 +160,7 @@ def list_users(
Arguments:
user_id: The ID of the user to be listed.
search_query: The search query for name of the users to be found.
skill: The skill of the user to be listed.
is_verified: Status of the user's verification; None when provided as an argument.
page: The page of users to be returned
per_page: The number of users to return per page
Expand All @@ -168,14 +170,18 @@ def list_users(

"""

users_list = (
UserModel.query.filter(
UserModel.id != user_id,
not is_verified or UserModel.is_email_verified,
func.lower(UserModel.name).contains(search_query.lower())
| func.lower(UserModel.username).contains(search_query.lower()),
users_list_query = UserModel.query.filter(
UserModel.id != user_id,
not is_verified or UserModel.is_email_verified,
func.lower(UserModel.name).contains(search_query.lower())
| func.lower(UserModel.username).contains(search_query.lower()),
)
if skill:
users_list_query = users_list_query.filter(
func.lower(UserModel.skills) == func.lower(skill)
Copy link
Member

@epicadk epicadk May 10, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make the necessary changes and get back :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@epicadk epicadk May 11, 2021

Choose a reason for hiding this comment

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

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.)

)
.order_by(UserModel.id)
users_list = (
users_list_query.order_by(UserModel.id)
.paginate(
page=page,
per_page=per_page,
Expand Down
17 changes: 15 additions & 2 deletions app/api/resources/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class UserList(Resource):
"list_users",
params={
"search": "Search query",
"skills": "Search based on skill",
"page": "specify page of users (default: 1)",
"per_page": "specify number of users per page (default: 10)",
},
Expand Down Expand Up @@ -93,7 +94,13 @@ def get(cls):
)

user_id = get_jwt_identity()
return DAO.list_users(user_id, request.args.get("search", ""), page, per_page)
return DAO.list_users(
user_id,
request.args.get("search", ""),
request.args.get("skills", None),
page,
per_page,
)


@users_ns.route("users/<int:user_id>")
Expand Down Expand Up @@ -260,6 +267,7 @@ class VerifiedUser(Resource):
"get_verified_users",
params={
"search": "Search query",
"skills": "Search based on skill",
"page": "specify page of users",
"per_page": "specify number of users per page",
},
Expand Down Expand Up @@ -298,7 +306,12 @@ def get(cls):

user_id = get_jwt_identity()
return DAO.list_users(
user_id, request.args.get("search", ""), page, per_page, is_verified=True
user_id,
request.args.get("search", ""),
battuAshita marked this conversation as resolved.
Show resolved Hide resolved
request.args.get("skills", None),
page,
per_page,
is_verified=True,
)


Expand Down
15 changes: 12 additions & 3 deletions app/database/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,16 @@ class UserModel(db.Model):
need_mentoring = db.Column(db.Boolean)
available_to_mentor = db.Column(db.Boolean)

def __init__(self, name, username, password, email, terms_and_conditions_checked):
def __init__(
self,
name,
username,
password,
email,
terms_and_conditions_checked,
need_mentoring=False,
available_to_mentor=False,
):
"""Initialises userModel class with name, username, password, email, and terms_and_conditions_checked."""
# required fields

Expand All @@ -78,8 +87,8 @@ def __init__(self, name, username, password, email, terms_and_conditions_checked

# optional fields

self.need_mentoring = False
self.available_to_mentor = False
self.need_mentoring = need_mentoring
self.available_to_mentor = available_to_mentor
Comment on lines +90 to +91
Copy link
Member

Choose a reason for hiding this comment

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

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


def json(self):
"""Returns Usermodel object in json format."""
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ gunicorn==20.0.4
psycopg2-binary==2.8.6
python-dotenv==0.18.0
six==1.11.0
pre-commit
battuAshita marked this conversation as resolved.
Show resolved Hide resolved
pre-commit
74 changes: 74 additions & 0 deletions tests/users/test_dao_filter_by_skill.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import unittest

from flask import json
from http import HTTPStatus
from tests.base_test_case import BaseTestCase
from app.database.models.user import UserModel
from app.database.sqlalchemy_extension import db
from tests.test_data import user1, user2, user3
from tests.test_utils import get_test_request_header


class TestFilterUsersBySkill(BaseTestCase):
def setUp(self):
super().setUp()

# Insert data of the first entry
self.first_user = UserModel(**user1)
self.first_user.is_email_verified = True
self.first_user.skills = "Problem Solving"

db.session.add(self.first_user)
db.session.commit()

# Insert data of the second entry
self.second_user = UserModel(**user2)
self.second_user.is_email_verified = True
self.second_user.skills = "Problem Solving"

db.session.add(self.second_user)
db.session.commit()

# Insert data of the third entry
self.third_user = UserModel(**user3)
self.third_user.is_email_verified = True
self.third_user.skills = "Critical thinking"

db.session.add(self.third_user)
db.session.commit()

def test_filter_users_by_skill_problem_solving(self):

auth_header = get_test_request_header(self.admin_user.id)
expected_response = "Problem Solving"

actual_response = self.client.get(
"/users/verified?skills=Problem Solving",
headers=auth_header,
content_type="application/json",
)

self.assertEqual(HTTPStatus.OK, actual_response.status_code)

for data in json.loads(actual_response.data):
self.assertEqual(expected_response, data["skills"])

def test_filter_users_by_skill_critical(self):

auth_header = get_test_request_header(self.admin_user.id)
expected_response = "Critical thinking"

actual_response = self.client.get(
"/users/verified?skills=Critical thinking",
headers=auth_header,
content_type="application/json",
)

self.assertEqual(HTTPStatus.OK, actual_response.status_code)

for data in json.loads(actual_response.data):
self.assertEqual(expected_response, data["skills"])


if __name__ == "__main__":
unittest.main()