baseapp-profiles: change profile filter to add name and urlpath query - BA-1737 #178
Conversation
pt-tsl
requested review from
nossila,
vitorguima,
anicioalexandre and
Ronan-Fernandes
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
2 times, most recently
from
1713a96 to
be6c2bb
Compare
| ("created", "created"), | ||
| ("followers_count__total", "followers_count_total"), | ||
| ("following_count__total", "following_count_total"), | ||
| ("is_followed_by_me", "is_followed_by_me"), |
There was a problem hiding this comment.
You can't just sort by is_followed_by_me its not a field in the model and it is not being annotated in the queryset, so lets remove it for now.
nossila
changed the title
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
be6c2bb to
13ce906
Compare
WalkthroughThe changes introduce a new filtering mechanism for the Changes
Poem
Warning Rate limit exceeded@pt-tsl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between 143b6c80d3bdad261910d6b1cfcc05464d8d6f52 and a2f62ef. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
baseapp-profiles/baseapp_profiles/graphql/filters.py (2)
8-10: Add docstring to describe the filter's purposeThe filter setup looks good, but adding a docstring would help other developers understand its purpose and usage.
class ProfileFilter(django_filters.FilterSet): + """Filter for Profile model supporting name and URL path search via 'q' parameter.""" q = django_filters.CharFilter(method="filter_q")
23-24: Consider indexing for query performanceThe implementation correctly handles the name and URL path filtering. However, the OR condition with
url_paths__pathlookup might affect performance on large datasets.Consider adding database indexes to improve query performance:
class URLPath(models.Model): path = models.CharField(max_length=255, db_index=True)Also ensure the
namefield on the Profile model is indexed:class Profile(models.Model): name = models.CharField(max_length=255, db_index=True)baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)
Line range hint
89-124: Good architectural approach maintaining securityThe integration of the new filter maintains the existing security model:
- Permission checks in
get_noderemain intact- Query visibility filtering in
get_querysetis preserved- Member access control in
resolve_membersis unchangedThis ensures that the new filtering capabilities work within the established security boundaries.
Consider documenting the filter's interaction with permissions in the class docstring for future maintainers.
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (1)
147-170: Consider enhancing test coverage with additional casesWhile the current test is good, consider adding:
- A docstring explaining the test's purpose
- Test data constants instead of hardcoded values
- Additional test cases for:
- Empty search string
- Special characters in search
- Case sensitivity
- Result ordering
Example enhancement:
+SEARCH_QUERY = "da" + def test_search_profiles(graphql_user_client): + """ + Test profile search functionality: + - Verifies profiles are found by name containing search string + - Verifies profiles are found by URL path containing search string + - Verifies non-matching profiles are excluded from results + """ profile1 = ProfileFactory(name="David")Would you like me to provide a complete implementation of the additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 9457faf and 13ce90617a20be7c698ed5d6100ec299435cd2f3.
📒 Files selected for processing (4)
baseapp-profiles/baseapp_profiles/graphql/filters.py(1 hunks)baseapp-profiles/baseapp_profiles/graphql/object_types.py(1 hunks)baseapp-profiles/baseapp_profiles/tests/test_get_queries.py(3 hunks)baseapp-profiles/setup.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- baseapp-profiles/setup.cfg
🔇 Additional comments (9)
baseapp-profiles/baseapp_profiles/graphql/filters.py (3)
1-5: LGTM! Well-structured imports and model loading
The imports are appropriate and the use of swapper for model loading provides good flexibility for model customization.
19-21: LGTM! Clean Meta configuration
The Meta configuration is appropriately minimal and matches the defined filters.
11-17: Verify queryset annotations for count fields
The ordering configuration looks correct, but we should verify that followers_count__total and following_count__total are properly annotated in the queryset.
baseapp-profiles/baseapp_profiles/graphql/object_types.py (3)
15-15: LGTM: Clean import addition
The import statement is properly placed and follows the project's import organization pattern.
15-15: Verify complete feature implementation
While the integration of ProfileFilter is correct, we should verify that the implementation in filters.py properly supports the PR's objective of querying both name and urlPath simultaneously.
#!/bin/bash
# Check the filter implementation
rg -A 10 'class ProfileFilter' --type python
# Look for related test coverage
rg -A 5 'test.*profile.*filter' --type pythonAlso applies to: 89-93
Line range hint 89-93: Verify filter's impact on existing queries
The integration of ProfileFilter through filterset_class looks correct. However, since this is a base class used by ProfileObjectType, we should verify that:
- Existing queries continue to work as expected
- The new filtering capabilities don't bypass any permission checks
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (3)
5-6: LGTM: Imports are appropriate and well-organized
The new imports are necessary for the URL path functionality being tested and follow proper import conventions.
27-37: LGTM: Well-structured GraphQL query
The query is well-defined with appropriate naming and structure, returning the minimal necessary fields for testing.
147-170: LGTM: Test implementation is functional and covers basic cases
The test properly validates the profile search functionality with both positive and negative cases.
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
13ce906 to
30aba02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (4)
27-37: Consider expanding the returned fields in the test queryWhile the current query works for basic testing, consider returning additional profile fields (like
name) to ensure the filtering works correctly with the actual data being matched. This would make the tests more robust and closer to real-world usage.SEARCH_PROFILES_BY_QUERY_PARAM = """ query AllProfiles($q: String!) { allProfiles(q: $q) { edges { node { id + name } } } } """
153-159: Simplify URL path setup and reduce database callsThe URL path setup has multiple unnecessary database refreshes and could be simplified.
- urlPath1 = URLPathFactory(path="danger.john", is_active=True, language=None) - profile_content_type = ContentType.objects.get_for_model(Profile) - urlPath1.target_content_type = profile_content_type - urlPath1.target_object_id = profile4.id - urlPath1.save() - urlPath1.refresh_from_db() - profile4.refresh_from_db() + urlPath1 = URLPathFactory( + path="danger.john", + is_active=True, + language=None, + target_content_type=ContentType.objects.get_for_model(Profile), + target_object_id=profile4.id + )
163-165: Simplify list comprehensionThe nested list comprehension can be simplified for better readability.
- profiles = [ - id for id in [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]] - ] + profiles = [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]]
147-170: Add test documentation and edge casesThe test function would benefit from:
- A docstring explaining the test purpose and the "da" matching logic
- Additional assertions to verify why profile4 (John) matches via its URL path
- Edge cases for special characters and case sensitivity
def test_search_profiles(graphql_user_client): + """ + Test profile search functionality: + - Matches profiles with "da" in their name (David, Daniel) + - Matches profiles with "da" in their URL path (danger.john) + - Verifies non-matching profiles are excluded + """ profile1 = ProfileFactory(name="David") profile2 = ProfileFactory(name="Daniel") profile3 = ProfileFactory(name="Mark") profile4 = ProfileFactory(name="John") profile5 = ProfileFactory(name="Donald") urlPath1 = URLPathFactory( path="danger.john", is_active=True, language=None, target_content_type=ContentType.objects.get_for_model(Profile), target_object_id=profile4.id ) # Test case-sensitive search + response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": "DA"}) + content = response.json() + profiles_upper = [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]] + assert len(profiles_upper) == 0 # Verify case sensitivity # Test substring search response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": "da"}) content = response.json() profiles = [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]] # Verify name matches assert profile1.relay_id in profiles # David assert profile2.relay_id in profiles # Daniel assert profile3.relay_id not in profiles # Mark + + # Verify URL path matches assert profile4.relay_id in profiles # John (matches via danger.john URL) + assert urlPath1.path == "danger.john" # Explicit verification of URL match + assert profile5.relay_id not in profiles # Donald
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 13ce90617a20be7c698ed5d6100ec299435cd2f3 and 30aba0295a6e9cf1defbdd3a8e96fac2ab5dd9e4.
📒 Files selected for processing (4)
baseapp-profiles/baseapp_profiles/graphql/filters.py(1 hunks)baseapp-profiles/baseapp_profiles/graphql/object_types.py(1 hunks)baseapp-profiles/baseapp_profiles/tests/test_get_queries.py(3 hunks)baseapp-profiles/setup.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- baseapp-profiles/baseapp_profiles/graphql/filters.py
- baseapp-profiles/baseapp_profiles/graphql/object_types.py
- baseapp-profiles/setup.cfg
🔇 Additional comments (1)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (1)
5-6: LGTM: Imports are appropriate for the new functionality
The new imports are necessary and correctly used for the URL path testing functionality.
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
30aba02 to
143b6c8
Compare
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
143b6c8 to
a2f62ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (1)
27-37: Consider expanding the returned fields for better reusabilityWhile the current implementation returns only the ID field, consider adding commonly needed fields (e.g., name, urlPath) to avoid additional queries in the future.
SEARCH_PROFILES_BY_QUERY_PARAM = """ query AllProfiles($q: String!) { allProfiles(q: $q) { edges { node { id + name + urlPath } } } } """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 143b6c80d3bdad261910d6b1cfcc05464d8d6f52 and a2f62ef.
📒 Files selected for processing (4)
baseapp-profiles/baseapp_profiles/graphql/filters.py(1 hunks)baseapp-profiles/baseapp_profiles/graphql/object_types.py(1 hunks)baseapp-profiles/baseapp_profiles/tests/test_get_queries.py(3 hunks)baseapp-profiles/setup.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- baseapp-profiles/baseapp_profiles/graphql/filters.py
- baseapp-profiles/baseapp_profiles/graphql/object_types.py
- baseapp-profiles/setup.cfg
🔇 Additional comments (1)
baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (1)
3-4: LGTM: Imports are appropriate and well-organized
The new imports are necessary for the URL path functionality and content type handling in the test.
| @@ -128,3 +142,29 @@ def test_another_user_cant_view_members(graphql_user_client): | |||
| content = response.json() | |||
|
|
|||
| assert content["data"]["profile"]["members"] | |||
There was a problem hiding this comment.
Critical: Existing test has incorrect assertion
The test test_another_user_cant_view_members asserts that another user CAN view members (assert content["data"]["profile"]["members"]), which contradicts the test name and likely the intended behavior. This should be fixed while we're updating the test file.
- assert content["data"]["profile"]["members"]
+ assert not content["data"]["profile"]["members"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert content["data"]["profile"]["members"] | |
| assert not content["data"]["profile"]["members"] |
| def test_search_profiles(graphql_user_client): | ||
| profile1 = ProfileFactory(name="David") | ||
| profile2 = ProfileFactory(name="Daniel") | ||
| profile3 = ProfileFactory(name="Mark") | ||
| profile4 = ProfileFactory(name="John") | ||
| profile5 = ProfileFactory(name="Donald") | ||
| urlPath1 = URLPathFactory(path="danger.john", is_active=True, language=None) | ||
| profile_content_type = ContentType.objects.get_for_model(Profile) | ||
| urlPath1.target_content_type = profile_content_type | ||
| urlPath1.target_object_id = profile4.id | ||
| urlPath1.save() | ||
| urlPath1.refresh_from_db() | ||
| profile4.refresh_from_db() | ||
|
|
||
| response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": "da"}) | ||
| content = response.json() | ||
| profiles = [ | ||
| id for id in [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]] | ||
| ] | ||
| assert profile1.relay_id in profiles | ||
| assert profile2.relay_id in profiles | ||
| assert profile3.relay_id not in profiles | ||
| assert profile4.relay_id in profiles | ||
| assert profile5.relay_id not in profiles |
There was a problem hiding this comment.
Critical: Test coverage and assertions need improvement
Several issues need to be addressed:
- The test only implicitly covers name search but doesn't explicitly test URL path search
- The assertion for profile4 (John) seems incorrect as it doesn't contain "da" but is expected in results
- Unnecessary database refreshes can be removed
Here's a suggested improvement:
def test_search_profiles(graphql_user_client):
- profile1 = ProfileFactory(name="David")
- profile2 = ProfileFactory(name="Daniel")
- profile3 = ProfileFactory(name="Mark")
- profile4 = ProfileFactory(name="John")
- profile5 = ProfileFactory(name="Donald")
- urlPath1 = URLPathFactory(path="danger.john", is_active=True, language=None)
- profile_content_type = ContentType.objects.get_for_model(Profile)
- urlPath1.target_content_type = profile_content_type
- urlPath1.target_object_id = profile4.id
- urlPath1.save()
- urlPath1.refresh_from_db()
- profile4.refresh_from_db()
+ # Test profiles for name search
+ profile_david = ProfileFactory(name="David")
+ profile_daniel = ProfileFactory(name="Daniel")
+ profile_mark = ProfileFactory(name="Mark")
+
+ # Test profile for URL path search
+ profile_with_path = ProfileFactory(name="John")
+ URLPathFactory(
+ path="dashboard/john",
+ is_active=True,
+ target_content_type=ContentType.objects.get_for_model(Profile),
+ target_object_id=profile_with_path.id
+ )
- response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": "da"})
- content = response.json()
- profiles = [
- id for id in [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]]
- ]
- assert profile1.relay_id in profiles
- assert profile2.relay_id in profiles
- assert profile3.relay_id not in profiles
- assert profile4.relay_id in profiles
- assert profile5.relay_id not in profiles
+ def get_profile_ids(query):
+ response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": query})
+ return [edge["node"]["id"] for edge in response.json()["data"]["allProfiles"]["edges"]]
+
+ # Test name search
+ name_results = get_profile_ids("da")
+ assert profile_david.relay_id in name_results
+ assert profile_daniel.relay_id in name_results
+ assert profile_mark.relay_id not in name_results
+ assert profile_with_path.relay_id not in name_results
+
+ # Test URL path search
+ path_results = get_profile_ids("dashboard")
+ assert profile_with_path.relay_id in path_results
+ assert profile_david.relay_id not in path_results📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_search_profiles(graphql_user_client): | |
| profile1 = ProfileFactory(name="David") | |
| profile2 = ProfileFactory(name="Daniel") | |
| profile3 = ProfileFactory(name="Mark") | |
| profile4 = ProfileFactory(name="John") | |
| profile5 = ProfileFactory(name="Donald") | |
| urlPath1 = URLPathFactory(path="danger.john", is_active=True, language=None) | |
| profile_content_type = ContentType.objects.get_for_model(Profile) | |
| urlPath1.target_content_type = profile_content_type | |
| urlPath1.target_object_id = profile4.id | |
| urlPath1.save() | |
| urlPath1.refresh_from_db() | |
| profile4.refresh_from_db() | |
| response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": "da"}) | |
| content = response.json() | |
| profiles = [ | |
| id for id in [edge["node"]["id"] for edge in content["data"]["allProfiles"]["edges"]] | |
| ] | |
| assert profile1.relay_id in profiles | |
| assert profile2.relay_id in profiles | |
| assert profile3.relay_id not in profiles | |
| assert profile4.relay_id in profiles | |
| assert profile5.relay_id not in profiles | |
| def test_search_profiles(graphql_user_client): | |
| # Test profiles for name search | |
| profile_david = ProfileFactory(name="David") | |
| profile_daniel = ProfileFactory(name="Daniel") | |
| profile_mark = ProfileFactory(name="Mark") | |
| # Test profile for URL path search | |
| profile_with_path = ProfileFactory(name="John") | |
| URLPathFactory( | |
| path="dashboard/john", | |
| is_active=True, | |
| target_content_type=ContentType.objects.get_for_model(Profile), | |
| target_object_id=profile_with_path.id | |
| ) | |
| def get_profile_ids(query): | |
| response = graphql_user_client(SEARCH_PROFILES_BY_QUERY_PARAM, variables={"q": query}) | |
| return [edge["node"]["id"] for edge in response.json()["data"]["allProfiles"]["edges"]] | |
| # Test name search | |
| name_results = get_profile_ids("da") | |
| assert profile_david.relay_id in name_results | |
| assert profile_daniel.relay_id in name_results | |
| assert profile_mark.relay_id not in name_results | |
| assert profile_with_path.relay_id not in name_results | |
| # Test URL path search | |
| path_results = get_profile_ids("dashboard") | |
| assert profile_with_path.relay_id in path_results | |
| assert profile_david.relay_id not in path_results |
|
Modify
ProfileFilterto allow queryingnameandurlPathat the same timehttps://silverlogic.atlassian.net/browse/BA-1572
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores