Conversation
27f10c6 to
30b145d
Compare
|
Size Change: +210 B (+0.01%) Total Size: 4.2 MB
|
c02659a to
0112981
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements feature flags for the search functionality using django-waffle, allowing gradual rollout of Find hybrid search and full-text search capabilities to different user groups. The implementation introduces a three-tier search system where users can be assigned different search capabilities through feature flags, with automatic fallback to simpler search methods when needed.
Changes:
- Added django-waffle dependency and integrated it into Django middleware and installed apps
- Implemented SearchType and FeatureFlag enums to define search types and feature flags
- Updated search indexer methods to accept and pass search_type parameter to the Find API
- Added _get_search_type() method in DocumentViewSet to determine search type based on user's feature flags
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/backend/pyproject.toml | Added django-waffle 5.0.0 dependency |
| src/backend/impress/settings.py | Configured WaffleMiddleware and waffle app |
| src/backend/core/enums.py | Added SearchType and FeatureFlag enums |
| src/backend/core/services/search_indexers.py | Updated search methods with search_type parameter and type hints |
| src/backend/core/api/viewsets.py | Implemented feature flag-based search type selection logic |
| src/backend/core/tests/test_services_search_indexers.py | Updated tests to use tuple instead of list for visited documents |
| src/backend/core/tests/test_services_find_document_indexer.py | Updated test to include search_type parameter |
| src/backend/core/tests/documents/test_api_documents_search_feature_flag.py | New comprehensive test for feature flag behavior |
| src/backend/core/tests/documents/test_api_documents_search.py | Added fixture to enable hybrid search flag for existing tests |
| docs/search.md | Documented feature flag usage and priority |
| CHANGELOG.md | Added entry for new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/backend/core/tests/documents/test_api_documents_search_feature_flag.py
Outdated
Show resolved
Hide resolved
| self, | ||
| q: str, | ||
| token: str, | ||
| visited: tuple[()] = (), |
There was a problem hiding this comment.
The type hint tuple[()] is incorrect syntax. This should be tuple[str, ...] to match the parent class signature in BaseDocumentIndexer and the actual return type of get_visited_document_ids_of. The current syntax is not valid Python type hint syntax for a tuple of strings.
| visited: tuple[()] = (), | |
| visited: tuple[str, ...] = (), |
| search_type: | ||
| hybrid or full-text search |
There was a problem hiding this comment.
The docstring for search_type parameter should include the type annotation (e.g., "search_type (SearchType, optional):") and be more descriptive. It should mention that it can be SearchType.HYBRID or SearchType.FULL_TEXT, and clarify what happens when it's None. Following the pattern of other parameters in this docstring would improve consistency.
| search_type: | |
| hybrid or full-text search | |
| search_type (SearchType, optional): | |
| Type of search to perform. Can be SearchType.HYBRID or SearchType.FULL_TEXT. | |
| If None, the backend search service will use its default search behavior. |
lunika
left a comment
There was a problem hiding this comment.
There is a typo in the tests, also spotted by Copilot. Otherwise everything is ok for me
0112981 to
2f8be55
Compare
36b137f to
8c48a47
Compare
5aa4ce1 to
a88264a
Compare
bf53dd3 to
84de00c
Compare
d2dbf2a to
702cd2f
Compare
9168ef3 to
521916c
Compare
944f3f5 to
64ac477
Compare
218fc25 to
5f6d884
Compare
Waffle has never been used in DOcs. it must be installed. the waffle app and middleware must also be activated in django settings. Signed-off-by: charles <charles.englebert@protonmail.com>
For beta testing purposes we need to be able to activate Find hybrid search to some users, Find full-text search to some others and leave remaining users on basic DRF title search. Signed-off-by: charles <charles.englebert@protonmail.com>
Documentation is a way to document things Signed-off-by: charles <charles.englebert@protonmail.com>
I add a separate test file for feature flags so the feature flags do not interact with the testings of other feature Signed-off-by: charles <charles.englebert@protonmail.com>
5f6d884 to
4a74177
Compare
Purpose
For beta testing purposes we need to be able to activate Find hybrid search to some users, Find full-text search to some others and leave remaining users on basic DRF title search.
Proposal
The solution proposed is based on django-waffle .
_get_search_typeinDocumentViewsetto determine which search type (title, hybrid or full-text) to use.search_typein the search query.External contributions
Thank you for your contribution! 🎉
Please ensure the following items are checked before submitting your pull request:
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)