Add region restriction handling for various services and views#850
Conversation
- Integrated timezone-based filtering for restricted items across multiple services, including accumulators, groups, mantras, plans, series, and recitations. - Updated service methods to accept a `timezone_name` parameter, ensuring that restricted items are hidden for Chinese timezones. - Enhanced view functions to include `X-Timezone` header support, allowing clients to specify their timezone for accurate data visibility. - Refactored relevant functions to utilize new filtering logic, improving data consistency and user experience.
Confidence Score: 4/5Safe to merge with one fix: the plan detail endpoint returns 500 instead of 404 to Chinese-timezone users for restricted plans. The pecha_api/plans/public/plan_service.py — the Reviews (8): Last reviewed commit: "Refactor region restriction services and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| recitations_with_images = get_recitations_with_image_urls(recitations=recitation_list_text_response.recitations) | ||
| recitations_with_first_segments = await get_recitations_with_first_segments(recitations=recitations_with_images) | ||
| visible_recitations = filter_items_for_timezone( | ||
| recitations_with_first_segments, | ||
| timezone_name=timezone_name, | ||
| item_type=RestrictedItemType.RECITATION, | ||
| id_of=lambda recitation: recitation.text_id, | ||
| ) | ||
|
|
||
| return RecitationsResponse( | ||
| recitations=recitations_with_first_segments, | ||
| recitations=visible_recitations, | ||
| skip=skip, | ||
| limit=limit, | ||
| total=recitation_list_text_response.total | ||
| ) |
There was a problem hiding this comment.
total returned after filtering recitations
recitation_list_text_response.total is the raw count from the upstream text service, fetched before filter_items_for_timezone runs. For a Chinese-timezone client, the returned recitations list is shorter than total advertises, breaking pagination. The same pattern also applies to series_service.py get_filtered_series and group_accumulator_service.py. The fix in each case is to derive the reported total from the post-filter list length (or, for better UX, move filtering to the database query level).
| @lru_cache(maxsize=1) | ||
| def _load_restricted_item_ids_by_type() -> dict[str, frozenset[UUID]]: | ||
| with SessionLocal() as db: | ||
| rows = get_all_china_restricted_items(db=db) | ||
|
|
||
| grouped: dict[str, set[UUID]] = defaultdict(set) | ||
| for row in rows: | ||
| item_type = ( | ||
| row.item_type.value | ||
| if hasattr(row.item_type, "value") | ||
| else str(row.item_type) | ||
| ) | ||
| grouped[item_type].add(row.item_id) | ||
| return {item_type: frozenset(item_ids) for item_type, item_ids in grouped.items()} | ||
|
|
||
|
|
||
| def clear_restricted_items_cache() -> None: | ||
| _load_restricted_item_ids_by_type.cache_clear() |
There was a problem hiding this comment.
lru_cache not shared across workers
_load_restricted_item_ids_by_type is cached per-process. When the app runs with multiple Gunicorn/Uvicorn workers, clear_restricted_items_cache() only clears the cache in the single worker that handled the admin create/delete request. All other workers continue serving stale restriction data until they restart. Consider a shared cache layer (e.g. Redis) or a short TTL to bound the inconsistency window.
| def assert_visible_for_timezone_db( | ||
| db, | ||
| *, | ||
| timezone_name: Optional[str], | ||
| item_type: RestrictedItemType, | ||
| item_id: UUID, | ||
| not_found_detail: str = "Not found", | ||
| ) -> None: | ||
| if not is_china_timezone(timezone_name): | ||
| return | ||
| if is_item_restricted_in_china(db=db, item_type=item_type, item_id=item_id): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=not_found_detail, | ||
| ) |
There was a problem hiding this comment.
assert_visible_for_timezone_db is defined but never called
This DB-querying variant of the visibility check is not wired up anywhere in the diff. If it is intended as a future-use helper, a short comment noting its purpose would help; otherwise it is dead code that adds surface area to maintain.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- Introduced `get_user_total_counted_by_parent` function to calculate the total history counts for a user across all accumulators associated with a specific parent ID, including soft-deleted instances. - Updated `get_accumulator_with_history` to utilize the new function for fetching the total counted, improving code clarity and reducing redundancy.
| assert_visible_for_timezone( | ||
| timezone_name=timezone_name, | ||
| item_type=RestrictedItemType.RECITATION, | ||
| item_id=UUID(text_id), | ||
| not_found_detail=ErrorConstants.TEXT_NOT_FOUND_MESSAGE, | ||
| ) |
There was a problem hiding this comment.
UUID(text_id) raises ValueError for any non-UUID string, which FastAPI propagates as an HTTP 500. Since the route declares text_id: str, the restriction check runs before FastAPI's own type coercion, so a caller passing something like /recitations/my-slug triggers an unhandled exception rather than a clean 404.
| assert_visible_for_timezone( | |
| timezone_name=timezone_name, | |
| item_type=RestrictedItemType.RECITATION, | |
| item_id=UUID(text_id), | |
| not_found_detail=ErrorConstants.TEXT_NOT_FOUND_MESSAGE, | |
| ) | |
| try: | |
| _text_uuid = UUID(text_id) | |
| except ValueError: | |
| _text_uuid = None | |
| if _text_uuid is not None: | |
| assert_visible_for_timezone( | |
| timezone_name=timezone_name, | |
| item_type=RestrictedItemType.RECITATION, | |
| item_id=_text_uuid, | |
| not_found_detail=ErrorConstants.TEXT_NOT_FOUND_MESSAGE, | |
| ) |
- Deleted the `Buddhist Traditions Taxonomy - i18n-v1.1.json` file as it is no longer needed. - Refactored `tradition_constants.py` to include a new function `tradition_id_from_code` for generating UUIDs from tradition codes. - Updated `tradition_prompt.py` to build tradition catalogs using the new onboarding methods. - Removed references to the old taxonomy in `tradition_repository.py`, simplifying the tradition retrieval process. - Adjusted `tradition_service.py` to utilize the new onboarding functions for tradition data handling. - Deleted `tradition_taxonomy.py` as its functionality has been replaced by the new onboarding structure.
|
|
||
|
|
||
| def create_admin_china_restricted_item( | ||
| token: str, | ||
| body: CreateChinaRestrictedItemRequest, | ||
| ) -> ChinaRestrictedItemDTO: | ||
| author = validate_and_extract_author_details(token=token) | ||
| require_super_admin(author) | ||
| with SessionLocal() as db: | ||
| if is_item_restricted_in_china( | ||
| db=db, | ||
| item_type=body.item_type, | ||
| item_id=body.item_id, | ||
| ): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_409_CONFLICT, | ||
| detail="Item is already restricted in China", |
There was a problem hiding this comment.
The is_item_restricted_in_china guard and the subsequent create_china_restricted_item are two separate DB round-trips with no transaction-level lock between them. Two concurrent admin requests for the same (item_type, item_id) pair can both pass the check and both proceed to db.commit(). The second commit hits the uq_china_restricted_items_type_id unique constraint and raises sqlalchemy.exc.IntegrityError, which propagates as an unhandled HTTP 500 instead of the intended 409.
Wrap the insert in a try/except IntegrityError block (or use INSERT ... ON CONFLICT DO NOTHING) and translate the exception to the existing 409 response.
- Removed the dependency on `get_collection_id_by_slug` and `get_root_text_by_collection_id` for fetching recitations. - Integrated `get_ordered_recitations_response` to enhance the retrieval process. - Updated the `get_list_of_recitations_service` function to directly utilize the new response structure, improving efficiency. - Adjusted related tests to reflect the changes in the service logic and ensure accurate validation of recitation data retrieval.
- Refactored daily log cache and service functions to accept an optional `timezone_name` parameter, allowing for accurate date handling based on user timezones. - Updated cache key generation and cache timeout calculations to consider the specified timezone, improving data consistency. - Modified view functions to include `X-Timezone` header support, enabling clients to specify their timezone for accurate data retrieval. - Adjusted related tests to validate the new timezone functionality across services and ensure correct behavior in various scenarios.
| async def is_user_logged_today_in_cache(user_id: UUID, log_date: date) -> bool: | ||
| cache_key = _build_daily_log_cache_key(user_id=user_id, log_date=log_date) | ||
| return await exists_in_cache(cache_key) |
There was a problem hiding this comment.
timezone_name passed to a function that doesn't accept it
is_user_logged_today_in_cache accepts only user_id and log_date, but record_daily_log_if_needed in daily_log_service.py (lines 59-63) calls it with timezone_name=timezone_name. Python raises TypeError: is_user_logged_today_in_cache() got an unexpected keyword argument 'timezone_name' at call time regardless of the value — so every request to /daily-log/streak and /daily-log/stats introduced in this PR will 500. The function signature needs a timezone_name: Optional[str] = None parameter (even if not used for the cache key) to match how all callers now invoke it.
- Streamlined the `record_daily_log_if_needed` function by removing the unnecessary `timezone_name` parameter from the cache check, improving code clarity. - Updated the corresponding test to reflect the change in the cache assertion, ensuring consistency in the testing logic.
- Implemented `delete_author_group` service to handle the deletion of author groups, including validation for super admin and owner roles. - Added `delete_cms_group` endpoint in the groups view to facilitate group deletion via HTTP DELETE requests. - Enhanced tests to cover successful deletion, permission checks, and not found scenarios for the new functionality.
- Removed the unused `assert_visible_for_timezone` function from the accumulator service to streamline the codebase. - Updated exception handling in the region restriction item lookup to catch specific exceptions (OSError, ValueError, TypeError, KeyError, AttributeError, HTTPException) instead of a generic Exception, improving error management. - Changed the default and onupdate parameters for timestamps in the ChinaRestrictedItem model to use a lambda function for better performance and accuracy. - Added new tests for metadata text selection and title resolution for various item types, ensuring robust functionality and coverage.
|
1 participant
timezone_nameparameter, ensuring that restricted items are hidden for Chinese timezones.X-Timezoneheader support, allowing clients to specify their timezone for accurate data visibility.