Allow the data explorer to add to existing collections - #1578
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds an authenticated API endpoint for populating collections from ISIC IDs. The data explorer now supports creating collections and adding results to existing collections through one searchable modal. API and browser tests cover permissions, locking, focus, search, and population. ChangesCollection population workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
isic/core/api/collection.py (1)
79-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the ISIC ID list length.
IsicIdssetsmin_length=1but no upper bound. The data explorer posts everyisic_idin the query result set, which can be very large. The full list travels in the HTTP body and then in the Celery message body. Add amax_lengththat matches the largest supported result set.♻️ Proposed change
-IsicIds = Annotated[list[Annotated[str, Field(pattern=ISIC_ID_REGEX)]], Field(min_length=1)] +IsicIds = Annotated[ + list[Annotated[str, Field(pattern=ISIC_ID_REGEX)]], Field(min_length=1, max_length=100_000) +]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isic/core/api/collection.py` at line 79, Update the IsicIds type alias to add a max_length matching the largest supported result set, while preserving its existing non-empty constraint and ISIC_ID_REGEX validation.isic/core/templates/core/data_explorer.html (1)
666-681: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard against out-of-order search responses.
searchCollectionsassignscollectionResultsfrom whichever response resolves last. The 300 ms debounce reduces overlap but does not prevent it. A slow response for an older query can overwrite results for the current query. Track a request token and discard stale responses.♻️ Proposed change
async searchCollections() { if (!this.hasCollectionQuery) { this.collectionResults = []; return; } + const requestId = ++this.collectionSearchId; this.collectionSearchPending = true; try { const {data} = await axiosSession.get("{% url 'api:collection_autocomplete' %}", { params: {query: this.collectionQuery.trim()}, }); + if (requestId !== this.collectionSearchId) { + return; + } this.collectionResults = data.map(c => ({id: c.id, name: c.name})); } catch (e) { + if (requestId !== this.collectionSearchId) { + return; + } this.collectionError = this.errorMessage(e); } finally { - this.collectionSearchPending = false; + if (requestId === this.collectionSearchId) { + this.collectionSearchPending = false; + }Add the counter to the state block near line 325:
collectionSearchId: 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isic/core/templates/core/data_explorer.html` around lines 666 - 681, Update the component state with the proposed collectionSearchId counter, then modify searchCollections to increment and capture a request token for each search. Before assigning collectionResults or collectionError, ignore the response when its token is no longer the latest, while preserving collectionSearchPending cleanup for the active request.isic/core/tests/test_data_explorer_browser.py (1)
237-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse exact collection-name matching.
get_by_role(..., name=...)uses case-insensitive substring matching by default. The factory does not guarantee unique collection names. Passexact=Trueto both locators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isic/core/tests/test_data_explorer_browser.py` around lines 237 - 240, Update both get_by_role calls in the collection search test to pass exact=True, ensuring button name matching uses the complete collection name rather than case-insensitive substring matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@isic/core/api/collection.py`:
- Line 79: Update the IsicIds type alias to add a max_length matching the
largest supported result set, while preserving its existing non-empty constraint
and ISIC_ID_REGEX validation.
In `@isic/core/templates/core/data_explorer.html`:
- Around line 666-681: Update the component state with the proposed
collectionSearchId counter, then modify searchCollections to increment and
capture a request token for each search. Before assigning collectionResults or
collectionError, ignore the response when its token is no longer the latest,
while preserving collectionSearchPending cleanup for the active request.
In `@isic/core/tests/test_data_explorer_browser.py`:
- Around line 237-240: Update both get_by_role calls in the collection search
test to pass exact=True, ensuring button name matching uses the complete
collection name rather than case-insensitive substring matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5f97a26-da73-4d0c-a83c-6fbc2e8ecca2
📒 Files selected for processing (5)
isic/core/api/collection.pyisic/core/templates/core/data_explorer.htmlisic/core/tests/test_api_collection.pyisic/core/tests/test_data_explorer_browser.pyisic/core/views/data_explorer.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes