Fix Migration Conflicts and Re-implement Test Mode - #137
Conversation
- Add test_mode (Boolean) and test_revision_ids (JSON) fields to WikiConfiguration model - Update wiki_client.py to use test revision IDs when test mode is enabled - Add API endpoints to save and retrieve test mode configuration - Generate new migration
3e86ce9 to
7ef0432
Compare
ademolaomosanya
added
enhancement
|
Hi @zache-fi, please any thoughts on this so far? |
|
Branch is updated to "main" |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a test mode feature to the wiki reviews system, allowing users to specify specific revision IDs for testing instead of using live flaggedpages data. The feature is controlled through the WikiConfiguration model and affects how pending pages are fetched.
Key changes:
- Added
test_mode(boolean) andtest_revision_ids(list) fields to theWikiConfigurationmodel - Modified
fetch_pending_pagesinWikiClientto use test revision IDs when test mode is enabled - Updated API views to expose and accept the new configuration fields
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/reviews/models/wiki_configuration.py | Added test_mode and test_revision_ids fields to WikiConfiguration model |
| app/reviews/migrations/0018_add_test_mode_fields.py | Database migration to add the new test mode fields |
| app/reviews/views.py | Updated views to expose and handle test mode configuration in API responses and updates |
| app/reviews/services/wiki_client.py | Modified fetch_pending_pages to use test revision IDs instead of flaggedpages when test mode is enabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Build CSV string for SQL IN clause | ||
| ids_csv = ",".join(valid_ids) |
There was a problem hiding this comment.
The ids_csv variable is directly interpolated into a SQL query at line 190 without parameterization. While the IDs are validated as integers, this pattern is vulnerable to SQL injection if the validation logic changes. Use parameterized queries or ensure this construction is well-documented as requiring validated integer strings only.
| # Build CSV string for SQL IN clause | |
| ids_csv = ",".join(valid_ids) | |
| # Prepare parameterized IN clause for revision IDs | |
| placeholders = ",".join(["%s"] * len(valid_ids)) |
| FROM | ||
| (SELECT rev_page AS fp_page_id, | ||
| rev_id AS fp_stable, | ||
| BINARY('20250901000000') AS fp_pending_since |
There was a problem hiding this comment.
The hardcoded timestamp '20250901000000' (September 1, 2025) is a magic value without explanation. This should either be documented with a comment explaining why this specific date is used for test mode, or extracted as a named constant.
| if cleaned.isdigit(): | ||
| validated_ids.append(cleaned) |
There was a problem hiding this comment.
The validation logic only accepts positive integers using isdigit(), which will reject valid revision IDs if they arrive as actual integers or fail to detect negative numbers passed as strings with a minus sign. Additionally, storing IDs as strings in a JSONField intended for integers is inconsistent. Convert to integers: replace line 593 with validated_ids.append(int(cleaned)) after using cleaned.lstrip('-').isdigit() or try/except int() conversion.
| if cleaned.isdigit(): | |
| validated_ids.append(cleaned) | |
| try: | |
| validated_ids.append(int(cleaned)) | |
| except ValueError: | |
| continue |
| for item in test_ids: | ||
| try: | ||
| val = int(str(item).strip()) | ||
| valid_ids.append(str(val)) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
The validation logic converts integers to strings for SQL interpolation but stores them as strings. This is inconsistent with lines 589-593 in views.py which also stores IDs as strings. Since revision IDs are integers, they should be stored as integers in the JSONField and only converted to strings when building the SQL query. This reduces confusion and ensures type consistency.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Create merge migration 0019 to handle conflicting 0018 migrations - Resolve conflict between test mode fields and statistics refactoring
This Patch brings back the test mode feature from #21, this time with properly ordered database migrations that aligns with the latest upstream changes. The earlier version had to be reverted because of migration file numbering conflicts, so this PR focuses on rebuilding the feature cleanly and conflict-free.
Related to #21