Add BookmarkFilterType enum and update bookmark retrieval logic#810
Conversation
tenkus47
commented
Jun 25, 2026
tenkus47
commented
- Introduced BookmarkFilterType enum to categorize bookmarks by TEXT, PLAN, SERIES, ACCUMULATOR, and TIMER.
- Updated get_bookmarks_by_user_id function to filter bookmarks based on the new BookmarkFilterType.
- Enhanced get_bookmarks_service to enrich bookmarks with additional data when filtered by TEXT.
- Modified BookmarkDTO to include new fields for text and segment information.
- Added tests to validate the new filtering functionality and ensure proper enrichment of text bookmarks.
- Introduced BookmarkFilterType enum to categorize bookmarks by TEXT, PLAN, SERIES, ACCUMULATOR, and TIMER. - Updated get_bookmarks_by_user_id function to filter bookmarks based on the new BookmarkFilterType. - Enhanced get_bookmarks_service to enrich bookmarks with additional data when filtered by TEXT. - Modified BookmarkDTO to include new fields for text and segment information. - Added tests to validate the new filtering functionality and ensure proper enrichment of text bookmarks.
Confidence Score: 4/5Mostly safe to merge; the enrichment and filter logic is correct for the happy path, but two defects noted in earlier review rounds remain unaddressed in the changed files. The guard on line 64 of bookmark_utils.py checks only segment_id and not segment, so a stale table-of-contents entry can produce a response with a valid-looking segment_id paired with null segment_content. Separately, bookmark_repository.py relies on BookmarkType[type.name] without a KeyError guard, meaning any future BookmarkFilterType value without a matching BookmarkType entry will return a 500. Both issues were raised in prior review rounds but remain in the code. pecha_api/bookmarks/bookmark_utils.py (segment guard), pecha_api/bookmarks/bookmark_repository.py (enum key lookup) Reviews (3): Last reviewed commit: "Merge branch 'develop' into update_bookm..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| else: | ||
| query = query.filter(Bookmark.type == BookmarkType[type.name]) |
There was a problem hiding this comment.
KeyError risk when enum values fall out of sync. BookmarkType[type.name] works today because every non-TEXT BookmarkFilterType value happens to exist in BookmarkType, but there is no compile-time or runtime guard. Adding a new BookmarkFilterType member without a matching BookmarkType entry would raise an unhandled KeyError, returning a 500 to the caller. An explicit mapping or a try/except KeyError makes the contract clear and the failure explicit.
| else: | |
| query = query.filter(Bookmark.type == BookmarkType[type.name]) | |
| else: | |
| try: | |
| bookmark_type = BookmarkType[type.name] | |
| except KeyError: | |
| raise HTTPException( | |
| status_code=status.HTTP_400_BAD_REQUEST, | |
| detail=ResponseError( | |
| error=BAD_REQUEST, | |
| message=f"Unsupported filter type: {type.value}", | |
| ).model_dump(), | |
| ) | |
| query = query.filter(Bookmark.type == bookmark_type) |
| elif bookmark.type == BookmarkType.TEXT: | ||
| text_id = bookmark.source_id | ||
| if bookmark.name: | ||
| candidate = await _resolve_segment_by_ref(bookmark.name) | ||
| if candidate and candidate.text_id == text_id: | ||
| verse_id = bookmark.name | ||
| segment_id, segment = await _resolve_text_segment(text_id=text_id, verse_id=verse_id) | ||
| if not segment_id: | ||
| return {} |
There was a problem hiding this comment.
bookmark.name resolves to a segment belonging to the text, _resolve_segment_by_ref(bookmark.name) is awaited here to validate ownership, and then awaited a second time inside _resolve_text_segment (line ~30) when it processes the same verse_id. This causes two async DB round-trips for the identical lookup. Consider short-circuiting _resolve_text_segment by passing the already-resolved candidate when it matches, or by returning early with candidate directly from enrich_text_bookmark.
- Introduced a new pytest plugin configuration in pyproject.toml to integrate the bookmark coverage plugin. - This addition enhances testing capabilities for the bookmark-related functionalities in the application.
| segment_id, segment = await _resolve_text_segment(text_id=text_id, verse_id=verse_id) | ||
| if not segment_id: | ||
| return {} |
There was a problem hiding this comment.
segment_id, not segment
_resolve_text_segment can return (segment_id, None) — specifically when get_first_segment_table_of_content yields a stale entry that get_segment_by_id can no longer find (lines 34–37 of the same file). The guard here only checks if not segment_id, so execution falls through and produces a result dict with a non-null segment_id paired with segment_content: None. API consumers receive a pointer to a segment that can't be fetched, with no signal that retrieval failed. The check should cover both: if not segment_id or not segment: return {}.
- Updated the test for creating a new plan with a series ID to verify that the series lookup is called twice with the correct database session and series ID. - Improved assertions to ensure the integrity of the series retrieval logic in the test case.
|
2 participants
