[ENG-737] Questionnaire Redesign - #3719
Draft
vigneshhari wants to merge 7 commits into
Draft
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| from care.utils.lock import Lock | ||
|
|
||
|
|
||
| class QuestionnaireLock(Lock): |
| from .template import * # noqa | ||
| from .token import * # noqa | ||
| from .user import * # noqa | ||
| from .valueset import * # noqa |
| from .device import * # noqa F403 | ||
| from .diagnostic_report import * # noqa F403 | ||
| from .encounter import * # noqa F403 | ||
| from .facility_resource import * # noqa F403 |
Comment on lines
+169
to
+175
| # def validate_slug(self, info): | ||
| # # Uniqueness changes based on the auth context | ||
| # if self.auth_context == ValueSetAuthContext.instance: | ||
| # queryset = ValuesetDatabaseModel.objects.filter(slug=self.slug) | ||
| # elif self.auth_context == ValueSetAuthContext.facility: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # facility__external_id=self.facility |
Comment on lines
+177
to
+179
| # elif self.auth_context == ValueSetAuthContext.facility_organization: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # facility_organization__organization__external_id=self.facility_organization |
Comment on lines
+181
to
+183
| # elif self.auth_context == ValueSetAuthContext.user: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # created_by=self.get_serializer_context(info)["user"] |
Comment on lines
+185
to
+189
| # else: | ||
| # raise ValueError("Invalid auth context") | ||
| # if queryset.exists(): | ||
| # err = "Slug must be unique" | ||
| # raise ValueError(err) |
Comment on lines
+353
to
+361
| # def validate_slug(self, info): | ||
| # # Uniqueness changes based on the auth context | ||
| # if self.auth_context == QuestionnaireAuthContext.instance: | ||
| # queryset = Questionnaire.objects.filter(slug=self.slug) | ||
| # elif self.auth_context == QuestionnaireAuthContext.facility: | ||
| # queryset = Questionnaire.objects.filter(facility__external_id=self.facility) | ||
| # elif self.auth_context == QuestionnaireAuthContext.facility_organization: | ||
| # queryset = Questionnaire.objects.filter( | ||
| # facility_organization__organization__external_id=self.facility_organization |
Comment on lines
+363
to
+365
| # elif self.auth_context == QuestionnaireAuthContext.user: | ||
| # queryset = Questionnaire.objects.filter( | ||
| # created_by=self.get_serializer_context(info)["user"] |
Comment on lines
+367
to
+371
| # else: | ||
| # raise ValueError("Invalid auth context") | ||
| # if queryset.exists(): | ||
| # err = "Slug must be unique" | ||
| # raise ValueError(err) |
There was a problem hiding this comment.
Pull request overview
This PR implements a major redesign of questionnaires/value sets in CARE’s Django EMR backend by introducing multi-scope authorization contexts (instance/facility/facility-organization/user), questionnaire revisioning, and new “resource questionnaire” APIs with dedicated response/observation storage and cleaned-response projections.
Changes:
- Added auth-context scoping + authorization plumbing for questionnaires and value sets (permissions + access filtering).
- Introduced questionnaire revisioning (
internal_revision,latest_revision,questions_hash) and “resolved questionnaire” rendering for historical responses. - Added resource-level questionnaire submission/read endpoints plus new models/resources for resource questionnaire responses and observations, including
cleaned_response.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| config/api_router.py | Updates questionnaire import path and registers new resource endpoints. |
| care/security/permissions/valueset.py | Adds ValueSet permission definitions. |
| care/security/permissions/questionnaire.py | Adds permission for viewing resource responses. |
| care/security/permissions/base.py | Registers ValueSet permissions in the permission controller. |
| care/security/authorization/valueset.py | Adds ValueSet access checks + queryset filtering. |
| care/security/authorization/questionnaire.py | Extends questionnaire access checks + queryset filtering for new scopes. |
| care/security/authorization/facility.py | Adds facility-level questionnaire submit/read authorization helpers. |
| care/security/authorization/facility_location.py | Adds location-level questionnaire submit/read authorization helpers. |
| care/security/authorization/device.py | Adds device-level questionnaire submit/read authorization helpers. |
| care/security/authorization/init.py | Exposes new valueset authorization controller. |
| care/emr/tests/test_questionnaire_api.py | Extends assertions around cleaned_response. |
| care/emr/resources/valueset/spec.py | Adds auth-context aware create/update/read specs and inheritance rules. |
| care/emr/resources/questionnaire/utils.py | Adds cleaned_response builder + resource submission handler + valueset validation changes. |
| care/emr/resources/questionnaire/spec.py | Adds auth-context + subject extensions, valueset config, and revision-aware creation/update specs. |
| care/emr/resources/questionnaire_response/spec.py | Adds cleaned_response and revision-aware questionnaire serialization. |
| care/emr/resources/questionnaire_response/resource_spce.py | Adds resource questionnaire response request/read/update specs. |
| care/emr/resources/observation/resource_spec.py | Adds resource observation specs for new resource observation model. |
| care/emr/reports/context_builder/data_points/questionnaire.py | Uses resolved_questionnaire in report context. |
| care/emr/models/valueset.py | Adds scoped valuesets (facility/user/org), inheritance, and org-cache support. |
| care/emr/models/questionnaire.py | Adds questionnaire revisioning, scoped fields, resolved rendering, and cleaned_response storage. |
| care/emr/models/facility_resource.py | Introduces resource questionnaire response + observation persistence models. |
| care/emr/models/init.py | Exports new facility_resource models. |
| care/emr/migrations/0085_alter_facilityresourceobservation_questionnaire_response.py | Fixes FK to point to the new resource questionnaire response model. |
| care/emr/migrations/0084_facilityresourceobservation_and_more.py | Creates new resource response/observation tables. |
| care/emr/migrations/0083_remove_questionnaire_unique_questionnaire_slug_user_and_more.py | Adjusts scoped uniqueness constraints. |
| care/emr/migrations/0082_userfacilityvaluesetpreference_and_more.py | Adds scoped valueset fields + constraints + preferences. |
| care/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.py | Adds questionnaire scoped fields + revisioning and backfills questions hash. |
| care/emr/locks/questionnaire.py | Adds a lock to serialize questionnaire updates. |
| care/emr/api/viewsets/valueset.py | Reworks valueset API for scoped CRUD/auth, org assignment, and slug expansion/preferences. |
| care/emr/api/viewsets/questionnaire/resource_questionnaire_response.py | Adds read/update APIs for resource questionnaire responses. |
| care/emr/api/viewsets/questionnaire/resource_observation.py | Adds read/analyse APIs for resource observations. |
| care/emr/api/viewsets/questionnaire/resource_authz.py | Adds shared auth helpers for resource questionnaire access. |
| care/emr/api/viewsets/questionnaire/questionnaire.py | Replaces questionnaire API with scoped CRUD, revisioning, locking, and resource submit flow. |
| care/emr/api/viewsets/questionnaire/init.py | Package marker for split questionnaire viewsets. |
| care/emr/api/viewsets/questionnaire.py | Removes old monolithic questionnaire viewset implementation. |
| care/emr/api/viewsets/questionnaire_response.py | Updates filtering to include revisions and syncs observation status on “entered_in_error”. |
Comments suppressed due to low confidence (4)
care/emr/api/viewsets/valueset.py:126
- This is in
authorize_update, but the error message says "create". Updating the wording avoids confusing clients when update requests are rejected.
None,
read_only=False,
)
):
raise PermissionDenied("You are not authorized to create a value set")
if (
care/emr/api/viewsets/valueset.py:136
- This is in
authorize_update, but the error message says "create". Updating the wording avoids confusing clients when update requests are rejected.
model_instance.facility_organization,
read_only=False,
)
):
raise PermissionDenied("You are not authorized to create a value set")
if (
care/emr/api/viewsets/valueset.py:146
- This is in
authorize_update, but the error message says "create". Updating the wording avoids confusing clients when update requests are rejected.
model_instance.facility,
read_only=False,
)
):
raise PermissionDenied("You are not authorized to create a value set")
if (
care/emr/api/viewsets/valueset.py:199
- This permission error references "questionnaires" in a ValueSet endpoint, which is confusing for API consumers.
if not valueset.auth_context == ValueSetAuthContext.facility:
raise PermissionDenied(
"Facility organizations can only be set for facility level questionnaires"
)
| id: UUID4 | UUID5 = Field( | ||
| description="Unique machine provided UUID", default_factory=uuid.uuid4 | ||
| ) | ||
| id: UUID4 | UUID5 = Field(description="Unique machine provided UUID") |
| class QuestionnaireSpec(QuestionnaireWriteSpec): | ||
| organizations: list[UUID4] = Field(min_length=1) | ||
| class QuestionnaireCreateSpec(QuestionnaireWriteSpec): | ||
| auth_context: QuestionnaireAuthContext |
Comment on lines
+214
to
+217
| """ | ||
| - Guard questionnaire submit so that other facilities cannot submit their forms | ||
| - Ensure Questionnaire valuesets are from the same facility or instance | ||
| """ |
Comment on lines
+82
to
+104
| def get_queryset(self): | ||
| queryset = ( | ||
| super() | ||
| .get_queryset() | ||
| .order_by("-created_date") | ||
| .select_related("questionnaire") | ||
| ) | ||
|
|
||
| if self.action in ["list", "retrieve"]: | ||
| subject_id = self.request.GET.get("subject_id") | ||
| subject_type = self.request.GET.get("subject_type") | ||
| if not subject_id or not subject_type: | ||
| raise ValidationError("subject_id and subject_type are required") | ||
| subject = get_questionniare_resource(subject_type, subject_id) | ||
| authorize_resource_questionnaire_response_read( | ||
| subject_type, subject, self.request.user | ||
| ) | ||
| queryset = queryset.filter(subject_type=subject_type, subject_id=subject_id) | ||
|
|
||
| if "questionnaire_slugs" in self.request.GET: | ||
| questionnaire_slugs = self.request.GET.get("questionnaire_slugs").split(",") | ||
| queryset = queryset.filter(questionnaire__slug__in=questionnaire_slugs) | ||
| return queryset |
Comment on lines
+57
to
+69
| def get_queryset(self): | ||
| queryset = super().get_queryset() | ||
| subject_id = self.request.GET.get("subject_id") | ||
| subject_type = self.request.GET.get("subject_type") | ||
| if not subject_id or not subject_type: | ||
| raise ValidationError("subject_id and subject_type are required") | ||
| subject = get_questionniare_resource(subject_type, subject_id) | ||
| authorize_resource_questionnaire_response_read( | ||
| subject_type, subject, self.request.user | ||
| ) | ||
| queryset = queryset.filter(subject_type=subject_type, subject_id=subject_id) | ||
|
|
||
| return queryset.order_by("-modified_date") |
| model_instance.auth_context == ValueSetAuthContext.instance | ||
| and not self.request.user.is_superuser | ||
| ): | ||
| raise PermissionDenied("You are not authorized to create a value set") |
Comment on lines
+171
to
+173
| raise PermissionDenied( | ||
| "Facility organizations can only be set for facility level questionnaires" | ||
| ) |
Comment on lines
+75
to
+82
| class QuestionnaireViewSet(EMRModelViewSet, EMRFavoritesMixin): | ||
| database_model = Questionnaire | ||
| pydantic_model = QuestionnaireCreateSpec | ||
| pydantic_read_model = QuestionnaireReadSpec | ||
| pydantic_update_model = QuestionnaireUpdateSpec | ||
| filterset_class = QuestionnaireFilter | ||
| filter_backends = [filters.DjangoFilterBackend, FavoritesFilter] | ||
| FAVORITE_RESOURCE = FavoriteResourceChoices.questionnaire.value |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (22)
care/emr/api/viewsets/valueset.py (6)
274-274: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnnecessary related fetch.
preference.valueset.idloads the whole related row to read a PK you already have aspreference.valueset_id.♻️ Proposed change
- preferred_valueset = valuesets.filter(id=preference.valueset.id).first() + preferred_valueset = valuesets.filter(id=preference.valueset_id).first()🤖 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 `@care/emr/api/viewsets/valueset.py` at line 274, Update the valueset lookup in the preference handling flow to filter using preference.valueset_id instead of preference.valueset.id, avoiding the unnecessary related-object fetch while preserving the existing first-match behavior.
216-220: 🩺 Stability & Availability | 🔵 TrivialCache-key format change drops existing recent views.
Keying by UUID instead of slug is the right call now that slugs aren't globally unique. Just be aware favourites re-populate from the DB, while
recent_viewslive only in cache and will quietly reset for every user on deploy. Fine if intended — maybe just worth a release note.🤖 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 `@care/emr/api/viewsets/valueset.py` around lines 216 - 220, Update the release documentation for the cache-key change in get_recent_view_cache_key and get_favourites_cache_key, explicitly noting that switching recent_views keys from slug to valueset UUID invalidates existing cached recent views and resets them after deployment. Do not alter the UUID-based key format or database-backed favourites behavior.
170-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated facility-context guard in both organization endpoints. The same copy-pasted block (
not x == yplus a message about "questionnaires") appears twice; extracting a small helper fixes both and keeps them honest.
care/emr/api/viewsets/valueset.py#L170-L174: replace the inline guard with a shared helper that checksauth_context != ValueSetAuthContext.facilityand callsauthorize_update.care/emr/api/viewsets/valueset.py#L196-L199: call the same helper instead of repeating the guard and the questionnaire-flavoured message.🤖 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 `@care/emr/api/viewsets/valueset.py` around lines 170 - 174, Extract the duplicated facility-context validation and authorization from the organization endpoints into a shared helper, using auth_context != ValueSetAuthContext.facility and authorize_update. Replace the inline guard at care/emr/api/viewsets/valueset.py:170-174 and the repeated guard at care/emr/api/viewsets/valueset.py:196-199 with calls to that helper, preserving the PermissionDenied behavior without the questionnaire-specific message.
291-296: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelection priority relies on the alphabetical ordering of
auth_contextstrings.
order_by("auth_context")happens to putfacilitybeforeinstance, so the fallback picks the facility-scoped valueset — correct today, purely by accident of spelling. Renaming or adding a context silently changes precedence. An explicitCase/Whenordering (or a priority map) documents the intent. Thefor ... breakis also just.first().♻️ Proposed change
- if not valueset: - for valueset_option in valuesets.order_by("auth_context"): - valueset = valueset_option - break + if not valueset: + valueset = valuesets.order_by( + Case( + When(auth_context=ValueSetAuthContext.facility, then=0), + When(auth_context=ValueSetAuthContext.facility_organization, then=1), + When(auth_context=ValueSetAuthContext.instance, then=2), + default=3, + output_field=IntegerField(), + ) + ).first() if not valueset: raise ValidationError("No valueset found")Requires
from django.db.models import Case, IntegerField, When.🤖 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 `@care/emr/api/viewsets/valueset.py` around lines 291 - 296, Update the fallback selection around the valueset lookup to encode the intended auth_context precedence explicitly with a Django Case/When ordering, prioritizing facility over instance rather than relying on alphabetical order. Replace the order-and-break loop with .first() on that prioritized queryset, while preserving the existing “No valueset found” ValidationError.
201-213: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-organization queries inside the transaction; a bulk fetch/create would be kinder.
Each entry triggers a
get_object_or_404plus an individualINSERT, all while holding the transaction open. Fetching the whole set once andbulk_create-ing keeps the lock window short and still validates that every id belongs tovalueset.facility.♻️ Proposed refactor
request_params = self.ValueSetFacilityOrganizationUpdateSchema(**request.data) + requested_ids = set(request_params.facility_organizations) with transaction.atomic(): ValueSetFacilityOrganization.objects.filter(valueset=valueset).delete() - for org in request_params.facility_organizations: - organization = get_object_or_404( - FacilityOrganization.objects.only("id"), - external_id=org, - facility=valueset.facility, - ) - ValueSetFacilityOrganization.objects.create( - valueset=valueset, organization=organization - ) + organizations = FacilityOrganization.objects.filter( + external_id__in=requested_ids, facility=valueset.facility + ).only("id", "external_id") + if len(organizations) != len(requested_ids): + raise ValidationError("Invalid facility organization") + ValueSetFacilityOrganization.objects.bulk_create( + ValueSetFacilityOrganization(valueset=valueset, organization=org) + for org in organizations + ) valueset.sync_facility_org_cache()🤖 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 `@care/emr/api/viewsets/valueset.py` around lines 201 - 213, Refactor the update flow around ValueSetFacilityOrganizationUpdateSchema and ValueSetFacilityOrganization creation to fetch all requested FacilityOrganization records for valueset.facility in one query, validate that every requested external_id was found, and bulk_create the corresponding ValueSetFacilityOrganization rows. Preserve deletion, transaction atomicity, 404 behavior for invalid organizations, and the final valueset.sync_facility_org_cache() call.
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the authorization hook to fix the typo
can_access_user_valueset_in_faciltiyis misspelled in both places, so it still lines up, but the contract is going to stay embarrassing until someone renames it to...facility.🤖 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 `@care/emr/api/viewsets/valueset.py` around lines 97 - 105, Rename the authorization hook from can_access_user_valueset_in_faciltiy to can_access_user_valueset_in_facility everywhere it is defined or invoked, including this AuthorizationController.call usage, while preserving its existing arguments and behavior.care/emr/resources/valueset/spec.py (4)
108-138: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueExistence checks duplicate the lookups done during deserialization; annotations say non-optional.
Each of these validators runs an
EXISTSquery, andperform_extra_deserializationimmediately fetches the same rows again viaget_object_or_404. Since the 404 path already produces a reasonable error, these are largely redundant. Also, the parameters are annotatedUUID4whileNoneis clearly expected —UUID4 | Nonewould be honest.🤖 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 `@care/emr/resources/valueset/spec.py` around lines 108 - 138, Remove the redundant database existence checks and “not found” errors from validate_parent, validate_facility, and validate_facility_organization, allowing perform_extra_deserialization to handle missing records. Update each validator parameter annotation to UUID4 | None while preserving the existing return behavior.
206-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
mapping["id"]assignment.
ValueSetReadSpecrepeats the parent's line instead of delegating; the two will drift the moment the minimal spec's serialization grows.♻️ Proposed change
`@classmethod` def perform_extra_serialization(cls, mapping, obj): - mapping["id"] = obj.external_id + super().perform_extra_serialization(mapping, obj) cls.serialize_audit_users(mapping, obj)🤖 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 `@care/emr/resources/valueset/spec.py` around lines 206 - 221, Remove the duplicated mapping["id"] assignment from ValueSetReadSpec.perform_extra_serialization and delegate the inherited serialization to ValueSetMinimalReadSpec before serializing audit users. Preserve the existing audit-user serialization behavior while ensuring future minimal-spec changes are reused.
140-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
validate_unique_iddoesn't validate a unique id.It enforces required scope fields per auth context.
validate_required_scope(or similar) would spare the next reader a puzzled moment. The first two branches also collapse neatly.♻️ Proposed change
`@model_validator`(mode="after") - def validate_unique_id(self): - if self.auth_context == ValueSetAuthContext.user and not self.facility: - raise ValueError("Facility is required") - if self.auth_context == ValueSetAuthContext.facility and not self.facility: + def validate_required_scope(self): + if ( + self.auth_context + in (ValueSetAuthContext.user, ValueSetAuthContext.facility) + and not self.facility + ): raise ValueError("Facility is required")As per coding guidelines, "Use descriptive variable and function names".
🤖 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 `@care/emr/resources/valueset/spec.py` around lines 140 - 151, Rename the model validator validate_unique_id to a descriptive name such as validate_required_scope that reflects its auth-context field validation. Combine the user and facility branches into one condition requiring facility, while preserving the existing facility_organization validation and error messages.Source: Coding guidelines
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ValueSetAuthContext.instancein both branches (care/emr/resources/questionnaire/utils.py:31-43). Keeping one branch on the literal while the other uses the enum is just extra maintenance for no gain.🤖 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 `@care/emr/resources/valueset/spec.py` around lines 16 - 20, Update both branches in the relevant conditional within the questionnaire utility to use the existing ValueSetAuthContext.instance enum member, replacing the remaining literal value while preserving the current branch behavior.care/emr/resources/questionnaire/utils.py (2)
30-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo branches, two ways of spelling
instance.Line 33 uses
ValueSetAuthContext.instance; line 42 hardcodes"instance". They agree today, which is the only reason this isn't a bug yet.♻️ Use the enum consistently
valueset = ValueSet.objects.filter( slug=valueset_config.get("slug"), - auth_context="instance", + auth_context=ValueSetAuthContext.instance, ).first()🤖 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 `@care/emr/resources/questionnaire/utils.py` around lines 30 - 47, Update the fallback ValueSet query in validate_questionnaire_valueset to use ValueSetAuthContext.instance for auth_context, matching the string-config branch instead of hardcoding "instance"; leave the other lookup and validation behavior unchanged.
750-827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
handle_resource_responseishandle_responsewith the patient/encounter parts deleted.Lines 761-790 are character-for-character identical to lines 680-709. Any future fix to validation ordering, error shaping, or
collect_and_validate_enable_when_questionsnow has to be applied twice, and one of the two will inevitably be forgotten.♻️ Extract the shared validation step
+def _validate_and_build_observations(questionnaire_obj, results): + if questionnaire_obj.status != "active": + raise ValidationError( + {"type": "questionnaire_inactive", "msg": "Questionnaire is inactive"} + ) + responses = create_responses_mapping(results.results) + if not responses: + raise ValidationError( + { + "type": "questionnaire_empty", + "msg": "Empty Questionnaire cannot be submitted", + } + ) + errors = [] + questionnaire_mapping = {} + questionnaire_obj.questions = collect_and_validate_enable_when_questions( + questionnaire_obj.questions, responses, questionnaire_obj, errors + ) + for question in questionnaire_obj.questions: + validate_question_result( + question, + responses, + errors, + parent=None, + questionnaire_mapping=questionnaire_mapping, + ) + if errors: + raise ValidationError({"errors": errors}) + observations = convert_to_observation_spec( + {"questions": questionnaire_obj.questions}, responses + ) + return responses, observationsAlso note the
bulkloop at lines 820-823 is just a list comprehension in disguise.🤖 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 `@care/emr/resources/questionnaire/utils.py` around lines 750 - 827, Refactor the duplicated validation flow in handle_resource_response and handle_response into a shared helper that performs response mapping, empty-response validation, enableWhen processing, question validation, and error raising; have both callers reuse it while preserving their existing resource-specific handling. Also replace the bulk construction loop in handle_resource_response with an equivalent list comprehension.care/emr/migrations/0084_facilityresourceobservation_and_more.py (1)
56-82: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an index on
(subject_type, subject_id).
FacilityResourceQuestionnaireResponselist/retrieve filters on exactly these two columns (seeresource_questionnaire_response.pyget_queryset), andFacilityResourceObservationwill be queried the same way. A composite index now saves a follow-up migration later.🤖 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 `@care/emr/migrations/0084_facilityresourceobservation_and_more.py` around lines 56 - 82, Add a composite database index covering subject_type and subject_id to the FacilityResourceQuestionnaireResponse model definition in migration 0084, and apply the same index to FacilityResourceObservation if it is defined in this migration. Define the indexes through each model’s options so queries using both filters are optimized.care/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.py (1)
10-18: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBackfill loads every questionnaire into memory and saves one row at a time.
Fine on a small table, less fine on a production one.
iterator()+bulk_updatewould keep this from being a long single transaction of N UPDATEs.♻️ Suggested batched backfill
def populate_questionnaire_hash(apps, schema_editor): Questionnaire = apps.get_model('emr', 'Questionnaire') - for questionnaire in Questionnaire.objects.all(): - current_obj_questions = json.dumps(questionnaire.questions, sort_keys=True).encode( - "utf-8" - ) - current_obj_hash = hashlib.sha256(current_obj_questions).hexdigest() - questionnaire.questions_hash = current_obj_hash - questionnaire.save(update_fields=["questions_hash"]) + batch = [] + for questionnaire in Questionnaire.objects.only("id", "questions").iterator( + chunk_size=1000 + ): + payload = json.dumps(questionnaire.questions, sort_keys=True).encode("utf-8") + questionnaire.questions_hash = hashlib.sha256(payload).hexdigest() + batch.append(questionnaire) + if len(batch) >= 1000: + Questionnaire.objects.bulk_update(batch, ["questions_hash"]) + batch = [] + if batch: + Questionnaire.objects.bulk_update(batch, ["questions_hash"])🤖 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 `@care/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.py` around lines 10 - 18, Update populate_questionnaire_hash to stream questionnaires with QuerySet.iterator() instead of loading all rows, compute each questions_hash as before, and accumulate records for batched bulk_update calls. Flush batches at a bounded size and handle any final partial batch, updating only the questions_hash field.care/emr/models/questionnaire.py (2)
212-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray module-level string used as a TODO list.
This evaluates as a no-op expression at import time and won't show up in any issue tracker, which rather defeats the purpose. Want me to open an issue for the two guards ("guard questionnaire submit across facilities", "valueset/facility alignment") and drop this block?
🤖 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 `@care/emr/models/questionnaire.py` around lines 212 - 217, Remove the stray module-level TODO string from the questionnaire module, including both guard notes; track these follow-up items through the project’s issue workflow instead of leaving them as a no-op expression.
98-121: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThree copies of the same org-cache sync loop, all missing
select_related("organization"). Each iteration lazy-loadsorganizationto readparent_cache, so syncing N mappings costs N+1 queries; the logic is otherwise identical across all three methods and only differs by join model and target field.
care/emr/models/questionnaire.py#L98-L121: add.select_related("organization")to bothsync_facility_org_cacheandsync_org_cachequerysets.care/emr/models/valueset.py#L97-L107: add.select_related("organization")to theValueSetFacilityOrganizationqueryset insync_facility_org_cache.A shared helper (e.g.
build_org_cache(queryset)) taking the join queryset and returning the deduped id list would collapse all three into one implementation.🤖 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 `@care/emr/models/questionnaire.py` around lines 98 - 121, Update Questionnaire.sync_facility_org_cache and Questionnaire.sync_org_cache in care/emr/models/questionnaire.py (lines 98-121) to add select_related("organization") to both mapping querysets, and update ValueSet.sync_facility_org_cache in care/emr/models/valueset.py (lines 97-107) likewise for the ValueSetFacilityOrganization queryset. Preserve the existing cache targets and deduplication; optionally consolidate the identical loop through a shared helper.care/emr/resources/questionnaire/spec.py (2)
289-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFacility is fetched twice, and the hash logic is copy-pasted into the update spec.
validate_facilityalready ran anexists()query;perform_extra_deserializationthen doesget_object_or_404for the same row. Same story forfacility_organization. Thequestions_hashcomputation is also duplicated verbatim at lines 413-416.♻️ Extract a module-level hash helper
+def compute_questions_hash(questions) -> str: + payload = json.dumps(questions, sort_keys=True).encode("utf-8") + return hashlib.sha256(payload).hexdigest() + + class QuestionnaireCreateSpec(QuestionnaireWriteSpec): @@ - current_obj_questions = json.dumps(obj.questions, sort_keys=True).encode( - "utf-8" - ) - obj.questions_hash = hashlib.sha256(current_obj_questions).hexdigest() + obj.questions_hash = compute_questions_hash(obj.questions)The migration at
care/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.pyhas a third copy, so a shared helper keeps the three from drifting.🤖 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 `@care/emr/resources/questionnaire/spec.py` around lines 289 - 309, Reuse the facility and facility-organization objects resolved during validation in perform_extra_deserialization instead of issuing duplicate get_object_or_404 queries after validate_facility. Extract the repeated questions hash calculation into a module-level helper, then use that helper here and in the update spec and migration so all three paths share the same implementation.
352-373: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead
QuestionnaireSpecstub and commented validators. Nothing referencesQuestionnaireSpec, so delete the empty class together with the commented slug validators; this file is cleaner without the little museum exhibit.🤖 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 `@care/emr/resources/questionnaire/spec.py` around lines 352 - 373, Remove the unused QuestionnaireSpec class stub and its commented validate_slug model validator block. Delete only this dead code, leaving the surrounding questionnaire schema definitions unchanged.care/emr/tests/test_questionnaire_api.py (1)
237-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly 5 of the 12 question types get a
cleaned_responseassertion.The date/dateTime/time/url/text types all fall through the pass-through branch of
_coerce_cleaned_value, which is exactly the branch nobody would notice breaking. A couple more asserts here would lock in the projection contract.As per coding guidelines, use Django's built-in testing tools to ensure code quality and reliability.
🤖 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 `@care/emr/tests/test_questionnaire_api.py` around lines 237 - 243, Extend the assertions in the test covering cleaned_response to include representative date, dateTime, time, url, and text question values. Assert each projected value matches its original expected representation, using Django’s built-in test assertions and the existing cleaned_response fixture/setup.Source: Coding guidelines
care/emr/api/viewsets/questionnaire/resource_authz.py (1)
88-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo in new function name:
get_questionniare_resource.Missing an "n" — should be
get_questionnaire_resource. Worth fixing now since this is a brand-new function with only a couple of call sites in this same PR. As per coding guidelines, "Use descriptive variable and function names."🤖 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 `@care/emr/api/viewsets/questionnaire/resource_authz.py` around lines 88 - 96, The new function is misspelled as get_questionniare_resource; rename it to get_questionnaire_resource and update all call sites introduced by this change to use the corrected name.Source: Coding guidelines
care/emr/api/viewsets/questionnaire/resource_questionnaire_response.py (1)
82-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
retrieveforces the client to already knowsubject_id/subject_typeand match them exactly.Unlike the sibling
QuestionnaireResponseViewSet.get_queryset, which resolvespatient/encounterfrom the fetched object itself forretrieve, this always requiressubject_id/subject_typequery params — even for a single-object GET by primary key. A client with a valid response ID but a mismatched/missing subject param gets a 404 despite the record existing.🤖 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 `@care/emr/api/viewsets/questionnaire/resource_questionnaire_response.py` around lines 82 - 99, The get_queryset logic in the resource QuestionnaireResponse viewset incorrectly applies subject query parameters to retrieve requests. Preserve the required subject_id/subject_type validation, resource lookup, authorization, and filtering for list, but for retrieve resolve the target response by its existing lookup and derive its subject information from that object before authorizing, without requiring or filtering by query parameters.care/security/authorization/questionnaire.py (1)
48-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the duplicated
faciltiymethods tofacility.
care/security/authorization/questionnaire.py#L48-L60:can_access_user_questionnaire_in_facilitycare/security/authorization/valueset.py#L46-L58:can_access_user_valueset_in_facility🤖 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 `@care/security/authorization/questionnaire.py` around lines 48 - 60, Rename the misspelled method can_access_user_questionnaire_in_faciltiy to can_access_user_questionnaire_in_facility in care/security/authorization/questionnaire.py:48-60, and rename the corresponding can_access_user_valueset_in_faciltiy method to can_access_user_valueset_in_facility in care/security/authorization/valueset.py:46-58. Update all call sites to use the corrected names.Source: Coding guidelines
🤖 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.
Inline comments:
In `@care/emr/api/viewsets/questionnaire_response.py`:
- Around line 24-36: Update QuestionnaireFilter.filter to match responses
through the latest revision in the forward direction: guard the nullable
latest_revision_id relation, then filter using questionnaire_id equal to
questionnaire.latest_revision_id alongside the direct questionnaire match. Apply
the same correction to the equivalent predicate referenced in the additional
lines.
- Around line 94-96: Update the observation status propagation around the
`Observation.objects.filter(...).update(...)` call to preserve audit metadata,
especially `updated_by`. Use the model-level transition or audit-aware update
path provided by `EMRUpdateMixin`, or explicitly set all required audit fields
within the same transaction, rather than bypassing `Observation.save()` and
signals with a bare queryset update.
- Around line 87-93: Lock and revalidate the QuestionnaireResponse row before
saving it to prevent a stale authorized instance from reopening an
entered-in-error response. Update the authorize_update/update flow around
transaction.atomic and old_obj to acquire the row lock with select_for_update
before authorization, or recheck the locked status immediately before invoking
the superclass update. Preserve the existing status-transition validation for
the locked record.
In `@care/emr/api/viewsets/questionnaire/questionnaire.py`:
- Around line 226-239: Replace the duplicated subject-type resolution block in
the questionnaire view with the existing get_questionniare_resource helper from
resource_authz.py. Add that helper to the existing import and pass the request
resource identifier and questionnaire subject type as required, preserving the
helper’s existing validation and 404 behavior.
In `@care/emr/api/viewsets/questionnaire/resource_observation.py`:
- Line 47: Update the page_size Field declaration to enforce a non-negative
lower bound in addition to the existing maximum of 30, preventing invalid
negative queryset slices while preserving the current upper limit.
In `@care/emr/migrations/0082_userfacilityvaluesetpreference_and_more.py`:
- Around line 82-113: Add a RunPython data-cleanup operation before the
Questionnaire and ValueSet AddConstraint operations in migration 0082. Using the
historical models, identify duplicate non-deleted rows with
auth_context='instance', retain one row per slug, and update or remove the
extras consistently for both models so the unique_questionnaire_slug_instance
and unique_valueset_slug_instance constraints can be applied successfully.
In `@care/emr/models/facility_resource.py`:
- Line 15: Update the structured_response_type model field to specify an
appropriate max_length for the expected value; if the value is unbounded,
replace the CharField with TextField. Preserve its existing default, blank, and
null behavior.
In `@care/emr/models/questionnaire.py`:
- Around line 153-162: Update resolved_questionnaire to cache its resolved value
and gracefully return None when the requested historical revision is missing,
avoiding repeated per-row database lookups in serializers and context building.
Add a database uniqueness constraint for the Questionnaire pair
latest_revision_id and internal_revision so the lookup is canonical.
In `@care/emr/models/valueset.py`:
- Around line 56-58: The create_composition parent-traversal logic must guard
against cyclic ValueSet parent links to prevent unbounded recursion during
search and lookup. Track visited ValueSetSpec nodes (or equivalent stable
identifiers) while following parents and stop expansion when a node repeats;
also validate self/ancestor assignments in
ValueSetSpec.perform_extra_deserialization where feasible.
In `@care/emr/resources/observation/resource_spec.py`:
- Around line 43-47: Update the Observation resource model fields
reference_range, interpretation, component, created_by, and updated_by to use
Pydantic Field(default_factory=...) instead of shared mutable defaults,
preserving each field’s existing collection or value type. Import Field from
Pydantic and leave unrelated defaults unchanged.
- Around line 37-41: Update BaseResourceObservationSpec so effective_datetime
accepts None, matching nullable FacilityResourceObservation values and
preventing serialization failures in ResourceObservationReadSpec and
ResourceObservationRetrieveSpec for existing rows without timestamps.
In `@care/emr/resources/questionnaire_response/resource_spce.py`:
- Around line 51-52: Update the `created_by` and `updated_by` default values in
the relevant resource specification to `None` instead of `dict`, so empty
audit-user responses produce a null value rather than a class object. Preserve
`serialize_audit_users` behavior when an ID exists.
In `@care/emr/resources/questionnaire/spec.py`:
- Around line 188-203: Move valueset validation from validate_value_set into
QuestionnaireCreateSpec and QuestionnaireUpdateSpec, where the questionnaire
auth_context or facility is available. For external_id lookups, require the
matched ValueSet to belong to that same scope, matching the slug branch’s
auth_context constraint; preserve the existing “Value set not found” error and
None handling.
- Line 153: Restore automatic UUID generation for Question.id in the Question
model so questionnaire creation remains valid when _create_questionnaire omits
question IDs. Use the existing field declaration’s default-factory pattern or
otherwise ensure IDs are populated before validation.
In `@care/emr/resources/questionnaire/utils.py`:
- Around line 516-527: Align _clean_question_response with repeat-value
validation by cleaning only the values that validation checks, or update the
validation path to cover every repeat value. Ensure invalid subsequent repeat
values raise ValidationError before _coerce_cleaned_value() can fail, while
preserving the existing single-value and valid-repeat behavior.
In `@care/emr/resources/valueset/spec.py`:
- Around line 159-166: The slug validation in validate_slug_system incorrectly
rejects any slug containing “system-”; change the check to only reject slugs
that begin with “system-”. Apply the same starts-with validation in the
corresponding validator for ValueSetUpdateSpec, while preserving inherited
values and existing error behavior.
- Around line 168-191: Restore slug uniqueness validation for ValueSetCreateSpec
using a working Pydantic model-validator signature and the serializer/request
context access pattern already used by the spec. Query ValuesetDatabaseModel
with the same auth-context-specific scopes as the database constraints,
including created_by for user scope, and raise a validation error before
persistence when a duplicate exists; ensure ValueSetCreateSpec invokes this
validator rather than relying on inherited validation.
---
Nitpick comments:
In `@care/emr/api/viewsets/questionnaire/resource_authz.py`:
- Around line 88-96: The new function is misspelled as
get_questionniare_resource; rename it to get_questionnaire_resource and update
all call sites introduced by this change to use the corrected name.
In `@care/emr/api/viewsets/questionnaire/resource_questionnaire_response.py`:
- Around line 82-99: The get_queryset logic in the resource
QuestionnaireResponse viewset incorrectly applies subject query parameters to
retrieve requests. Preserve the required subject_id/subject_type validation,
resource lookup, authorization, and filtering for list, but for retrieve resolve
the target response by its existing lookup and derive its subject information
from that object before authorizing, without requiring or filtering by query
parameters.
In `@care/emr/api/viewsets/valueset.py`:
- Line 274: Update the valueset lookup in the preference handling flow to filter
using preference.valueset_id instead of preference.valueset.id, avoiding the
unnecessary related-object fetch while preserving the existing first-match
behavior.
- Around line 216-220: Update the release documentation for the cache-key change
in get_recent_view_cache_key and get_favourites_cache_key, explicitly noting
that switching recent_views keys from slug to valueset UUID invalidates existing
cached recent views and resets them after deployment. Do not alter the
UUID-based key format or database-backed favourites behavior.
- Around line 170-174: Extract the duplicated facility-context validation and
authorization from the organization endpoints into a shared helper, using
auth_context != ValueSetAuthContext.facility and authorize_update. Replace the
inline guard at care/emr/api/viewsets/valueset.py:170-174 and the repeated guard
at care/emr/api/viewsets/valueset.py:196-199 with calls to that helper,
preserving the PermissionDenied behavior without the questionnaire-specific
message.
- Around line 291-296: Update the fallback selection around the valueset lookup
to encode the intended auth_context precedence explicitly with a Django
Case/When ordering, prioritizing facility over instance rather than relying on
alphabetical order. Replace the order-and-break loop with .first() on that
prioritized queryset, while preserving the existing “No valueset found”
ValidationError.
- Around line 201-213: Refactor the update flow around
ValueSetFacilityOrganizationUpdateSchema and ValueSetFacilityOrganization
creation to fetch all requested FacilityOrganization records for
valueset.facility in one query, validate that every requested external_id was
found, and bulk_create the corresponding ValueSetFacilityOrganization rows.
Preserve deletion, transaction atomicity, 404 behavior for invalid
organizations, and the final valueset.sync_facility_org_cache() call.
- Around line 97-105: Rename the authorization hook from
can_access_user_valueset_in_faciltiy to can_access_user_valueset_in_facility
everywhere it is defined or invoked, including this AuthorizationController.call
usage, while preserving its existing arguments and behavior.
In
`@care/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.py`:
- Around line 10-18: Update populate_questionnaire_hash to stream questionnaires
with QuerySet.iterator() instead of loading all rows, compute each
questions_hash as before, and accumulate records for batched bulk_update calls.
Flush batches at a bounded size and handle any final partial batch, updating
only the questions_hash field.
In `@care/emr/migrations/0084_facilityresourceobservation_and_more.py`:
- Around line 56-82: Add a composite database index covering subject_type and
subject_id to the FacilityResourceQuestionnaireResponse model definition in
migration 0084, and apply the same index to FacilityResourceObservation if it is
defined in this migration. Define the indexes through each model’s options so
queries using both filters are optimized.
In `@care/emr/models/questionnaire.py`:
- Around line 212-217: Remove the stray module-level TODO string from the
questionnaire module, including both guard notes; track these follow-up items
through the project’s issue workflow instead of leaving them as a no-op
expression.
- Around line 98-121: Update Questionnaire.sync_facility_org_cache and
Questionnaire.sync_org_cache in care/emr/models/questionnaire.py (lines 98-121)
to add select_related("organization") to both mapping querysets, and update
ValueSet.sync_facility_org_cache in care/emr/models/valueset.py (lines 97-107)
likewise for the ValueSetFacilityOrganization queryset. Preserve the existing
cache targets and deduplication; optionally consolidate the identical loop
through a shared helper.
In `@care/emr/resources/questionnaire/spec.py`:
- Around line 289-309: Reuse the facility and facility-organization objects
resolved during validation in perform_extra_deserialization instead of issuing
duplicate get_object_or_404 queries after validate_facility. Extract the
repeated questions hash calculation into a module-level helper, then use that
helper here and in the update spec and migration so all three paths share the
same implementation.
- Around line 352-373: Remove the unused QuestionnaireSpec class stub and its
commented validate_slug model validator block. Delete only this dead code,
leaving the surrounding questionnaire schema definitions unchanged.
In `@care/emr/resources/questionnaire/utils.py`:
- Around line 30-47: Update the fallback ValueSet query in
validate_questionnaire_valueset to use ValueSetAuthContext.instance for
auth_context, matching the string-config branch instead of hardcoding
"instance"; leave the other lookup and validation behavior unchanged.
- Around line 750-827: Refactor the duplicated validation flow in
handle_resource_response and handle_response into a shared helper that performs
response mapping, empty-response validation, enableWhen processing, question
validation, and error raising; have both callers reuse it while preserving their
existing resource-specific handling. Also replace the bulk construction loop in
handle_resource_response with an equivalent list comprehension.
In `@care/emr/resources/valueset/spec.py`:
- Around line 108-138: Remove the redundant database existence checks and “not
found” errors from validate_parent, validate_facility, and
validate_facility_organization, allowing perform_extra_deserialization to handle
missing records. Update each validator parameter annotation to UUID4 | None
while preserving the existing return behavior.
- Around line 206-221: Remove the duplicated mapping["id"] assignment from
ValueSetReadSpec.perform_extra_serialization and delegate the inherited
serialization to ValueSetMinimalReadSpec before serializing audit users.
Preserve the existing audit-user serialization behavior while ensuring future
minimal-spec changes are reused.
- Around line 140-151: Rename the model validator validate_unique_id to a
descriptive name such as validate_required_scope that reflects its auth-context
field validation. Combine the user and facility branches into one condition
requiring facility, while preserving the existing facility_organization
validation and error messages.
- Around line 16-20: Update both branches in the relevant conditional within the
questionnaire utility to use the existing ValueSetAuthContext.instance enum
member, replacing the remaining literal value while preserving the current
branch behavior.
In `@care/emr/tests/test_questionnaire_api.py`:
- Around line 237-243: Extend the assertions in the test covering
cleaned_response to include representative date, dateTime, time, url, and text
question values. Assert each projected value matches its original expected
representation, using Django’s built-in test assertions and the existing
cleaned_response fixture/setup.
In `@care/security/authorization/questionnaire.py`:
- Around line 48-60: Rename the misspelled method
can_access_user_questionnaire_in_faciltiy to
can_access_user_questionnaire_in_facility in
care/security/authorization/questionnaire.py:48-60, and rename the corresponding
can_access_user_valueset_in_faciltiy method to
can_access_user_valueset_in_facility in
care/security/authorization/valueset.py:46-58. Update all call sites to use the
corrected names.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3140603e-507d-4dcd-a6d0-c53d589b7415
📒 Files selected for processing (36)
care/emr/api/viewsets/questionnaire.pycare/emr/api/viewsets/questionnaire/__init__.pycare/emr/api/viewsets/questionnaire/questionnaire.pycare/emr/api/viewsets/questionnaire/resource_authz.pycare/emr/api/viewsets/questionnaire/resource_observation.pycare/emr/api/viewsets/questionnaire/resource_questionnaire_response.pycare/emr/api/viewsets/questionnaire_response.pycare/emr/api/viewsets/valueset.pycare/emr/locks/questionnaire.pycare/emr/migrations/0081_questionnaire_auth_context_questionnaire_facility_and_more.pycare/emr/migrations/0082_userfacilityvaluesetpreference_and_more.pycare/emr/migrations/0083_remove_questionnaire_unique_questionnaire_slug_user_and_more.pycare/emr/migrations/0084_facilityresourceobservation_and_more.pycare/emr/migrations/0085_alter_facilityresourceobservation_questionnaire_response.pycare/emr/models/__init__.pycare/emr/models/facility_resource.pycare/emr/models/questionnaire.pycare/emr/models/valueset.pycare/emr/reports/context_builder/data_points/questionnaire.pycare/emr/resources/observation/resource_spec.pycare/emr/resources/questionnaire/spec.pycare/emr/resources/questionnaire/utils.pycare/emr/resources/questionnaire_response/resource_spce.pycare/emr/resources/questionnaire_response/spec.pycare/emr/resources/valueset/spec.pycare/emr/tests/test_questionnaire_api.pycare/security/authorization/__init__.pycare/security/authorization/device.pycare/security/authorization/facility.pycare/security/authorization/facility_location.pycare/security/authorization/questionnaire.pycare/security/authorization/valueset.pycare/security/permissions/base.pycare/security/permissions/questionnaire.pycare/security/permissions/valueset.pyconfig/api_router.py
💤 Files with no reviewable changes (1)
- care/emr/api/viewsets/questionnaire.py
Comment on lines
+24
to
+36
| class QuestionnaireFilter(filters.UUIDFilter): | ||
| def filter(self, qs, value): | ||
| if value is None: | ||
| return qs | ||
| questionnaire = ( | ||
| Questionnaire.objects.only("id").filter(external_id=value).first() | ||
| ) | ||
| if not questionnaire: | ||
| return qs.none() | ||
| return qs.filter( | ||
| Q(questionnaire=questionnaire) | ||
| | Q(questionnaire__latest_revision_id=questionnaire.id) | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Match the latest revision in the forward direction.
Line [35] looks for a response questionnaire whose latest_revision_id points back to the requested revision. The model contract points the other way: a historical questionnaire’s latest_revision_id identifies the current revision, so filtering by a historical external ID currently misses responses saved against that latest questionnaire.
Guard the nullable relation and filter with questionnaire_id=questionnaire.latest_revision_id instead. The current predicate is, unfortunately, resolving the lineage backwards.
Proposed fix
- Questionnaire.objects.only("id").filter(external_id=value).first()
+ Questionnaire.objects.only("id", "latest_revision_id")
+ .filter(external_id=value)
+ .first()
...
- return qs.filter(
- Q(questionnaire=questionnaire)
- | Q(questionnaire__latest_revision_id=questionnaire.id)
- )
+ if questionnaire.latest_revision_id is None:
+ return qs.filter(questionnaire=questionnaire)
+ return qs.filter(
+ Q(questionnaire=questionnaire)
+ | Q(questionnaire_id=questionnaire.latest_revision_id)
+ )Also applies to: 39-42
🤖 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 `@care/emr/api/viewsets/questionnaire_response.py` around lines 24 - 36, Update
QuestionnaireFilter.filter to match responses through the latest revision in the
forward direction: guard the nullable latest_revision_id relation, then filter
using questionnaire_id equal to questionnaire.latest_revision_id alongside the
direct questionnaire match. Apply the same correction to the equivalent
predicate referenced in the additional lines.
Comment on lines
+87
to
+93
| with transaction.atomic(): | ||
| old_obj = QuestionnaireResponse.objects.get(id=instance.id) | ||
| if ( | ||
| old_obj.status != instance.status | ||
| and instance.status | ||
| == QuestionnaireResponseStatusChoices.entered_in_error.value | ||
| ): |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Lock and revalidate the response before saving it.
authorize_update runs before this transaction, and old_obj is loaded without a row lock. A concurrent request can mark the response entered_in_error after authorization; this request can then save its stale completed instance and reopen it. Acquire select_for_update() before authorization, or at minimum revalidate the locked status before calling the superclass update.
Also applies to: 97-97
🤖 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 `@care/emr/api/viewsets/questionnaire_response.py` around lines 87 - 93, Lock
and revalidate the QuestionnaireResponse row before saving it to prevent a stale
authorized instance from reopening an entered-in-error response. Update the
authorize_update/update flow around transaction.atomic and old_obj to acquire
the row lock with select_for_update before authorization, or recheck the locked
status immediately before invoking the superclass update. Preserve the existing
status-transition validation for the locked record.
Comment on lines
+94
to
+96
| Observation.objects.filter(questionnaire_response=instance).update( | ||
| status=ObservationStatus.entered_in_error.value | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve Observation audit metadata during propagation.
QuerySet.update() bypasses Observation.save(), signals, and the audit handling used by EMRUpdateMixin (updated_by in particular). Marking a response as entered_in_error can therefore leave its observations with stale audit information. Update the audit fields explicitly or use a model-level transition method inside this transaction.
🤖 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 `@care/emr/api/viewsets/questionnaire_response.py` around lines 94 - 96, Update
the observation status propagation around the
`Observation.objects.filter(...).update(...)` call to preserve audit metadata,
especially `updated_by`. Use the model-level transition or audit-aware update
path provided by `EMRUpdateMixin`, or explicitly set all required audit fields
within the same transaction, rather than bypassing `Observation.save()` and
signals with a bare queryset update.
Comment on lines
+226
to
+239
| resource = None | ||
| if questionnaire.subject_type == SubjectType.location: | ||
| resource = get_object_or_404( | ||
| FacilityLocation, external_id=request_params.resource_id | ||
| ) | ||
| elif questionnaire.subject_type == SubjectType.device: | ||
| resource = get_object_or_404(Device, external_id=request_params.resource_id) | ||
| elif questionnaire.subject_type == SubjectType.facility: | ||
| resource = get_object_or_404( | ||
| Facility, external_id=request_params.resource_id | ||
| ) | ||
| else: | ||
| err = f"Invalid resource type: {questionnaire.subject_type}" | ||
| raise ValidationError(err) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicates get_questionniare_resource from resource_authz.py.
This if/elif chain re-implements the exact same location/device/facility resolution (and "invalid resource type" error) that already exists as get_questionniare_resource in resource_authz.py. Reusing it avoids maintaining two copies when a new subject type is added.
♻️ Suggested refactor
- resource = None
- if questionnaire.subject_type == SubjectType.location:
- resource = get_object_or_404(
- FacilityLocation, external_id=request_params.resource_id
- )
- elif questionnaire.subject_type == SubjectType.device:
- resource = get_object_or_404(Device, external_id=request_params.resource_id)
- elif questionnaire.subject_type == SubjectType.facility:
- resource = get_object_or_404(
- Facility, external_id=request_params.resource_id
- )
- else:
- err = f"Invalid resource type: {questionnaire.subject_type}"
- raise ValidationError(err)
+ resource = get_questionniare_resource(
+ questionnaire.subject_type, request_params.resource_id
+ )(Add get_questionniare_resource to the existing import from care.emr.api.viewsets.questionnaire.resource_authz.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resource = None | |
| if questionnaire.subject_type == SubjectType.location: | |
| resource = get_object_or_404( | |
| FacilityLocation, external_id=request_params.resource_id | |
| ) | |
| elif questionnaire.subject_type == SubjectType.device: | |
| resource = get_object_or_404(Device, external_id=request_params.resource_id) | |
| elif questionnaire.subject_type == SubjectType.facility: | |
| resource = get_object_or_404( | |
| Facility, external_id=request_params.resource_id | |
| ) | |
| else: | |
| err = f"Invalid resource type: {questionnaire.subject_type}" | |
| raise ValidationError(err) | |
| resource = get_questionniare_resource( | |
| questionnaire.subject_type, request_params.resource_id | |
| ) |
🤖 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 `@care/emr/api/viewsets/questionnaire/questionnaire.py` around lines 226 - 239,
Replace the duplicated subject-type resolution block in the questionnaire view
with the existing get_questionniare_resource helper from resource_authz.py. Add
that helper to the existing import and pass the request resource identifier and
questionnaire subject type as required, preserving the helper’s existing
validation and 404 behavior.
|
|
||
| class ResourceObservationAnalyseRequest(BaseModel): | ||
| codes: list[Coding] = Field(min_length=1, max_length=20) | ||
| page_size: int = Field(10, le=30) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('care/emr/api/viewsets/questionnaire/resource_observation.py')
print('FILE EXISTS:', path.exists())
print('LINE COUNT:', sum(1 for _ in path.open()))
print('\n--- around referenced lines ---')
for i, line in enumerate(path.open(), start=1):
if 35 <= i <= 95:
print(f'{i:4}: {line.rstrip()}')
PYRepository: ohcnetwork/care
Length of output: 2844
🏁 Script executed:
rg -n "page_size" care/emr/api/viewsets/questionnaire/resource_observation.py care/emr/api -g '*.py'Repository: ohcnetwork/care
Length of output: 1379
🏁 Script executed:
python3 - <<'PY'
try:
import django
from django.db.models.query import QuerySet
import inspect
print("DJANGO_VERSION:", django.get_version())
src = inspect.getsource(QuerySet.__getitem__)
for i, line in enumerate(src.splitlines(), start=1):
if i <= 80:
print(f"{i:4}: {line}")
except Exception as e:
print(type(e).__name__ + ":", e)
PYRepository: ohcnetwork/care
Length of output: 199
care/emr/api/viewsets/questionnaire/resource_observation.py:47 — Add a lower bound to page_size. le=30 still lets negative values through, and Django rejects negative queryset slices. A tiny extra bound would spare the endpoint from that little surprise.
Suggested fix
- page_size: int = Field(10, le=30)
+ page_size: int = Field(10, ge=1, le=30)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| page_size: int = Field(10, le=30) | |
| page_size: int = Field(10, ge=1, le=30) |
🤖 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 `@care/emr/api/viewsets/questionnaire/resource_observation.py` at line 47,
Update the page_size Field declaration to enforce a non-negative lower bound in
addition to the existing maximum of 30, preventing invalid negative queryset
slices while preserving the current upper limit.
| id: UUID4 | UUID5 = Field( | ||
| description="Unique machine provided UUID", default_factory=uuid.uuid4 | ||
| ) | ||
| id: UUID4 | UUID5 = Field(description="Unique machine provided UUID") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find questionnaire creation payloads that omit question ids
rg -nP --type=py -C6 '"type":\s*"boolean"' care/emr/tests | head -80
# Confirm no default is applied for Question.id
ast-grep run --pattern 'id: $TYPE = $VAL' --lang python care/emr/resources/questionnaire/spec.pyRepository: ohcnetwork/care
Length of output: 6585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- spec.py excerpt ---'
sed -n '130,170p' care/emr/resources/questionnaire/spec.py
echo
echo '--- questionnaire tests around helper ---'
sed -n '1,220p' care/emr/tests/test_questionnaire_api.py
echo
echo '--- search for QuestionnaireValidationTests and id generation ---'
rg -n "QuestionnaireValidationTests|default_factory|UUID4 \| UUID5|machine provided UUID|question.*id|\"id\"" care/emr -g '*.py'Repository: ohcnetwork/care
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the helper that constructs questionnaire payloads and whether it injects ids.
sed -n '100,190p' care/emr/tests/test_questionnaire_api.py
# Locate any code that auto-populates question ids.
rg -n "default_factory=.*UUID|default_factory=.*uuid|uuid4\(|id\s*=\s*Field|question.*id" care/emr -g '*.py'Repository: ohcnetwork/care
Length of output: 46872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- questionnaire resource/viewset create path ---'
rg -n "def create|serializer|Question\(|id: UUID4 \| UUID5|default_factory|uuid4\(" care/emr/api care/emr/resources/questionnaire care/emr/models -g '*.py'
echo
echo '--- questionnaire model and utils around id handling ---'
sed -n '1,240p' care/emr/models/questionnaire.py
echo
sed -n '1,260p' care/emr/resources/questionnaire/utils.py
echo
sed -n '1,260p' care/emr/api/viewsets/questionnaire/questionnaire.pyRepository: ohcnetwork/care
Length of output: 35873
Restore the question UUID default — Question.id is required here, but care/emr/tests/test_questionnaire_api.py::_create_questionnaire still posts questions without id. Unless something upstream is secretly minting them, the create payload will start failing. Either add back the default factory or populate IDs in the payload builder.
🤖 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 `@care/emr/resources/questionnaire/spec.py` at line 153, Restore automatic UUID
generation for Question.id in the Question model so questionnaire creation
remains valid when _create_questionnaire omits question IDs. Use the existing
field declaration’s default-factory pattern or otherwise ensure IDs are
populated before validation.
Comment on lines
+188
to
+203
| def validate_value_set(cls, valueset): | ||
| if valueset is None: | ||
| return valueset | ||
|
|
||
| err = "Value set not found" | ||
| if valueset.external_id: | ||
| if not ValueSet.objects.filter(external_id=valueset.external_id).exists(): | ||
| raise ValueError(err) | ||
| return valueset | ||
|
|
||
| if not ValueSet.objects.filter( | ||
| slug=valueset.slug, | ||
| auth_context=QuestionnaireAuthContext.instance, | ||
| ).exists(): | ||
| raise ValueError(err) | ||
| return slug | ||
| return valueset |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
The external_id branch skips the scoping check that the slug branch enforces.
Slug lookups are pinned to auth_context=instance, but external_id accepts any valueset — including one owned by a different facility or another user's private valueset. A facility-scoped questionnaire can therefore bind questions to a foreign facility's valueset, which is precisely the concern noted in the trailing comment block in care/emr/models/questionnaire.py (lines 214-217).
The validator has no access to the questionnaire's facility, so this likely needs to move into QuestionnaireCreateSpec/QuestionnaireUpdateSpec where facility/auth_context are known.
🤖 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 `@care/emr/resources/questionnaire/spec.py` around lines 188 - 203, Move
valueset validation from validate_value_set into QuestionnaireCreateSpec and
QuestionnaireUpdateSpec, where the questionnaire auth_context or facility is
available. For external_id lookups, require the matched ValueSet to belong to
that same scope, matching the slug branch’s auth_context constraint; preserve
the existing “Value set not found” error and None handling.
Comment on lines
+516
to
+527
| def _clean_question_response(question, response): | ||
| values = getattr(response, "values", None) or [] | ||
| cleaned_values = [ | ||
| _clean_result_value(question, value) | ||
| for value in values | ||
| if value.value is not None or value.coding or value.unit | ||
| ] | ||
| if not cleaned_values: | ||
| return None | ||
| if question.get("repeats", False) or len(cleaned_values) > 1: | ||
| return cleaned_values | ||
| return cleaned_values[0] |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and relevant symbols first.
ast-grep outline care/emr/resources/questionnaire/utils.py --view expanded || true
printf '\n--- validate_question_result ---\n'
rg -n "def validate_question_result|values\[0:1\]|_clean_question_response|_coerce_cleaned_value|handle_response|cleaned_response" care/emr/resources/questionnaire/utils.py
printf '\n--- surrounding lines ---\n'
sed -n '260,360p' care/emr/resources/questionnaire/utils.py
printf '\n--- cleaning lines ---\n'
sed -n '500,560p' care/emr/resources/questionnaire/utils.pyRepository: ohcnetwork/care
Length of output: 8805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '480,690p' care/emr/resources/questionnaire/utils.py
printf '\n--- handle_response tail ---\n'
sed -n '690,840p' care/emr/resources/questionnaire/utils.pyRepository: ohcnetwork/care
Length of output: 11917
Validate every repeat value, or truncate cleaning to match For repeat questions, validation only checks values[0:1], while cleaning still walks every item. So the 2nd+ bad value gets to crash in _coerce_cleaned_value() instead of surfacing as a ValidationError—because consistency is apparently optional here.
🤖 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 `@care/emr/resources/questionnaire/utils.py` around lines 516 - 527, Align
_clean_question_response with repeat-value validation by cleaning only the
values that validation checks, or update the validation path to cover every
repeat value. Ensure invalid subsequent repeat values raise ValidationError
before _coerce_cleaned_value() can fail, while preserving the existing
single-value and valid-repeat behavior.
Comment on lines
159
to
166
| @model_validator(mode="after") | ||
| def validate_slug_system(self): | ||
| if not self.is_system_defined and self.slug and "system-" in self.slug: | ||
| if self.inherited: | ||
| return self | ||
| if "system-" in self.slug: | ||
| err = "Cannot create valueset with system like slug" | ||
| raise ValueError(err) | ||
| return self |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"system-" in self.slug matches anywhere in the slug.
A perfectly innocent slug like hospital-system-codes gets rejected. self.slug.startswith("system-") is presumably the intent (same applies to the copy in ValueSetUpdateSpec).
🤖 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 `@care/emr/resources/valueset/spec.py` around lines 159 - 166, The slug
validation in validate_slug_system incorrectly rejects any slug containing
“system-”; change the check to only reject slugs that begin with “system-”.
Apply the same starts-with validation in the corresponding validator for
ValueSetUpdateSpec, while preserving inherited values and existing error
behavior.
Comment on lines
+168
to
+191
| # @model_validator(mode="after") | ||
| # def validate_slug(self, info): | ||
| # # Uniqueness changes based on the auth context | ||
| # if self.auth_context == ValueSetAuthContext.instance: | ||
| # queryset = ValuesetDatabaseModel.objects.filter(slug=self.slug) | ||
| # elif self.auth_context == ValueSetAuthContext.facility: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # facility__external_id=self.facility | ||
| # ) | ||
| # elif self.auth_context == ValueSetAuthContext.facility_organization: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # facility_organization__organization__external_id=self.facility_organization | ||
| # ) | ||
| # elif self.auth_context == ValueSetAuthContext.user: | ||
| # queryset = ValuesetDatabaseModel.objects.filter( | ||
| # created_by=self.get_serializer_context(info)["user"] | ||
| # ) | ||
| # else: | ||
| # raise ValueError("Invalid auth context") | ||
| # if queryset.exists(): | ||
| # err = "Slug must be unique" | ||
| # raise ValueError(err) | ||
|
|
||
| # return self |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Commented-out slug uniqueness validator leaves duplicates to the database.
With this block disabled and ValueSetCreateSpec no longer inheriting ValueSetSpec.validate_slug, a duplicate slug now reaches the DB and surfaces as an IntegrityError (500) instead of a 400 validation error. The dead code also references info, which a mode="after" model validator doesn't receive positionally — so it wouldn't work as written if uncommented.
Happy to draft the working per-auth-context uniqueness validator (including the created_by/facility scoping the constraints use) if you want — or should this be tracked as a follow-up issue?
🤖 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 `@care/emr/resources/valueset/spec.py` around lines 168 - 191, Restore slug
uniqueness validation for ValueSetCreateSpec using a working Pydantic
model-validator signature and the serializer/request context access pattern
already used by the spec. Query ValuesetDatabaseModel with the same
auth-context-specific scopes as the database constraints, including created_by
for user scope, and raise a validation error before persistence when a duplicate
exists; ensure ValueSetCreateSpec invokes this validator rather than relying on
inherited validation.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
care/emr/resources/questionnaire/spec.py:154
Question.idis now required, which breaks questionnaire creation payloads that omit per-question IDs (e.g., tests/builders). Consider keeping backward compatibility by generating a UUID when the client doesn’t provide one.
link_id: str = Field(description="Unique human readable ID for linking")
id: UUID4 | UUID5 = Field(description="Unique machine provided UUID")
code: ValueSetBoundCoding[CARE_OBSERVATION_VALUSET.slug] | None = None
care/emr/api/viewsets/valueset.py:210
- Error message refers to “questionnaires”, but this endpoint is for value sets.
if not valueset.auth_context == ValueSetAuthContext.facility:
raise PermissionDenied(
"Facility organizations can only be set for facility level questionnaires"
)
care/emr/api/viewsets/valueset.py:236
- Error message refers to “questionnaires”, but this endpoint is for value sets.
if not valueset.auth_context == ValueSetAuthContext.facility:
raise PermissionDenied(
"Facility organizations can only be set for facility level questionnaires"
)
care/emr/api/viewsets/questionnaire/questionnaire.py:269
- Typo in validation error message: “Questionniare” → “Questionnaire”.
raise ValidationError(
"This Questionniare is a past revision, please submit to the latest revision"
)
care/emr/models/questionnaire.py:162
resolved_questionnaireuses.objects.get(...), which can raiseDoesNotExistand turn response serialization into a 500 if the referenced revision row is missing. Safer to use.filter(...).first()and fall back to the current questionnaire.
return Questionnaire.objects.get(
latest_revision_id=self.questionnaire_id,
internal_revision=self.revision,
)
care/emr/models/facility_resource.py:25
resolved_questionnaireuses.objects.get(...), which can raiseDoesNotExistand break reads if the revision row isn’t present. Prefer a safe lookup with fallback.
if self.revision == self.questionnaire.internal_revision:
return self.questionnaire
return self.questionnaire.__class__.objects.get(
latest_revision_id=self.questionnaire_id,
internal_revision=self.revision,
)
care/emr/api/viewsets/questionnaire/questionnaire.py:225
- Typo in validation error message: “Questionniare” → “Questionnaire”.
This issue also appears on line 267 of the same file.
raise ValidationError(
"This Questionniare is a past revision, please submit to the latest revision"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (9)
care/emr/resources/questionnaire/spec.py:154
Question.idis now required, but existing in-repo API tests create questionnaires with onlylink_id/type/text(noid) and rely on the backend to generate question IDs for later submissions. Makingidmandatory will break those tests and likely existing API clients. If the intent isn’t to make this a breaking change, consider restoring a default_factory (or generating a deterministic UUID fromlink_id).
link_id: str = Field(description="Unique human readable ID for linking")
id: UUID4 | UUID5 = Field(description="Unique machine provided UUID")
code: ValueSetBoundCoding[CARE_OBSERVATION_VALUSET.slug] | None = None
care/emr/api/viewsets/valueset.py:212
- The PermissionDenied messages in
get_facility_organizations/set_facility_organizationssay “facility level questionnaires”, but these endpoints are on the ValueSet API. This is confusing for API clients and makes debugging harder.
if not valueset.auth_context == ValueSetAuthContext.facility:
raise PermissionDenied(
"Facility organizations can only be set for facility level questionnaires"
)
care/emr/api/viewsets/valueset.py:238
- Same message typo here: this ValueSet endpoint mentions “facility level questionnaires” instead of value sets, which is misleading for clients.
if not valueset.auth_context == ValueSetAuthContext.facility:
raise PermissionDenied(
"Facility organizations can only be set for facility level questionnaires"
)
care/security/authorization/valueset.py:75
get_filtered_valuesetsexcludes facility-scoped ValueSets unlessinternal_organization_cacheoverlaps the caller’s FacilityOrganization IDs. A newly created facility ValueSet starts with an empty cache (untilset_facility_organizationsis called), which means it becomes impossible to retrieve/manage it via the API (detail routes will 404 becauseget_object()uses this filtered queryset). Consider also allowing the creator to see their facility ValueSets so they can set facility organizations after creation.
care/security/authorization/questionnaire.py:108get_filtered_questionnaireshas the same “new facility item is unreachable” problem as ValueSets: facility-scoped questionnaires are only included viainternal_organization_cacheoverlap, but a newly created facility questionnaire has an empty cache untilset_facility_organizationsruns. Becauseget_object()uses the filtered queryset, the creator may be unable to retrieve the questionnaire to configure its facility organizations (404). Consider also including facility questionnaires created by the current user (at least) so they remain manageable.
care/emr/models/questionnaire.py:162resolved_questionnairecan issue an extra DB query every time it’s accessed for a non-latest revision. This is easy to trigger in reporting/context-building code that readsresolved_questionnaire.title/descriptionper row, creating an N+1 query pattern. Consider caching the resolved questionnaire per instance to avoid repeated queries within the same request.
@property
def resolved_questionnaire(self):
if not self.questionnaire:
return None
if self.revision == self.questionnaire.internal_revision:
return self.questionnaire
return Questionnaire.objects.get(
latest_revision_id=self.questionnaire_id,
internal_revision=self.revision,
)
care/emr/api/viewsets/questionnaire/resource_authz.py:96
get_questionniare_resourceis misspelled (“questionniare”). Since this is a shared helper that’s imported by multiple new viewsets, keeping the typo will spread throughout the codebase and make future search/maintenance harder. Consider renaming it toget_questionnaire_resourceand updating imports/callers.
def get_questionniare_resource(resource_type, resource):
if resource_type == SubjectType.location:
return get_object_or_404(FacilityLocation, external_id=resource)
if resource_type == SubjectType.device:
return get_object_or_404(Device, external_id=resource)
if resource_type == SubjectType.facility:
return get_object_or_404(Facility, external_id=resource)
err = f"Invalid resource type: {resource_type}"
raise ValidationError(err)
care/emr/resources/questionnaire_response/resource_spce.py:29
- The file name
resource_spce.pylooks like a typo (likelyresource_spec.py). Also,ResourceQuestionnaireSubmitResultis currently unused (the request schema usesQuestionnaireSubmitResultinstead), which adds confusion. Consider renaming the module and either removing the unused class or switching the request schema to use it (and updating call sites).
from datetime import datetime
from pydantic import UUID4, UUID5, BaseModel
from care.emr.models.facility_resource import FacilityResourceQuestionnaireResponse
from care.emr.resources.base import EMRResource
from care.emr.resources.common import Coding
from care.emr.resources.questionnaire.spec import QuestionnaireReadSpec
from care.emr.resources.questionnaire_response.spec import (
QuestionnaireResponseStatusChoices,
QuestionnaireSubmitResult,
QuestionnaireSubmitResultValue,
)
from care.emr.resources.user.spec import UserSpec
class ResourceQuestionnaireSubmitResult(BaseModel):
question_id: UUID4 | UUID5
method: Coding | None = None
taken_at: datetime | None = None
values: list[QuestionnaireSubmitResultValue] = []
note: str | None = None
sub_results: list[list["ResourceQuestionnaireSubmitResult"]] = []
class ResourceQuestionnaireSubmitRequest(BaseModel):
resource_id: UUID4
results: list[QuestionnaireSubmitResult]
care/emr/api/viewsets/questionnaire/resource_questionnaire_response.py:99
- New resource questionnaire response API behavior (required
subject_id/subject_typequery params, authorization, andentered_in_errorcascading to observations) is introduced here, but there are no corresponding tests covering these endpoints/filters/permission checks. Adding API tests similar to the existing questionnaire response tests would help prevent regressions.
def get_queryset(self):
queryset = (
super()
.get_queryset()
.order_by("-created_date")
.select_related("questionnaire")
)
if self.action in ["list", "retrieve"]:
subject_id = self.request.GET.get("subject_id")
subject_type = self.request.GET.get("subject_type")
if not subject_id or not subject_type:
raise ValidationError("subject_id and subject_type are required")
subject = get_questionniare_resource(subject_type, subject_id)
authorize_resource_questionnaire_response_read(
subject_type, subject, self.request.user
)
queryset = queryset.filter(subject_type=subject_type, subject_id=subject_id)
|
One edge on the parent chain: |
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
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.
Proposed Changes
Associated Issue
ENG-737
Summary by CodeRabbit