Merged
Conversation
- Added medical specialty routing to the API for better organization and access. - Updated practitioner filtering to utilize specialty_id instead of the deprecated specialty string. - Enhanced CRUD operations for medical specialties, allowing for improved data handling and validation. - Refactored frontend components to support the new specialty management system, including updates to forms and selection components. - Removed legacy specialty handling from the practitioner model and related schemas, streamlining the data structure.
…lties - Changed practitioner model to use `specialty_id` instead of the legacy `specialty` string. - Enhanced API reference documentation to clarify the requirements for `specialty_id` and its relationship with medical specialties. - Added new section for managing medical specialties, including endpoints for listing and creating specialties. - Updated database schema to reflect the new `medical_specialties` table and its relationship with practitioners. - Improved response structures in API documentation to include resolved names for specialties.
📝 WalkthroughWalkthroughNormalize practitioner specialties: drop legacy practitioners.specialty via migration (with validation), seed and expose medical_specialties, migrate APIs/schemas/CRUD to use specialty_id FK, add public medical-specialties endpoints (with rate limiting), update frontend to use specialtyApi and specialty-select, and update tests/docs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User Browser
participant UI as SpecialtySelect (frontend)
participant API as specialtyApi (frontend)
participant Server as API Server (/api/v1)
participant DB as Database (medical_specialties)
participant Log as ActivityLog
Browser->>UI: open form / quick-add specialty
UI->>API: migrateLegacyCustomSpecialties() [reads localStorage]
API->>Server: POST /medical-specialties/ (for each migrated name)
Server->>DB: INSERT or SELECT medical_specialties (case-insensitive)
DB-->>Server: existing or new row (id)
Server->>Log: record CREATED activity (when new)
Server-->>API: 201 (created) or 200 (existing) with specialty object
API-->>UI: return created/existing specialty
UI-->>Browser: update options, set selected specialty_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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.
Actionable comments posted: 3
🧹 Nitpick comments (8)
app/crud/treatment.py (1)
150-155: Optional: deduplicate identical eager-load options.Both query methods now repeat the same
.options(...)tree. Extracting this into one shared constant/helper would reduce drift risk in future relation changes.Refactor sketch
class CRUDTreatmentMedication( CRUDBase[TreatmentMedication, TreatmentMedicationCreate, TreatmentMedicationUpdate] ): + _EAGER_LOAD_OPTIONS = ( + joinedload(TreatmentMedication.medication) + .joinedload(Medication.practitioner) + .joinedload(Practitioner.specialty_rel), + joinedload(TreatmentMedication.specific_prescriber) + .joinedload(Practitioner.specialty_rel), + joinedload(TreatmentMedication.specific_pharmacy), + ) + def get_by_treatment( self, db: Session, *, treatment_id: int ) -> List[TreatmentMedication]: return ( db.query(self.model) - .options(...) + .options(*self._EAGER_LOAD_OPTIONS) .filter(self.model.treatment_id == treatment_id) .all() ) def get_by_medication( self, db: Session, *, medication_id: int ) -> List[TreatmentMedication]: return ( db.query(self.model) - .options(...) + .options(*self._EAGER_LOAD_OPTIONS) .filter(self.model.medication_id == medication_id) .all() )Also applies to: 169-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/crud/treatment.py` around lines 150 - 155, Two query methods repeat the same eager-load tree; extract the repeated joinedload(...) tuples into a single shared constant or helper and reuse it in both .options(...) calls to avoid duplication. Create a constant (e.g., TREATMENT_MEDICATION_EAGER_OPTIONS) composed of the two joinedload entries referencing TreatmentMedication.medication -> Medication.practitioner -> Practitioner.specialty_rel and TreatmentMedication.specific_prescriber -> Practitioner.specialty_rel, then replace the duplicated .options(...) arguments with .options(*TREATMENT_MEDICATION_EAGER_OPTIONS) in both query methods.frontend/src/components/admin/SpecialtySelect.jsx (1)
29-30: Fail loudly on an unexpected specialties payload.
Array.isArray(items) ? items : []turns an API contract regression into an empty dropdown, which is hard to distinguish from “there are no specialties”. Prefer normalizing inspecialtyApi.list()or throwing/logging here so the existing error path is used.Suggested change
- const items = await specialtyApi.list(); - setSpecialties(Array.isArray(items) ? items : []); + const items = await specialtyApi.list(); + if (!Array.isArray(items)) { + throw new Error('Unexpected medical specialties response shape'); + } + setSpecialties(items);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/SpecialtySelect.jsx` around lines 29 - 30, The current code silently converts a non-array API response into an empty list (items -> setSpecialties(Array.isArray(items) ? items : [])), masking contract regressions; change this to fail loudly by validating the return of specialtyApi.list(): if items is not an array, either throw an error or log and rethrow so the existing error handling runs (or move normalization into specialtyApi.list() so it always returns an array), and then call setSpecialties(items) unconditionally; reference specialtyApi.list(), the items variable, and setSpecialties when making the change.tests/api/test_practitioners.py (1)
203-226: Assert the migrated specialty field in at least one happy-path API test.These tests now send
specialty_id, but they never verify that the response actually persisted/serialized that field. Adding onespecialty_idassertion here would make the migration coverage much stronger.Suggested change
assert response.status_code == 200 data = response.json() assert data["name"] == "Dr. Sarah Johnson" + assert data["specialty_id"] == default_specialty.id assert data["practice"] == "City Medical Center" assert data["email"] == "sarah.johnson@citymed.com" assert data["rating"] == 4.8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_practitioners.py` around lines 203 - 226, The test test_create_practitioner_with_all_fields currently sends specialty_id in practitioner_data but never asserts it was persisted/returned; update the assertions after response = authenticated_client.post(...) to verify the migrated field by checking the response JSON includes the specialty identifier (e.g., assert data["specialty_id"] == default_specialty.id) so the test confirms the specialty was serialized/persisted correctly.tests/api/test_medical_specialty_public.py (1)
1-4: Docstring mentions rate limiting coverage but no rate limit test is present.The module docstring mentions "per-user rate limiting" as part of test coverage, but there's no test verifying the rate limiting behavior (e.g., hitting the endpoint multiple times and expecting a 429 response).
Consider adding a rate limit test or updating the docstring to reflect actual coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_medical_specialty_public.py` around lines 1 - 4, Docstring claims "per-user rate limiting" but no such test exists; either add a rate-limit test or remove/update the docstring. To fix, add a new test function (e.g., test_medical_specialty_rate_limit) in tests/api/test_medical_specialty_public.py that simulates a single user making repeated requests to the public /medical-specialties endpoint until the configured limit is exceeded and asserts a 429 response on the excess request, reusing the existing test client and auth helper(s), or alternatively remove the "per-user rate limiting" phrase from the module docstring so it accurately reflects current tests.tests/crud/test_practitioner.py (1)
173-175: Remove unuseddb_session_cleanparameter.The parameter
db_session_clean=Noneis never used within the test method and appears to be leftover from refactoring.Proposed fix
- def test_create_if_not_exists_returns_existing( - self, db_session: Session, db_session_clean=None - ): + def test_create_if_not_exists_returns_existing(self, db_session: Session):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crud/test_practitioner.py` around lines 173 - 175, The test method test_create_if_not_exists_returns_existing declares an unused parameter db_session_clean; remove db_session_clean=None from the method signature so only the required fixture db_session remains, updating any test callers if necessary and ensuring the function signature matches other tests (reference the test method name test_create_if_not_exists_returns_existing to locate the change).app/schemas/practitioner.py (1)
306-329: Consider includingspecialty_idin the output schema.The
Practitionerresponse schema includes the resolvedspecialtyname but not thespecialty_idFK. API consumers may need both: the display name for UI and the ID for subsequent API calls (e.g., filtering practitioners by specialty).Since
PractitionerBasealready hasspecialty_id, andPractitionerinherits from it, the field should already be present. However, the example response in the docstring (lines 314-319) doesn't show it, which may confuse API consumers.Update docstring example to include specialty_id
Example response: { "id": 1, "name": "Dr. John Smith", + "specialty_id": 3, "specialty": "Cardiology", "practice_name": "City General Hospital" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/schemas/practitioner.py` around lines 306 - 329, The docstring example for the Practitioner response is missing the specialty_id FK which is present via PractitionerBase inheritance; update the Practitioner class docstring example to include "specialty_id": <int> so API consumers see both the display name and the ID, and verify that Practitioner still inherits specialty_id from PractitionerBase (class Practitioner and PractitionerBase are the symbols to check).app/api/v1/endpoints/medical_specialty.py (2)
36-64: Consider adding periodic cleanup for stale user entries.The
_UserRateLimiterstores entries peruser_idindefinitely. While the window entries are pruned inallow(), the top-level dict keys (user IDs) are never removed. Over time with many unique users, this could cause unbounded memory growth.For abuse prevention this is likely acceptable, but consider adding a periodic cleanup of users with empty windows, or using a TTL-based cache like
cachetools.TTLCache.Optional: Add cleanup of empty user entries
def allow(self, user_id: int) -> bool: now = time.time() window = self.requests[user_id] while window and window[0] <= now - self.window_seconds: window.popleft() + # Clean up empty entries to prevent unbounded growth + if not window and user_id in self.requests: + del self.requests[user_id] + window = self.requests[user_id] # Re-create via defaultdict if len(window) < self.max_requests: window.append(now) return True return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/v1/endpoints/medical_specialty.py` around lines 36 - 64, The _UserRateLimiter keeps per-user deques in self.requests forever causing unbounded growth; update the implementation to remove user_id keys when their deque becomes empty (after pruning in allow and retry_after) or add a cleanup method (e.g., cleanup_stale_users) that scans self.requests and deletes entries with empty deques and call it periodically or from allow()/retry_after() to reclaim memory; refer to class _UserRateLimiter, its requests attribute, and the allow() and retry_after() methods when making the change.
104-116: Rate limiting applied before existence check - potential UX concern.The rate limit is decremented even when the specialty already exists (and would return 200 OK). A user repeatedly submitting the same specialty name would hit the rate limit despite not creating new records.
Consider moving the rate limit check after the existence check, or only counting requests that actually create new records.
Alternative: Only rate-limit actual creations
+ with handle_database_errors(request=request): + existing = medical_specialty.get_by_name(db, name=specialty_in.name) + if existing: + response.status_code = status.HTTP_200_OK + return existing + if not _create_limiter.allow(current_user_id): retry_after = _create_limiter.retry_after(current_user_id) raise HTTPException( status_code=status.HTTP_429_TOO_MANY_REQUESTS, detail="Too many specialty create requests. Try again later.", headers={"Retry-After": str(retry_after)}, ) - with handle_database_errors(request=request): - existing = medical_specialty.get_by_name(db, name=specialty_in.name) - if existing: - response.status_code = status.HTTP_200_OK - return existing - + with handle_database_errors(request=request): created = medical_specialty.create(db, obj_in=specialty_in)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/v1/endpoints/medical_specialty.py` around lines 104 - 116, The rate limiter (_create_limiter.allow / _create_limiter.retry_after) is being consumed before checking if the specialty already exists, so repeated lookups against medical_specialty.get_by_name(specialty_in.name) will decrement the quota even when nothing is created; move the existence check (medical_specialty.get_by_name) ahead of the rate-limit check or only invoke/decrement the limiter when you are about to create a new record (i.e., after confirming not existing and before calling the create flow), ensuring response.status_code and return of existing remain untouched and that headers/HTTPException using _create_limiter.retry_after are only used for actual creation attempts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/medical/BaseMedicalForm.jsx`:
- Around line 613-639: The specialty field renders label/description/error
visually but does not associate them with the input control; update the
SpecialtySelect usage in BaseMedicalForm.jsx to pass semantic props (label,
description, required, error) into the <SpecialtySelect /> call (e.g.,
label={label}, description={description}, required={required},
error={fieldErrors[name]}), and then modify
frontend/src/components/admin/SpecialtySelect.jsx to accept and forward those
props to its inner Mantine Select (ensure it accepts label, description,
required, error and passes them through along with value, onChange, placeholder
and hasError so the Select receives proper accessibility attributes).
In `@frontend/src/services/api/specialtyApi.js`:
- Around line 64-76: Update the catch block that currently checks only
err?.message?.includes('401') so it treats auth failures when the message is
either a 401 or "Unauthorized": change the condition in the catch for the legacy
specialty migration (the block that returns on auth failure before logging
'legacy_specialty_migration_item_failed') to detect both patterns (e.g. use a
case-insensitive match for "401" or "Unauthorized" against err.message) and keep
the existing logger.warn path and return behavior unchanged; reference the same
logger, name variable, and 'SpecialtyApi' context when making the change.
In `@tests/conftest.py`:
- Around line 296-313: The test SQLite engine needs foreign-key enforcement
enabled so fixtures like default_specialty and sample_practitioner_data validate
specialty_id correctly; update the test DB setup (the
test_db_engine/test_db_session factory used by tests) to execute "PRAGMA
foreign_keys=ON" on each SQLite connection (e.g., via a connect-time PRAGMA or
SQLAlchemy event listener like on_connect/engine.connect) so tests mirror
production DB behavior and enforce FK constraints for MedicalSpecialty
references.
---
Nitpick comments:
In `@app/api/v1/endpoints/medical_specialty.py`:
- Around line 36-64: The _UserRateLimiter keeps per-user deques in self.requests
forever causing unbounded growth; update the implementation to remove user_id
keys when their deque becomes empty (after pruning in allow and retry_after) or
add a cleanup method (e.g., cleanup_stale_users) that scans self.requests and
deletes entries with empty deques and call it periodically or from
allow()/retry_after() to reclaim memory; refer to class _UserRateLimiter, its
requests attribute, and the allow() and retry_after() methods when making the
change.
- Around line 104-116: The rate limiter (_create_limiter.allow /
_create_limiter.retry_after) is being consumed before checking if the specialty
already exists, so repeated lookups against
medical_specialty.get_by_name(specialty_in.name) will decrement the quota even
when nothing is created; move the existence check
(medical_specialty.get_by_name) ahead of the rate-limit check or only
invoke/decrement the limiter when you are about to create a new record (i.e.,
after confirming not existing and before calling the create flow), ensuring
response.status_code and return of existing remain untouched and that
headers/HTTPException using _create_limiter.retry_after are only used for actual
creation attempts.
In `@app/crud/treatment.py`:
- Around line 150-155: Two query methods repeat the same eager-load tree;
extract the repeated joinedload(...) tuples into a single shared constant or
helper and reuse it in both .options(...) calls to avoid duplication. Create a
constant (e.g., TREATMENT_MEDICATION_EAGER_OPTIONS) composed of the two
joinedload entries referencing TreatmentMedication.medication ->
Medication.practitioner -> Practitioner.specialty_rel and
TreatmentMedication.specific_prescriber -> Practitioner.specialty_rel, then
replace the duplicated .options(...) arguments with
.options(*TREATMENT_MEDICATION_EAGER_OPTIONS) in both query methods.
In `@app/schemas/practitioner.py`:
- Around line 306-329: The docstring example for the Practitioner response is
missing the specialty_id FK which is present via PractitionerBase inheritance;
update the Practitioner class docstring example to include "specialty_id": <int>
so API consumers see both the display name and the ID, and verify that
Practitioner still inherits specialty_id from PractitionerBase (class
Practitioner and PractitionerBase are the symbols to check).
In `@frontend/src/components/admin/SpecialtySelect.jsx`:
- Around line 29-30: The current code silently converts a non-array API response
into an empty list (items -> setSpecialties(Array.isArray(items) ? items : [])),
masking contract regressions; change this to fail loudly by validating the
return of specialtyApi.list(): if items is not an array, either throw an error
or log and rethrow so the existing error handling runs (or move normalization
into specialtyApi.list() so it always returns an array), and then call
setSpecialties(items) unconditionally; reference specialtyApi.list(), the items
variable, and setSpecialties when making the change.
In `@tests/api/test_medical_specialty_public.py`:
- Around line 1-4: Docstring claims "per-user rate limiting" but no such test
exists; either add a rate-limit test or remove/update the docstring. To fix, add
a new test function (e.g., test_medical_specialty_rate_limit) in
tests/api/test_medical_specialty_public.py that simulates a single user making
repeated requests to the public /medical-specialties endpoint until the
configured limit is exceeded and asserts a 429 response on the excess request,
reusing the existing test client and auth helper(s), or alternatively remove the
"per-user rate limiting" phrase from the module docstring so it accurately
reflects current tests.
In `@tests/api/test_practitioners.py`:
- Around line 203-226: The test test_create_practitioner_with_all_fields
currently sends specialty_id in practitioner_data but never asserts it was
persisted/returned; update the assertions after response =
authenticated_client.post(...) to verify the migrated field by checking the
response JSON includes the specialty identifier (e.g., assert
data["specialty_id"] == default_specialty.id) so the test confirms the specialty
was serialized/persisted correctly.
In `@tests/crud/test_practitioner.py`:
- Around line 173-175: The test method
test_create_if_not_exists_returns_existing declares an unused parameter
db_session_clean; remove db_session_clean=None from the method signature so only
the required fixture db_session remains, updating any test callers if necessary
and ensuring the function signature matches other tests (reference the test
method name test_create_if_not_exists_returns_existing to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2dbf5ae-7864-4b4e-8360-8b34fc337e13
📒 Files selected for processing (42)
alembic/migrations/versions/20260422_0900_a3b4c5d6e7f8_drop_practitioner_specialty_column.pyalembic/migrations/versions/20260422_1000_b4c5d6e7f8a9_seed_default_medical_specialties.pyapp/api/v1/admin/models.pyapp/api/v1/api.pyapp/api/v1/endpoints/medical_specialty.pyapp/api/v1/endpoints/practitioner.pyapp/crud/medical_specialty.pyapp/crud/practitioner.pyapp/crud/treatment.pyapp/models/activity_log.pyapp/models/practice.pyapp/schemas/medical_specialty.pyapp/schemas/practitioner.pyapp/services/export_service.pydocs/developer-guide/02-api-reference.mddocs/developer-guide/03-database-schema.mdfrontend/public/locales/en/admin.jsonfrontend/src/components/admin/FieldRenderer.jsxfrontend/src/components/admin/SpecialtySelect.jsxfrontend/src/components/medical/BaseMedicalForm.jsxfrontend/src/components/medical/MantinePractitionerForm.jsxfrontend/src/components/medical/practitioners/PractitionerFormWrapper.jsxfrontend/src/config/medicalSpecialties.jsfrontend/src/pages/medical/Practitioners.jsxfrontend/src/services/api/README.mdfrontend/src/services/api/specialtyApi.jsfrontend/src/utils/medicalFormFields/practitioner.jstests/api/test_conditions.pytests/api/test_encounter.pytests/api/test_medical_specialty_admin.pytests/api/test_medical_specialty_public.pytests/api/test_practice.pytests/api/test_practitioners.pytests/api/test_vitals.pytests/conftest.pytests/crud/test_condition.pytests/crud/test_immunization.pytests/crud/test_lab_result.pytests/crud/test_medical_specialty.pytests/crud/test_practice.pytests/crud/test_practitioner.pytests/crud/test_procedure.py
💤 Files with no reviewable changes (5)
- frontend/public/locales/en/admin.json
- frontend/src/components/admin/FieldRenderer.jsx
- tests/api/test_medical_specialty_admin.py
- frontend/src/config/medicalSpecialties.js
- frontend/src/components/medical/MantinePractitionerForm.jsx
Comment on lines
+613
to
+639
| case 'specialty-select': | ||
| return ( | ||
| <div> | ||
| <Text size="sm" fw={500} mb={5}> | ||
| {label} | ||
| {required && <span style={{ color: 'red' }}> *</span>} | ||
| </Text> | ||
| {description && ( | ||
| <Text size="xs" c="dimmed" mb={5}> | ||
| {description} | ||
| </Text> | ||
| )} | ||
| <SpecialtySelect | ||
| value={formData[name]} | ||
| onChange={next => | ||
| onInputChange({ target: { name, value: next } }) | ||
| } | ||
| placeholder={placeholder} | ||
| hasError={Boolean(fieldErrors[name])} | ||
| /> | ||
| {fieldErrors[name] && ( | ||
| <Text size="xs" c="red" mt={4}> | ||
| {fieldErrors[name]} | ||
| </Text> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Associate specialty field label/error with the actual input control.
Line 616 and Line 633 render text visually, but the input itself is not receiving semantic label/description/full error props. This breaks proper label/error association for assistive tech.
Proposed direction
case 'specialty-select':
return (
- <div>
- <Text size="sm" fw={500} mb={5}>
- {label}
- {required && <span style={{ color: 'red' }}> *</span>}
- </Text>
- {description && (
- <Text size="xs" c="dimmed" mb={5}>
- {description}
- </Text>
- )}
- <SpecialtySelect
- value={formData[name]}
- onChange={next =>
- onInputChange({ target: { name, value: next } })
- }
- placeholder={placeholder}
- hasError={Boolean(fieldErrors[name])}
- />
- {fieldErrors[name] && (
- <Text size="xs" c="red" mt={4}>
- {fieldErrors[name]}
- </Text>
- )}
- </div>
+ <SpecialtySelect
+ value={formData[name]}
+ onChange={next =>
+ onInputChange({ target: { name, value: next } })
+ }
+ placeholder={placeholder}
+ label={label}
+ description={description}
+ required={required}
+ error={fieldErrors[name] || null}
+ hasError={Boolean(fieldErrors[name])}
+ />
);Also update frontend/src/components/admin/SpecialtySelect.jsx to accept/forward label, description, required, and error to its inner Mantine Select.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/medical/BaseMedicalForm.jsx` around lines 613 - 639,
The specialty field renders label/description/error visually but does not
associate them with the input control; update the SpecialtySelect usage in
BaseMedicalForm.jsx to pass semantic props (label, description, required, error)
into the <SpecialtySelect /> call (e.g., label={label},
description={description}, required={required}, error={fieldErrors[name]}), and
then modify frontend/src/components/admin/SpecialtySelect.jsx to accept and
forward those props to its inner Mantine Select (ensure it accepts label,
description, required, error and passes them through along with value, onChange,
placeholder and hasError so the Select receives proper accessibility
attributes).
- Simplified the user rate limiter by using a plain dictionary for request tracking, enhancing memory management. - Updated the create_or_get_specialty function to ensure rate limits only apply to actual create requests, improving user experience. - Enhanced the SpecialtySelect component in the frontend to support additional props for better usability. - Refactored the BaseMedicalForm component to streamline the rendering of the SpecialtySelect component. - Added error handling in the specialtyApi to ensure proper response types are returned.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/admin/SpecialtySelect.jsx (1)
74-87:⚠️ Potential issue | 🟡 MinorUse neutral success copy here.
This POST can return an existing specialty with HTTP 200 for a case-insensitive match, so
"Specialty created"is incorrect on that path. A neutral message like"Specialty saved"or"Specialty ready to use"fits both outcomes.✏️ Suggested tweak
notifications.show({ title: t('shared:labels.success', 'Success'), message: t( - 'admin:practitioner.specialtyCreated', - 'Specialty created' + 'admin:practitioner.specialtySaved', + 'Specialty saved' ), color: 'green', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/SpecialtySelect.jsx` around lines 74 - 87, The success notification currently always uses the "Specialty created" copy though specialtyApi.create can return an existing specialty (HTTP 200); update the notifications.show call to use neutral copy (e.g., t('admin:practitioner.specialtySaved','Specialty saved') or t('admin:practitioner.specialtyReady','Specialty ready to use')) instead of 'admin:practitioner.specialtyCreated', and add/update the corresponding translation key(s); this change touches the block after specialtyApi.create() where loadSpecialties(), onChange(created.id) and notifications.show(...) are called (look for specialtyApi.create, loadSpecialties, onChange, notifications.show).
♻️ Duplicate comments (1)
frontend/src/services/api/specialtyApi.js (1)
63-90:⚠️ Potential issue | 🟠 MajorDon't mark the migration complete after partial failures.
Any non-auth error is only logged, but
markComplete()still deletescustomMedicalSpecialtiesand sets the migrated flag. Becauseapp/api/v1/endpoints/medical_specialty.py:122-128returns 429 after 20 creates/hour, users with more than 20 cached names will deterministically lose everything after the 20th item. Keep the failed names in localStorage and retry later instead of marking completion.🛠️ Suggested fix
- let migrated = 0; + let migrated = 0; + const remaining = []; for (const rawName of names) { const name = typeof rawName === 'string' ? rawName.trim() : ''; if (!name) continue; try { await this.create({ name }); migrated += 1; } catch (err) { // Abort on auth failure so we retry next time the user loads the // app while authenticated. All other errors are per-item and // shouldn't prevent the remaining names from migrating. if (/401|unauthorized/i.test(err?.message || '')) { return; } + remaining.push(name); logger.warn( 'legacy_specialty_migration_item_failed', 'Failed to migrate a legacy specialty', { name, error: err.message, component: 'SpecialtyApi' } ); } } @@ - markComplete(); + if (remaining.length === 0) { + markComplete(); + } else { + localStorage.setItem(LEGACY_LOCALSTORAGE_KEY, JSON.stringify(remaining)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/api/specialtyApi.js` around lines 63 - 90, The migration currently calls markComplete() unconditionally even when some items failed (e.g., rate-limited 429), which deletes customMedicalSpecialties and loses pending names; change the flow in the migration loop (referencing names, create, migrated, logger.warn) to collect failed names into a list and only call markComplete() if failed list is empty, otherwise write the remaining failed names back to localStorage (do not call markComplete) so they can be retried later; keep the existing early return on auth errors (401/unauthorized).
🧹 Nitpick comments (2)
tests/conftest.py (1)
316-328: Remove extraneous fields that don't exist in the schema.The
sample_practitioner_datafixture includesaddressandstatusfields that don't exist inPractitionerBase/PractitionerCreate. These are silently ignored by Pydantic but create misleading test data. Consider removing them to keep the fixture aligned with the actual schema.🧹 Proposed cleanup
`@pytest.fixture` def sample_practitioner_data(default_specialty): """Sample practitioner data for testing.""" return { "name": "Dr. Test Smith", "specialty_id": default_specialty.id, "phone_number": "555-0123", "email": "dr.test@example.com", - "address": "123 Medical Center Dr", "website": "https://drtest.com", "rating": 4.5, - "status": "active", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 316 - 328, The fixture sample_practitioner_data includes extraneous keys ("address" and "status") that are not part of the PractitionerBase/PractitionerCreate schema; remove those keys from the returned dict in sample_practitioner_data so the fixture only contains valid fields (e.g., name, specialty_id, phone_number, email, website, rating) and update any tests relying on the removed keys to use the proper schema fields or a separate fixture if needed.app/schemas/practitioner.py (1)
261-303: Extract duplicate validation logic to shared helpers.The
validate_website_updateandvalidate_rating_updatemethods duplicate the logic from their counterparts inPractitionerBase. The file already uses shared helper functions for practice and email validation (_validate_practice_value,_validate_email_value). Apply the same pattern here for consistency.♻️ Proposed refactor for website validation
+def _validate_website_value(v: Optional[str]) -> Optional[str]: + """Shared website URL validation logic.""" + if v is None or v.strip() == "": + return None + + url_pattern = re.compile( + r"^https?://" + r"(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+[A-Z]{2,6}\.?|" + r"localhost|" + r"\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})" + r"(?::\d+)?" + r"(?:/?|[/?]\S+)$", + re.IGNORECASE, + ) + + cleaned_url = v.strip() + if not cleaned_url.startswith(("http://", "https://")): + cleaned_url = "https://" + cleaned_url + + if not url_pattern.match(cleaned_url): + raise ValueError("Please enter a valid website URL") + + return cleaned_url + + +def _validate_rating_value(v: Optional[float]) -> Optional[float]: + """Shared rating validation logic.""" + if v is None: + return None + if not isinstance(v, (int, float)): + raise ValueError("Rating must be a number") + if v < 0 or v > 5: + raise ValueError("Rating must be between 0 and 5") + return round(float(v), 1)Then use in both classes:
`@field_validator`("website") `@classmethod` def validate_website(cls, v): return _validate_website_value(v) `@field_validator`("rating") `@classmethod` def validate_rating(cls, v): return _validate_rating_value(v)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/schemas/practitioner.py` around lines 261 - 303, The validate_website_update and validate_rating_update methods in this class duplicate logic already present in PractitionerBase; extract and reuse shared helpers by replacing their bodies to call the centralized validators (e.g. call _validate_website_value(v) from validate_website_update and _validate_rating_value(v) from validate_rating_update) so both this class and PractitionerBase share the same validation logic; ensure the methods keep the same decorator/signature (`@field_validator`("website")/@field_validator("rating"), `@classmethod`) and return the helper result to preserve behavior and rounding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/v1/endpoints/medical_specialty.py`:
- Around line 116-130: Replace the read-then-insert flow with the race-safe
helper: stop calling medical_specialty.get_by_name(db, name=...) followed by
medical_specialty.create(...); instead call medical_specialty.get_or_create(db,
obj_in=specialty_in) so the CRUD layer handles IntegrityError and deduplication
for MedicalSpecialty.name. Keep the rate-limiter check (_create_limiter.allow
and retry_after) and the surrounding handle_database_errors context, but remove
the separate existence check and create call and use the get_or_create return
value to set response.status_code and return the specialty.
---
Outside diff comments:
In `@frontend/src/components/admin/SpecialtySelect.jsx`:
- Around line 74-87: The success notification currently always uses the
"Specialty created" copy though specialtyApi.create can return an existing
specialty (HTTP 200); update the notifications.show call to use neutral copy
(e.g., t('admin:practitioner.specialtySaved','Specialty saved') or
t('admin:practitioner.specialtyReady','Specialty ready to use')) instead of
'admin:practitioner.specialtyCreated', and add/update the corresponding
translation key(s); this change touches the block after specialtyApi.create()
where loadSpecialties(), onChange(created.id) and notifications.show(...) are
called (look for specialtyApi.create, loadSpecialties, onChange,
notifications.show).
---
Duplicate comments:
In `@frontend/src/services/api/specialtyApi.js`:
- Around line 63-90: The migration currently calls markComplete()
unconditionally even when some items failed (e.g., rate-limited 429), which
deletes customMedicalSpecialties and loses pending names; change the flow in the
migration loop (referencing names, create, migrated, logger.warn) to collect
failed names into a list and only call markComplete() if failed list is empty,
otherwise write the remaining failed names back to localStorage (do not call
markComplete) so they can be retried later; keep the existing early return on
auth errors (401/unauthorized).
---
Nitpick comments:
In `@app/schemas/practitioner.py`:
- Around line 261-303: The validate_website_update and validate_rating_update
methods in this class duplicate logic already present in PractitionerBase;
extract and reuse shared helpers by replacing their bodies to call the
centralized validators (e.g. call _validate_website_value(v) from
validate_website_update and _validate_rating_value(v) from
validate_rating_update) so both this class and PractitionerBase share the same
validation logic; ensure the methods keep the same decorator/signature
(`@field_validator`("website")/@field_validator("rating"), `@classmethod`) and
return the helper result to preserve behavior and rounding.
In `@tests/conftest.py`:
- Around line 316-328: The fixture sample_practitioner_data includes extraneous
keys ("address" and "status") that are not part of the
PractitionerBase/PractitionerCreate schema; remove those keys from the returned
dict in sample_practitioner_data so the fixture only contains valid fields
(e.g., name, specialty_id, phone_number, email, website, rating) and update any
tests relying on the removed keys to use the proper schema fields or a separate
fixture if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe4178a3-428a-42f5-84e5-e37a92a2041b
📒 Files selected for processing (11)
app/api/v1/endpoints/medical_specialty.pyapp/crud/treatment.pyapp/schemas/practitioner.pyfrontend/src/components/admin/SpecialtySelect.jsxfrontend/src/components/medical/BaseMedicalForm.jsxfrontend/src/services/api/specialtyApi.jstest_database.db-journaltests/api/test_medical_specialty_public.pytests/api/test_practitioners.pytests/conftest.pytests/crud/test_practitioner.py
✅ Files skipped from review due to trivial changes (1)
- app/crud/treatment.py
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/medical/BaseMedicalForm.jsx
- tests/api/test_practitioners.py
- tests/api/test_medical_specialty_public.py
- tests/crud/test_practitioner.py
Comment on lines
+36
to
+79
| class _UserRateLimiter: | ||
| """Tiny in-memory per-user rate limiter. | ||
|
|
||
| Mirrors the pattern from ``app.api.v1.endpoints.system.SimpleRateLimiter`` | ||
| but keyed on user_id instead of IP so SSO users behind a shared NAT | ||
| aren't limited collectively. Not durable across restarts — adequate for | ||
| abuse prevention, not for hard quotas. | ||
| """ | ||
|
|
||
| def __init__(self, max_requests: int, window_seconds: int): | ||
| self.max_requests = max_requests | ||
| self.window_seconds = window_seconds | ||
| # Plain dict — entries are only created when we actually record a | ||
| # request, and pruned-to-empty entries are deleted so idle users | ||
| # don't accumulate forever. | ||
| self.requests: Dict[int, deque] = {} | ||
|
|
||
| def allow(self, user_id: int) -> bool: | ||
| now = time.time() | ||
| window = self.requests.get(user_id) | ||
| if window is not None: | ||
| while window and window[0] <= now - self.window_seconds: | ||
| window.popleft() | ||
| if not window: | ||
| del self.requests[user_id] | ||
| window = None | ||
| if window is None: | ||
| window = deque() | ||
| if len(window) < self.max_requests: | ||
| window.append(now) | ||
| self.requests[user_id] = window | ||
| return True | ||
| return False | ||
|
|
||
| def retry_after(self, user_id: int) -> int: | ||
| window = self.requests.get(user_id) | ||
| if not window: | ||
| return 0 | ||
| return max(1, int(window[0] + self.window_seconds - time.time())) | ||
|
|
||
|
|
||
| # 20 specialty creates per user per hour — tight enough to prevent abuse, | ||
| # generous enough for a real user onboarding multiple practitioners in one session. | ||
| _create_limiter = _UserRateLimiter(max_requests=20, window_seconds=3600) |
There was a problem hiding this comment.
This limiter won't enforce a real per-user cap in production.
allow() mutates shared state without synchronization, and every app worker gets its own in-memory budget. Concurrent POSTs can overshoot the 20/hour cap on a single process, and a multi-worker deployment multiplies that cap by worker count. If this route is meant to curb abuse, the counter needs shared storage (for example Redis/DB), or at minimum a per-process lock.
- Removed the legacy journal file for the test database. - Updated the `create_or_get_specialty` function to delegate to `get_or_create`, improving concurrency handling and response consistency. - Refactored the `get_or_create` method in the CRUD layer to return a tuple indicating whether a new specialty was created. - Introduced shared validation functions for website URLs and ratings in the practitioner schema, streamlining validation logic. - Updated frontend components to reflect changes in specialty creation messaging and improved user feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/schemas/practitioner.py (1)
64-72: TLD length restriction may reject valid domains.The regex restricts TLDs to 2-6 characters (
[A-Z]{2,6}), but some valid TLDs are longer (e.g.,.photographyis 11 chars). Consider increasing to{2,20}or similar to accommodate newer gTLDs.♻️ Suggested fix
_WEBSITE_URL_PATTERN = re.compile( r"^https?://" # http:// or https:// - r"(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+[A-Z]{2,6}\.?|" # domain... + r"(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+[A-Z]{2,63}\.?|" # domain... r"localhost|" # localhost... r"\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})" # ...or ip r"(?::\d+)?" # optional port r"(?:/?|[/?]\S+)$", re.IGNORECASE, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/schemas/practitioner.py` around lines 64 - 72, The _WEBSITE_URL_PATTERN currently limits TLD length with [A-Z]{2,6}, which will reject valid long gTLDs; update that character class quantifier in the regex used to compile _WEBSITE_URL_PATTERN (change {2,6} to a larger range such as {2,20}) and adjust the inline comment if present, then run tests/validation to ensure URLs like .photography are accepted; reference _WEBSITE_URL_PATTERN to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/schemas/practitioner.py`:
- Around line 64-72: The _WEBSITE_URL_PATTERN currently limits TLD length with
[A-Z]{2,6}, which will reject valid long gTLDs; update that character class
quantifier in the regex used to compile _WEBSITE_URL_PATTERN (change {2,6} to a
larger range such as {2,20}) and adjust the inline comment if present, then run
tests/validation to ensure URLs like .photography are accepted; reference
_WEBSITE_URL_PATTERN to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74962e93-6373-4b88-9725-505619ca518d
📒 Files selected for processing (13)
app/api/v1/endpoints/medical_specialty.pyapp/crud/medical_specialty.pyapp/crud/treatment.pyapp/schemas/practitioner.pyfrontend/public/locales/en/admin.jsonfrontend/src/components/admin/SpecialtySelect.jsxfrontend/src/components/medical/BaseMedicalForm.jsxfrontend/src/services/api/specialtyApi.jstests/api/test_medical_specialty_public.pytests/api/test_practitioners.pytests/conftest.pytests/crud/test_medical_specialty.pytests/crud/test_practitioner.py
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/medical/BaseMedicalForm.jsx
- tests/api/test_medical_specialty_public.py
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/components/admin/SpecialtySelect.jsx
- frontend/public/locales/en/admin.json
- app/crud/medical_specialty.py
- frontend/src/services/api/specialtyApi.js
- tests/conftest.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
2 participants
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
This pull request finalizes the migration of practitioner specialties from a legacy free-text string column to a normalized foreign key and lookup table model. It also introduces public API endpoints for reading and creating medical specialties, seeds the canonical list of specialties into the database, and updates practitioner filtering and relationships to use the new structure. Legacy endpoints and fields are removed or updated for consistency.
Migration and Data Model Changes:
practitioners.specialtystring column, enforcing use of thespecialty_idforeign key and theMedicalSpecialtylookup table. The migration is safety-checked and reversible. (alembic/migrations/versions/20260422_0900_a3b4c5d6e7f8_drop_practitioner_specialty_column.py)medical_specialtiestable with a canonical, case-insensitive, idempotent list of specialties, ensuring the dropdown is fully populated for users. Downgrade removes only unreferenced specialties. (alembic/migrations/versions/20260422_1000_b4c5d6e7f8a9_seed_default_medical_specialties.py)API and Endpoint Updates:
/medical-specialtiesAPI endpoint for listing and creating specialties, with rate limiting for non-admin users. The full CRUD surface remains admin-only. (app/api/v1/endpoints/medical_specialty.py,app/api/v1/api.py) [1] [2] [3]/practitioners/specialtiesendpoint and updates practitioner list/read endpoints to filter and join byspecialty_idandspecialty_relinstead of the old string field. (app/api/v1/endpoints/practitioner.py) [1] [2] [3]Codebase Consistency and Cleanup:
app/crud/medical_specialty.py)specialtysearch field from the practitioner admin model and cleans up unused imports. (app/api/v1/admin/models.py,app/crud/practitioner.py) [1] [2]These changes complete the transition to a normalized, maintainable, and user-friendly handling of medical specialties throughout the application.
Related issue
#813
Type of change
Testing
Checklist
npm run lintandnpm run buildpasspytest) if backend code changedconsole.logleft in the difft()translationsSummary by CodeRabbit
New Features
Behavior Changes
Documentation