Refactor load_fixtures command into modular fixture modules#3576
Open
Refactor load_fixtures command into modular fixture modules#3576
Conversation
… and add --output-json Break the 1,751-line load_fixtures.py into focused modules under fixtures/: - __init__.py: shared utilities (faker setup, phone generator, valueset bypass) - organizations.py: geo/role/facility organization factories - facilities.py: facility, location, and device factories - users.py: user creation with role attachment - patients.py: patient and encounter factories - clinical.py: lab definitions (specimen, observation, activity definitions) - inventory.py: products, inventory items, supply deliveries - questionnaires.py: questionnaire loading from JSON The main load_fixtures.py is now a slim orchestrator (~220 lines) that imports and calls functions from these modules. Add --output-json flag that writes a JSON manifest of all created entity IDs (facility, patients, encounters, users, locations, devices). This enables frontend Playwright tests to read IDs directly instead of navigating the UI to extract them. https://claude.ai/code/session_011i8in4FLq4renAvgodeJJu
Move shared helpers (create_object, create_resource_category, create_charge_item_definition) to fixtures/__init__.py to eliminate duplication between clinical.py and inventory.py. Make all factory functions public (remove _ prefix) so they can be composed individually. Update Makefile to pass --output-json to load_fixtures. https://claude.ai/code/session_011i8in4FLq4renAvgodeJJu
Contributor
📝 WalkthroughWalkthroughThe fixture loader was reorganized into multiple fixture modules, moving monolithic fixture creation into dedicated helpers and introducing a --output-json option that writes a generated JSON manifest from load_fixtures. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55c931a39a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| geo_organization = self._create_organization(fake, super_user) | ||
| self.geo_organization = geo_organization | ||
| manifest["admin"] = {"username": "admin", "password": "admin"} |
There was a problem hiding this comment.
The manifest always records {"username": "admin", "password": "admin"} even when the command explicitly reports that it reused an existing admin account. On a reused database where admin has a non-default password, automation that reads this manifest for login will fail with incorrect credentials. Only emit the password when the admin user was newly created in this run, or omit it entirely for existing users.
Useful? React with 👍 / 👎.
Comment on lines
+79
to
+83
| if options["output_json"]: | ||
| from pathlib import Path | ||
|
|
||
| with Path(options["output_json"]).open("w") as f: | ||
| json.dump(manifest, f, indent=2) |
There was a problem hiding this comment.
The JSON manifest is written after the transaction.atomic() block, so an unwritable or missing --output-json destination raises only after fixtures have already been committed. In that case callers see a failed command and often retry, which can create duplicate fixture data even though the first run partially succeeded. Keeping this write in the atomic section (or handling I/O errors without failing the command) avoids this inconsistent failure mode.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3576 +/- ##
===========================================
- Coverage 77.05% 77.05% -0.01%
===========================================
Files 474 474
Lines 22271 22271
Branches 2325 2325
===========================================
- Hits 17161 17160 -1
Misses 4553 4553
- Partials 557 558 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace parameter-threading with a shared FixtureContext dataclass that accumulates state as fixture modules run. Each module now exposes a setup_*(ctx) function that reads what it needs and writes back what it produces. Adding a new fixture domain is now: 1. Create fixtures/<domain>.py with a setup_<domain>(ctx) function 2. Add one import + one line in load_fixtures.py Key changes: - FixtureContext holds fake, super_user, facility, geo_organization, facility_organization, manifest, options, and a log() helper - Each module (organizations, facilities, users, patients, clinical, inventory, questionnaires) has a setup_* entry point - load_fixtures.py orchestrator reduced from ~210 lines to ~25 lines - Individual create_* functions preserved for direct use in tests https://claude.ai/code/session_011i8in4FLq4renAvgodeJJu
![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: 9
🧹 Nitpick comments (3)
care/emr/management/commands/fixtures/clinical.py (1)
69-766: This helper is still carrying the whole lab catalog in one function.Most of this body is static payload data plus repeated creation calls. Pulling those definitions into declarative constants or a data file and iterating over them would make the orchestration readable again, and a lot less fun to diff next time. As per coding guidelines, "Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/clinical.py` around lines 69 - 766, create_lab_definition_objects is doing too much inline: large static payloads and repeated create_object/create_charge_item_definition/create_resource_category calls make it unreadable and hard to maintain; refactor by extracting the static dictionaries (e.g., code_ucum_*, code_hl7_*, code_snomed_*, default_price_components) and each resource spec body (used to instantiate BaseSpecimenDefinitionSpec, BaseObservationDefinitionSpec, BaseActivityDefinitionSpec, charge definitions) into module-level declarative constants or an external JSON/YAML data file, then iterate over those structures to call create_object, create_charge_item_definition, and create_resource_category; also split logic into small helpers like build_specimen_definitions, build_observation_definitions, build_activity_definitions to keep create_lab_definition_objects as orchestration only and reference symbols such as create_lab_definition_objects, BaseSpecimenDefinitionSpec, BaseObservationDefinitionSpec, BaseActivityDefinitionSpec, create_charge_item_definition, and create_resource_category when implementing the changes.care/emr/management/commands/load_fixtures.py (1)
79-88: Consider adding error handling for the file write operation.The JSON file writing could fail (e.g., permission issues, disk full), and currently this would raise an unhandled exception after the transaction has already committed successfully. It might be nice to wrap this in a try-except to provide a clearer error message while still indicating that the fixtures themselves were created successfully.
💡 Suggested improvement
if options["output_json"]: from pathlib import Path - with Path(options["output_json"]).open("w") as f: - json.dump(manifest, f, indent=2) - self.stdout.write( - self.style.SUCCESS( - f"Fixture manifest written to {options['output_json']}" + try: + with Path(options["output_json"]).open("w") as f: + json.dump(manifest, f, indent=2) + self.stdout.write( + self.style.SUCCESS( + f"Fixture manifest written to {options['output_json']}" + ) ) - ) + except OSError as e: + self.stdout.write( + self.style.WARNING( + f"Fixtures created successfully, but failed to write manifest: {e}" + ) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/load_fixtures.py` around lines 79 - 88, The file write of the manifest using Path(options["output_json"]).open and json.dump can raise IO errors after the DB transaction commits; wrap that block in a try/except around the open/json.dump call to catch OSError/IOError/Exception, and on failure call self.stdout.write(self.style.ERROR(...)) (or self.stderr.write) to report a clear message that the fixture creation succeeded but writing the JSON failed, including the exception text and the target path (options["output_json"]); ensure the original success message is only emitted when the dump completes without exception.Makefile (1)
112-112: Consider extracting the manifest path to a variable.The path
/app/fixture_manifest.jsonappears in both line 39 and here. Not that it's a huge issue, but extracting it to a variable would reduce duplication and make future path changes easier.Also, if Playwright tests run on the host and need access to this manifest, you'll need to ensure the path is accessible (e.g., via a volume mount or
docker compose cp).💡 Suggested improvement
Add a variable near the top of the Makefile:
FIXTURE_MANIFEST := /app/fixture_manifest.jsonThen use it in both targets:
load-fixtures load-dummy-data: - docker compose exec backend bash -c "python manage.py load_fixtures --output-json /app/fixture_manifest.json" + docker compose exec backend bash -c "python manage.py load_fixtures --output-json $(FIXTURE_MANIFEST)"- docker compose exec backend bash -c "python manage.py migrate && python manage.py load_fixtures --output-json /app/fixture_manifest.json"; \ + docker compose exec backend bash -c "python manage.py migrate && python manage.py load_fixtures --output-json $(FIXTURE_MANIFEST)"; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 112, Extract the hard-coded fixture manifest path into a Makefile variable (suggest name FIXTURE_MANIFEST) declared near the top of the Makefile, then replace the literal path occurrences in the targets that call the manage.py commands (the line invoking docker compose exec backend ... migrate && ... load_fixtures and the other target at line ~39) to reference $(FIXTURE_MANIFEST); also update any relevant Makefile recipe comments and ensure the Docker Compose service exposes that path (via a volume or copy step) if Playwright on the host needs access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/management/commands/fixtures/__init__.py`:
- Around line 109-112: The helper currently skips adding the "price_components"
entry when price is 0 because it uses a truthiness check; change the conditional
to test for absence rather than falsiness (e.g., use "if price is not None" or
equivalent) so that price=0 is treated as a valid value and
_data["price_components"] is populated with {"amount": price,
"monetary_component_type": "base"}; update the conditional near the use of the
local variable price and the assignment to _data["price_components"]
accordingly.
- Around line 8-30: Extract the import-time monkey patches into an explicit
context manager (e.g., fixture_patches() implemented with
contextlib.contextmanager) that, on enter, replaces
sys.modules["care.emr.utils.valueset_coding_type"].validate_valueset with the
lambda and sets GeoProvider.coordinate = safe_coordinate, and on exit restores
the original validate_valueset and GeoProvider.coordinate references; keep the
safe_coordinate function unchanged but move its definition next to the new
context manager so callers can import it without activating patches. Also change
the price conditional from a truthy check to an explicit None check (replace "if
price:" with "if price is not None") wherever price_components are set so
Decimal(0) prices still produce price_components. Ensure callers that generate
fixtures use the new fixture_patches() context manager when applying these
patches.
In `@care/emr/management/commands/fixtures/inventory.py`:
- Around line 124-130: When a supply_delivery is for a transfer
(supply_delivery.order.origin is truthy) the code currently only calls
sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item)
which recomputes the origin side and leaves the destination
InventoryItem.net_content stale; update the branch so that for transfers you
call sync_inventory_item for both sides: keep the existing
sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) to
recompute the origin and also call
sync_inventory_item(location=supply_delivery.order.destination,
product=supply_delivery.supplied_inventory_item.product) to recompute the
destination InventoryItem.net_content.
In `@care/emr/management/commands/fixtures/patients.py`:
- Around line 24-39: The fixture fails because
PatientCreateSpec.validate_identifiers() expects required identifiers but the
spec is created with the default empty identifiers list; update the patient
fixture to populate patient_spec.identifiers with the required identifier
entries before calling de_serialize/save. Inspect the system's patient
identifier configuration (via PatientCreateSpec.validate_identifiers or your
identifier config helper) and either generate the required identifier objects
(matching expected fields like type/system/value) and assign them to
patient_spec.identifiers, or accept identifiers from callers and pass them into
PatientCreateSpec on construction so validate_identifiers passes. Ensure you
reference PatientCreateSpec, its identifiers attribute, and the
validate_identifiers behavior when implementing the change.
In `@care/emr/management/commands/fixtures/questionnaires.py`:
- Around line 17-18: The code opens "data/questionnaire_fixtures.json" relative
to the current working directory; change it to compute the fixture path relative
to the module file so imports work from anywhere: use
Path(__file__).resolve().parent.joinpath("data", "questionnaire_fixtures.json")
(or equivalent) and open that path when loading JSON (replace the current
Path.open call). Target the module-level loader where the code calls Path.open
and adjust to use the computed fixture_path.
In `@care/emr/management/commands/fixtures/users.py`:
- Around line 131-153: The loop in the fixture generator builds deterministic
usernames and then always calls create_user, which fails on rerun due to
uniqueness constraints; before calling create_user (in the loop that builds
username, role and password) check whether a user with that username already
exists (e.g., via User.objects.filter(username=username).exists() or the
project's user lookup helper) and skip to the next iteration if found, only
calling create_user and appending to created_users when the username does not
already exist.
- Around line 69-75: The call in create_user() that uses
Organization.objects.get(name=role.name, org_type=OrganizationTypeChoices.role)
can raise DoesNotExist if create_role_organizations() wasn't run; change this to
ensure the role organization exists before attaching by using a safe creation
pattern (e.g. Organization.objects.get_or_create(...) or try/except DoesNotExist
and create) when preparing the organization for attach_role_organization_user;
update the logic in the create_user()/attach_role_organization_user flow to
reference Organization with org_type=OrganizationTypeChoices.role and
name=role.name so the org is created if missing.
In `@care/emr/management/commands/load_fixtures.py`:
- Around line 90-93: The CI failed because Ruff reformatted files; run the
project's formatter (ruff) and apply its suggested fixes, then commit the
updated files referenced by the CI; specifically run ruff/ruff --fix (or the
repo's preconfigured format command) and ensure the changes touching the
_generate_fixtures function in load_fixtures.py and the other three files
reformatted by Ruff are committed so the PR matches CI formatting.
- Around line 120-121: The manifest currently writes plaintext credentials via
manifest["admin"] = {"username": "admin", "password": "admin"}; remove or redact
the password before writing output JSON (e.g., set password to None or
"<redacted>" or omit the "password" key) so credentials are not exposed, and add
a short comment near the manifest assignment explaining that output JSON may be
committed and should not contain real secrets; update any related helpers that
consume manifest (if any) to handle the redacted/omitted password.
---
Nitpick comments:
In `@care/emr/management/commands/fixtures/clinical.py`:
- Around line 69-766: create_lab_definition_objects is doing too much inline:
large static payloads and repeated
create_object/create_charge_item_definition/create_resource_category calls make
it unreadable and hard to maintain; refactor by extracting the static
dictionaries (e.g., code_ucum_*, code_hl7_*, code_snomed_*,
default_price_components) and each resource spec body (used to instantiate
BaseSpecimenDefinitionSpec, BaseObservationDefinitionSpec,
BaseActivityDefinitionSpec, charge definitions) into module-level declarative
constants or an external JSON/YAML data file, then iterate over those structures
to call create_object, create_charge_item_definition, and
create_resource_category; also split logic into small helpers like
build_specimen_definitions, build_observation_definitions,
build_activity_definitions to keep create_lab_definition_objects as
orchestration only and reference symbols such as create_lab_definition_objects,
BaseSpecimenDefinitionSpec, BaseObservationDefinitionSpec,
BaseActivityDefinitionSpec, create_charge_item_definition, and
create_resource_category when implementing the changes.
In `@care/emr/management/commands/load_fixtures.py`:
- Around line 79-88: The file write of the manifest using
Path(options["output_json"]).open and json.dump can raise IO errors after the DB
transaction commits; wrap that block in a try/except around the open/json.dump
call to catch OSError/IOError/Exception, and on failure call
self.stdout.write(self.style.ERROR(...)) (or self.stderr.write) to report a
clear message that the fixture creation succeeded but writing the JSON failed,
including the exception text and the target path (options["output_json"]);
ensure the original success message is only emitted when the dump completes
without exception.
In `@Makefile`:
- Line 112: Extract the hard-coded fixture manifest path into a Makefile
variable (suggest name FIXTURE_MANIFEST) declared near the top of the Makefile,
then replace the literal path occurrences in the targets that call the manage.py
commands (the line invoking docker compose exec backend ... migrate && ...
load_fixtures and the other target at line ~39) to reference
$(FIXTURE_MANIFEST); also update any relevant Makefile recipe comments and
ensure the Docker Compose service exposes that path (via a volume or copy step)
if Playwright on the host needs access.
🪄 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: 689c26bb-9e02-4c1d-9923-a95b6b90270f
📒 Files selected for processing (10)
Makefilecare/emr/management/commands/fixtures/__init__.pycare/emr/management/commands/fixtures/clinical.pycare/emr/management/commands/fixtures/facilities.pycare/emr/management/commands/fixtures/inventory.pycare/emr/management/commands/fixtures/organizations.pycare/emr/management/commands/fixtures/patients.pycare/emr/management/commands/fixtures/questionnaires.pycare/emr/management/commands/fixtures/users.pycare/emr/management/commands/load_fixtures.py
Comment on lines
+8
to
+30
| # Override the validate_valueset function to skip validation for fixtures | ||
| import care.emr.utils.valueset_coding_type # isort:skip | ||
|
|
||
| sys.modules["care.emr.utils.valueset_coding_type"].validate_valueset = ( | ||
| lambda _, __, code: code | ||
| ) | ||
|
|
||
|
|
||
| def safe_coordinate(self, center=None, radius=0.001): | ||
| with localcontext() as ctx: | ||
| ctx.prec = 10 | ||
| if center is None: | ||
| return Decimal( | ||
| str(self.generator.random.randint(-180000000, 180000000) / 1000000) | ||
| ).quantize(Decimal(".000001")) | ||
| center = float(center) | ||
| radius = float(radius) | ||
| geo = self.generator.random.uniform(center - radius, center + radius) | ||
| return Decimal(str(geo)).quantize(Decimal(".000001")) | ||
|
|
||
|
|
||
| # Monkey patching the coordinate method of Faker's GeoProvider as it conflicts with our Decimal precision settings | ||
| GeoProvider.coordinate = safe_coordinate |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file in question
fd -t f "__init__.py" care/emr/management/commands/fixtures/ | head -20Repository: ohcnetwork/care
Length of output: 108
🏁 Script executed:
# Get the full content of the file to see lines 8-30 and 96-118
cat -n "care/emr/management/commands/fixtures/__init__.py" | head -150Repository: ohcnetwork/care
Length of output: 5955
🏁 Script executed:
# Search for imports of this fixtures module to understand scope
rg "from.*fixtures.*import|import.*fixtures" --type pyRepository: ohcnetwork/care
Length of output: 829
🏁 Script executed:
# Search for test imports of fixtures module
rg "from.*fixtures.*import|import.*fixtures" tests/ --type py | head -30Repository: ohcnetwork/care
Length of output: 105
🏁 Script executed:
# Check if there are any comments or TODOs about import-time side effects
rg "side effect|monkey patch|global state" --type py -i | grep -v ".pyc"Repository: ohcnetwork/care
Length of output: 223
🏁 Script executed:
# Look for zero-price or free item scenarios in the codebase
rg "price.*=.*0|free.*item" --type py -B2 -A2 | head -50Repository: ohcnetwork/care
Length of output: 3522
Move fixture setup patches behind an explicit context manager.
Lines 9-31 apply global patches to validate_valueset and GeoProvider.coordinate at import time. Any code importing from this module—even to use just a helper function—gets these patches applied process-wide. Moving these to an explicit setup function (like the context manager shown) ensures the patches only activate when fixtures are actually being generated, not when this module is incidentally imported elsewhere.
Additionally, line 146's if price: check will skip setting price_components for both None and Decimal(0), which may be unintended if free items (price=0) should still have price component definitions.
♻️ Possible direction
+from contextlib import contextmanager
+
-# Override the validate_valueset function to skip validation for fixtures
-import care.emr.utils.valueset_coding_type # isort:skip
-
-sys.modules["care.emr.utils.valueset_coding_type"].validate_valueset = (
- lambda _, __, code: code
-)
-
-# Monkey patching the coordinate method of Faker's GeoProvider as it conflicts with our Decimal precision settings
-GeoProvider.coordinate = safe_coordinate
+@contextmanager
+def fixture_overrides():
+ import care.emr.utils.valueset_coding_type as valueset_coding_type # isort:skip
+
+ original_validate_valueset = valueset_coding_type.validate_valueset
+ original_coordinate = GeoProvider.coordinate
+ valueset_coding_type.validate_valueset = lambda _, __, code: code
+ GeoProvider.coordinate = safe_coordinate
+ try:
+ yield
+ finally:
+ valueset_coding_type.validate_valueset = original_validate_valueset
+ GeoProvider.coordinate = original_coordinate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/__init__.py` around lines 8 - 30,
Extract the import-time monkey patches into an explicit context manager (e.g.,
fixture_patches() implemented with contextlib.contextmanager) that, on enter,
replaces sys.modules["care.emr.utils.valueset_coding_type"].validate_valueset
with the lambda and sets GeoProvider.coordinate = safe_coordinate, and on exit
restores the original validate_valueset and GeoProvider.coordinate references;
keep the safe_coordinate function unchanged but move its definition next to the
new context manager so callers can import it without activating patches. Also
change the price conditional from a truthy check to an explicit None check
(replace "if price:" with "if price is not None") wherever price_components are
set so Decimal(0) prices still produce price_components. Ensure callers that
generate fixtures use the new fixture_patches() context manager when applying
these patches.
Comment on lines
+109
to
+112
| if price: | ||
| _data["price_components"] = [ | ||
| {"amount": price, "monetary_component_type": "base"} | ||
| ] |
Contributor
There was a problem hiding this comment.
Treat price=0 as a real value.
The truthiness check skips the base price component for valid zero-priced items, so this helper silently produces a different payload than the caller asked for.
🐛 Suggested fix
- if price:
+ if price is not None:
_data["price_components"] = [
{"amount": price, "monetary_component_type": "base"}
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/__init__.py` around lines 109 - 112,
The helper currently skips adding the "price_components" entry when price is 0
because it uses a truthiness check; change the conditional to test for absence
rather than falsiness (e.g., use "if price is not None" or equivalent) so that
price=0 is treated as a valid value and _data["price_components"] is populated
with {"amount": price, "monetary_component_type": "base"}; update the
conditional near the use of the local variable price and the assignment to
_data["price_components"] accordingly.
Comment on lines
+124
to
+130
| if supply_delivery.order.origin: | ||
| sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) | ||
| else: | ||
| sync_inventory_item( | ||
| location=supply_delivery.order.destination, | ||
| product=supply_delivery.supplied_inventory_item.product, | ||
| ) |
Contributor
There was a problem hiding this comment.
Sync the destination stock on transfer orders too.
When order.origin is set, this only recomputes the origin inventory item. sync_inventory_item() recalculates stock per location, so completed transfer orders leave the destination InventoryItem.net_content stale until something else happens to touch it.
🐛 Suggested fix
- if supply_delivery.order.origin:
- sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item)
- else:
+ if supply_delivery.order.origin:
+ sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item)
+ if supply_delivery.order.destination:
sync_inventory_item(
location=supply_delivery.order.destination,
product=supply_delivery.supplied_inventory_item.product,
)📝 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
| if supply_delivery.order.origin: | |
| sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) | |
| else: | |
| sync_inventory_item( | |
| location=supply_delivery.order.destination, | |
| product=supply_delivery.supplied_inventory_item.product, | |
| ) | |
| if supply_delivery.order.origin: | |
| sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) | |
| if supply_delivery.order.destination: | |
| sync_inventory_item( | |
| location=supply_delivery.order.destination, | |
| product=supply_delivery.supplied_inventory_item.product, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/inventory.py` around lines 124 - 130,
When a supply_delivery is for a transfer (supply_delivery.order.origin is
truthy) the code currently only calls
sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item)
which recomputes the origin side and leaves the destination
InventoryItem.net_content stale; update the branch so that for transfers you
call sync_inventory_item for both sides: keep the existing
sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) to
recompute the origin and also call
sync_inventory_item(location=supply_delivery.order.destination,
product=supply_delivery.supplied_inventory_item.product) to recompute the
destination InventoryItem.net_content.
Comment on lines
+24
to
+39
| patient_spec = PatientCreateSpec( | ||
| name=fake.name(), | ||
| gender=secrets.choice(list(GenderChoices)).value, | ||
| phone_number=generate_unique_indian_phone_number(), | ||
| emergency_phone_number=generate_unique_indian_phone_number(), | ||
| address=fake.address(), | ||
| permanent_address=fake.address(), | ||
| pincode=fake.random_int(min=100000, max=999999), | ||
| blood_group=secrets.choice(list(BloodGroupChoices)).value, | ||
| geo_organization=geo_organization.external_id, | ||
| date_of_birth=fake.date_of_birth(), | ||
| ) | ||
| patient = patient_spec.de_serialize() | ||
| patient.created_by = super_user | ||
| patient.updated_by = super_user | ||
| patient.save() |
Contributor
There was a problem hiding this comment.
Populate required patient identifiers here instead of assuming there are none.
PatientCreateSpec.validate_identifiers() enforces any required identifier configs on the instance. This helper always relies on the default empty identifiers list, so fixture loading starts failing as soon as an environment marks any patient identifier as required. Please either generate those identifiers here or let callers supply them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/patients.py` around lines 24 - 39, The
fixture fails because PatientCreateSpec.validate_identifiers() expects required
identifiers but the spec is created with the default empty identifiers list;
update the patient fixture to populate patient_spec.identifiers with the
required identifier entries before calling de_serialize/save. Inspect the
system's patient identifier configuration (via
PatientCreateSpec.validate_identifiers or your identifier config helper) and
either generate the required identifier objects (matching expected fields like
type/system/value) and assign them to patient_spec.identifiers, or accept
identifiers from callers and pass them into PatientCreateSpec on construction so
validate_identifiers passes. Ensure you reference PatientCreateSpec, its
identifiers attribute, and the validate_identifiers behavior when implementing
the change.
Comment on lines
+17
to
+18
| with Path.open("data/questionnaire_fixtures.json") as f: | ||
| questionnaires = json.load(f) |
Contributor
There was a problem hiding this comment.
Resolve the questionnaire fixture path independently of the caller’s CWD.
This only works when the process starts from the repo root. The PR explicitly makes these helpers importable from tests/scripts, so calling this from anywhere else will raise FileNotFoundError for data/questionnaire_fixtures.json.
🐛 Suggested fix
import json
-from pathlib import Path
+
+from django.conf import settings
...
def create_questionnaires(facility, super_user):
- with Path.open("data/questionnaire_fixtures.json") as f:
+ fixtures_path = settings.BASE_DIR / "data" / "questionnaire_fixtures.json"
+ with fixtures_path.open() as f:
questionnaires = json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/questionnaires.py` around lines 17 -
18, The code opens "data/questionnaire_fixtures.json" relative to the current
working directory; change it to compute the fixture path relative to the module
file so imports work from anywhere: use
Path(__file__).resolve().parent.joinpath("data", "questionnaire_fixtures.json")
(or equivalent) and open that path when loading JSON (replace the current
Path.open call). Target the module-level loader where the code calls Path.open
and adjust to use the computed fixture_path.
Comment on lines
+69
to
+75
| attach_role_organization_user( | ||
| organization=Organization.objects.get( | ||
| name=role.name, org_type=OrganizationTypeChoices.role | ||
| ), | ||
| user=user, | ||
| role=role, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don’t assume the role organization was seeded earlier.
Organization.objects.get(name=role.name, org_type=OrganizationTypeChoices.role) makes create_user() crash unless callers happened to run create_role_organizations() first. Now that these helpers are public, that ordering dependency is a bit too easy to trip over.
🐛 Suggested fix
- attach_role_organization_user(
- organization=Organization.objects.get(
- name=role.name, org_type=OrganizationTypeChoices.role
- ),
+ role_organization, _ = Organization.objects.get_or_create(
+ name=role.name,
+ org_type=OrganizationTypeChoices.role,
+ defaults={"created_by": super_user, "updated_by": super_user},
+ )
+ attach_role_organization_user(
+ organization=role_organization,
user=user,
role=role,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/users.py` around lines 69 - 75, The
call in create_user() that uses Organization.objects.get(name=role.name,
org_type=OrganizationTypeChoices.role) can raise DoesNotExist if
create_role_organizations() wasn't run; change this to ensure the role
organization exists before attaching by using a safe creation pattern (e.g.
Organization.objects.get_or_create(...) or try/except DoesNotExist and create)
when preparing the organization for attach_role_organization_user; update the
logic in the create_user()/attach_role_organization_user flow to reference
Organization with org_type=OrganizationTypeChoices.role and name=role.name so
the org is created if missing.
Comment on lines
+131
to
+153
| for i in range(count): | ||
| password = default_password or fake.password( | ||
| length=10, special_chars=False | ||
| ) | ||
| username = ( | ||
| f"{role_name.lower()}_{facility_organization.id}_{i}".replace( | ||
| " ", "_" | ||
| ) | ||
| ) | ||
|
|
||
| create_user( | ||
| fake, | ||
| username=username, | ||
| user_type=user_type, | ||
| super_user=super_user, | ||
| geo_organization=geo_organization, | ||
| facility_organization=facility_organization, | ||
| role=role, | ||
| password=password, | ||
| ) | ||
| created_users.append( | ||
| {"username": username, "password": password, "role": role_name} | ||
| ) |
Contributor
There was a problem hiding this comment.
Skip existing generated usernames before recreating them.
These usernames are deterministic per facility/role/index, so rerunning this helper on the same facility will just hit the username uniqueness constraint instead of behaving like the rest of the fixture loaders.
🐛 Suggested fix
username = (
f"{role_name.lower()}_{facility_organization.id}_{i}".replace(
" ", "_"
)
)
+ if User.objects.filter(username=username).exists():
+ continue
create_user(📝 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
| for i in range(count): | |
| password = default_password or fake.password( | |
| length=10, special_chars=False | |
| ) | |
| username = ( | |
| f"{role_name.lower()}_{facility_organization.id}_{i}".replace( | |
| " ", "_" | |
| ) | |
| ) | |
| create_user( | |
| fake, | |
| username=username, | |
| user_type=user_type, | |
| super_user=super_user, | |
| geo_organization=geo_organization, | |
| facility_organization=facility_organization, | |
| role=role, | |
| password=password, | |
| ) | |
| created_users.append( | |
| {"username": username, "password": password, "role": role_name} | |
| ) | |
| for i in range(count): | |
| password = default_password or fake.password( | |
| length=10, special_chars=False | |
| ) | |
| username = ( | |
| f"{role_name.lower()}_{facility_organization.id}_{i}".replace( | |
| " ", "_" | |
| ) | |
| ) | |
| if User.objects.filter(username=username).exists(): | |
| continue | |
| create_user( | |
| fake, | |
| username=username, | |
| user_type=user_type, | |
| super_user=super_user, | |
| geo_organization=geo_organization, | |
| facility_organization=facility_organization, | |
| role=role, | |
| password=password, | |
| ) | |
| created_users.append( | |
| {"username": username, "password": password, "role": role_name} | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/users.py` around lines 131 - 153, The
loop in the fixture generator builds deterministic usernames and then always
calls create_user, which fails on rerun due to uniqueness constraints; before
calling create_user (in the loop that builds username, role and password) check
whether a user with that username already exists (e.g., via
User.objects.filter(username=username).exists() or the project's user lookup
helper) and skip to the next iteration if found, only calling create_user and
appending to created_users when the username does not already exist.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/management/commands/load_fixtures.py (1)
58-80:⚠️ Potential issue | 🟡 MinorDon't report success before the manifest write succeeds.
When
--output-jsonpoints at a bad path, the success banner is already printed and the transaction commits before the file write even runs. That leaves fixtures created but no manifest, which is a pretty awkward half-success. Move the success log after the optional dump or include the dump in the same success path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/load_fixtures.py` around lines 58 - 80, The success message is printed immediately after _generate_fixtures() and before the optional manifest file write, so if options["output_json"] fails you get a committed transaction with no manifest; fix by deferring the success log until after the optional write (or by performing the JSON dump inside the same transaction.atomic() block), i.e., ensure that manifest and the Path(options["output_json"]).open write happen before calling self.stdout.write(self.style.SUCCESS(...)) and reference the manifest variable, _generate_fixtures, transaction.atomic(), and options["output_json"] when relocating the success message.
♻️ Duplicate comments (8)
care/emr/management/commands/fixtures/users.py (3)
131-153:⚠️ Potential issue | 🟠 MajorSkip existing generated usernames before recreating them.
These usernames are deterministic for a given
facility_organization/role/index, so rerunning this helper just walks into the uniqueness constraint. A small existence check keeps it reusable without drama.Possible fix
username = ( f"{role_name.lower()}_{facility_organization.id}_{i}".replace( " ", "_" ) ) + if User.objects.filter(username=username).exists(): + continue create_user(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/users.py` around lines 131 - 153, The loop currently always calls create_user with a deterministic username and hits uniqueness errors on reruns; before calling create_user (in the for i in range(count) block where username is built), check whether a user with that username already exists and skip creation if so, only appending existing username/password/role to created_users when appropriate; reference the username generation, create_user invocation, and created_users list to place the existence check (e.g., query the User model or the function that looks up users by username) and continue to the next iteration if found.
69-75:⚠️ Potential issue | 🟠 MajorDon't assume the role organization was seeded earlier.
Organization.objects.get(...)makescreate_user()crash unless some other setup happened first. Since this helper is now public, it should fetch-or-create the role organization itself rather than exploding here.Possible fix
- attach_role_organization_user( - organization=Organization.objects.get( - name=role.name, org_type=OrganizationTypeChoices.role - ), + role_organization, _ = Organization.objects.get_or_create( + name=role.name, + org_type=OrganizationTypeChoices.role, + defaults={"created_by": super_user, "updated_by": super_user}, + ) + attach_role_organization_user( + organization=role_organization, user=user, role=role, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/users.py` around lines 69 - 75, The call to Organization.objects.get(...) inside attach_role_organization_user can raise if the role organization wasn't seeded; change it to fetch-or-create the role org instead: use Organization.objects.get_or_create(name=role.name, org_type=OrganizationTypeChoices.role, defaults={...}) (or call Organization.objects.get_or_create with appropriate defaults) before calling attach_role_organization_user so the organization is guaranteed to exist; update the code around attach_role_organization_user and create_user to call or inline this get_or_create logic and ensure Organization and OrganizationTypeChoices symbols are imported/available.
109-110:⚠️ Potential issue | 🟠 MajorKeep passwords out of the manifest.
--output-jsonis supposed to export fixture handles, but these structures serialize every generated password in plaintext. Foradmin, the manifest can even be wrong when the user already existed and you intentionally skipped resetting the password. Please omit or redact passwords fromctx.manifestand keep them only in transient console output for newly created accounts.Also applies to: 151-153, 175-188, 227-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/users.py` around lines 109 - 110, The manifest currently stores plaintext passwords (e.g., in created_users entries appended to ctx.manifest), so stop serializing passwords: remove the "password" field (or replace it with a redacted flag like "password_redacted": true) from any manifest entries you add (see uses of created_users.append and where ctx.manifest is populated, including the admin branch that may skip resets). Keep printing/generated passwords only to transient console output for newly created accounts, and ensure existing-user branches never write a password into ctx.manifest.care/emr/management/commands/fixtures/__init__.py (2)
146-149:⚠️ Potential issue | 🟡 MinorTreat
price=0as a real price.The truthiness check skips
price_componentsfor valid zero-priced items, which is a little less helpful than advertised. Check forNoneinstead.Possible fix
- if price: + if price is not None: _data["price_components"] = [ {"amount": price, "monetary_component_type": "base"} ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/__init__.py` around lines 146 - 149, The current truthy check "if price:" skips valid zero values; change the condition to explicitly test for None (e.g., "if price is not None:") so _data["price_components"] is set for price=0; update the block that assigns _data["price_components"] (the price variable and _data dict in this snippet) accordingly.
9-31:⚠️ Potential issue | 🟠 MajorMove the monkey patches behind an explicit fixture-only scope.
Importing this module immediately disables valueset validation and replaces
GeoProvider.coordinateprocess-wide.load_fixtures.pyimports it beforesync_valueset(), so the override leaks into command setup as well. Please apply these patches inside an explicit context manager around fixture generation instead of at import time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/__init__.py` around lines 9 - 31, Currently this module performs global monkey patches at import: it replaces care.emr.utils.valueset_coding_type.validate_valueset and assigns GeoProvider.coordinate = safe_coordinate, which leaks into non-fixture code paths; change this so the patches are applied only inside an explicit fixture-only context manager (e.g., a FixturePatches or patch_fixtures_context) used by the fixture runner (the code that calls load_fixtures.py), move the validate_valueset override and the GeoProvider.coordinate assignment into that context manager’s enter/exit so the original functions are restored on exit, and update the fixture invocation to wrap fixture generation with that context (references: validate_valueset, safe_coordinate, GeoProvider.coordinate, and the fixture module that currently imports this file).care/emr/management/commands/fixtures/questionnaires.py (1)
22-24:⚠️ Potential issue | 🟠 MajorResolve the questionnaire fixture path independently of the caller's CWD.
This only works when the process starts from the repo root. These helpers are now importable from tests/scripts, so a CWD-relative open will fail from anywhere else.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/questionnaires.py` around lines 22 - 24, The function create_questionnaires opens "data/questionnaire_fixtures.json" relative to the current working directory, which breaks when imported from other locations; change the path resolution to use the module's directory instead (use Path(__file__).parent.joinpath("data","questionnaire_fixtures.json") or equivalent) and open that resolved path so create_questionnaires always loads the fixture file relative to its module rather than the caller's CWD.care/emr/management/commands/fixtures/inventory.py (1)
124-130:⚠️ Potential issue | 🟠 MajorRecompute the destination stock for transfers too.
In the transfer branch you only sync the origin-side inventory item. The destination
InventoryItem.net_contentstays stale until something else touches it, which is a bit annoying for a fixture that's supposed to leave the inventory ready to use.Possible fix
- if supply_delivery.order.origin: - sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) - else: + if supply_delivery.order.origin: + sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) + if supply_delivery.order.destination: sync_inventory_item( location=supply_delivery.order.destination, product=supply_delivery.supplied_inventory_item.product, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/inventory.py` around lines 124 - 130, Currently transfers (when supply_delivery.order.origin is truthy) only call sync_inventory_item for the origin-side inventory_item, leaving the destination InventoryItem.net_content stale; update the transfer branch to call sync_inventory_item for both sides: keep the existing sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) for the origin, and add a second call to sync the destination (pass location=supply_delivery.order.destination and product=supply_delivery.supplied_inventory_item.product or the destination inventory_item if available) so the destination InventoryItem.net_content is recomputed as well.care/emr/management/commands/fixtures/patients.py (1)
24-39:⚠️ Potential issue | 🟠 MajorPopulate required patient identifiers before
de_serialize().This helper still relies on the default empty
identifierslist. If any patient identifier config is required, fixture loading fails here instead of creating patients. Either build the required identifiers in this helper or let callers supply them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/management/commands/fixtures/patients.py` around lines 24 - 39, patient_spec currently calls de_serialize() while its identifiers list is empty, causing fixture creation to fail when patient identifier configs are required; before calling PatientCreateSpec.de_serialize(), populate patient_spec.identifiers with the required identifier entries (e.g., query the patient identifier config model for required identifiers and append dicts with identifier type/key and a generated unique value using your existing generator helpers), then call patient_spec.de_serialize(), set created_by/updated_by and save; ensure the identifiers structure matches what PatientCreateSpec expects so required identifiers are present at de_serialize() time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/management/commands/fixtures/__init__.py`:
- Around line 99-103: The code currently overwrites any provided slug by always
setting obj.slug = obj.calculate_slug(); update the logic in create_object (the
function containing the loop over kwargs and the hasAttribute check) to only
call obj.calculate_slug() when no slug was provided by the caller (i.e., when
'slug' is not a key in kwargs or kwargs.get('slug') is falsy); keep applying
kwargs via setattr(obj, key, value) and then, if hasattr(obj, "calculate_slug")
and 'slug' not supplied, set obj.slug = obj.calculate_slug() before calling
obj.save().
In `@care/emr/management/commands/fixtures/inventory.py`:
- Around line 87-98: create_delivery_order and create_supply_delivery currently
assume non-None related objects (they dereference destination and order) even
though their parameters default to None; update these helpers to either require
the related args or validate and fail fast: add explicit checks at the start of
create_delivery_order to raise a ValueError if both origin and destination are
None (or if destination is required, raise when destination is None) before
accessing destination.created_by, and in create_supply_delivery raise a
ValueError if order is None (or make order a required parameter) so the function
doesn't unconditionally dereference order; reference the create_delivery_order
and create_supply_delivery functions when making the changes.
In `@care/emr/management/commands/fixtures/questionnaires.py`:
- Around line 34-54: The loop currently bails when a Questionnaire with slug
questionnaire_slug exists, which skips attaching the current
facility_organizations and role links; instead, fetch the existing
QuestionnaireSpec (or Questionnaire) by slug when
Questionnaire.objects.filter(slug=questionnaire_slug).exists() is true, reuse
that instance (e.g., questionnaire_spec =
QuestionnaireSpec.objects.get(slug=questionnaire_slug) or deserialize and map to
it), ensure its organizations/tags include facility_organizations and tags = [],
update its updated_by to super_user (preserve created_by), save the
questionnaire_spec, and then create any missing QuestionnaireOrganization rows
for each role in roles (only create if not already present) so associations are
idempotent on repeated fixture runs while avoiding duplicate questionnaire rows.
---
Outside diff comments:
In `@care/emr/management/commands/load_fixtures.py`:
- Around line 58-80: The success message is printed immediately after
_generate_fixtures() and before the optional manifest file write, so if
options["output_json"] fails you get a committed transaction with no manifest;
fix by deferring the success log until after the optional write (or by
performing the JSON dump inside the same transaction.atomic() block), i.e.,
ensure that manifest and the Path(options["output_json"]).open write happen
before calling self.stdout.write(self.style.SUCCESS(...)) and reference the
manifest variable, _generate_fixtures, transaction.atomic(), and
options["output_json"] when relocating the success message.
---
Duplicate comments:
In `@care/emr/management/commands/fixtures/__init__.py`:
- Around line 146-149: The current truthy check "if price:" skips valid zero
values; change the condition to explicitly test for None (e.g., "if price is not
None:") so _data["price_components"] is set for price=0; update the block that
assigns _data["price_components"] (the price variable and _data dict in this
snippet) accordingly.
- Around line 9-31: Currently this module performs global monkey patches at
import: it replaces care.emr.utils.valueset_coding_type.validate_valueset and
assigns GeoProvider.coordinate = safe_coordinate, which leaks into non-fixture
code paths; change this so the patches are applied only inside an explicit
fixture-only context manager (e.g., a FixturePatches or patch_fixtures_context)
used by the fixture runner (the code that calls load_fixtures.py), move the
validate_valueset override and the GeoProvider.coordinate assignment into that
context manager’s enter/exit so the original functions are restored on exit, and
update the fixture invocation to wrap fixture generation with that context
(references: validate_valueset, safe_coordinate, GeoProvider.coordinate, and the
fixture module that currently imports this file).
In `@care/emr/management/commands/fixtures/inventory.py`:
- Around line 124-130: Currently transfers (when supply_delivery.order.origin is
truthy) only call sync_inventory_item for the origin-side inventory_item,
leaving the destination InventoryItem.net_content stale; update the transfer
branch to call sync_inventory_item for both sides: keep the existing
sync_inventory_item(inventory_item=supply_delivery.supplied_inventory_item) for
the origin, and add a second call to sync the destination (pass
location=supply_delivery.order.destination and
product=supply_delivery.supplied_inventory_item.product or the destination
inventory_item if available) so the destination InventoryItem.net_content is
recomputed as well.
In `@care/emr/management/commands/fixtures/patients.py`:
- Around line 24-39: patient_spec currently calls de_serialize() while its
identifiers list is empty, causing fixture creation to fail when patient
identifier configs are required; before calling
PatientCreateSpec.de_serialize(), populate patient_spec.identifiers with the
required identifier entries (e.g., query the patient identifier config model for
required identifiers and append dicts with identifier type/key and a generated
unique value using your existing generator helpers), then call
patient_spec.de_serialize(), set created_by/updated_by and save; ensure the
identifiers structure matches what PatientCreateSpec expects so required
identifiers are present at de_serialize() time.
In `@care/emr/management/commands/fixtures/questionnaires.py`:
- Around line 22-24: The function create_questionnaires opens
"data/questionnaire_fixtures.json" relative to the current working directory,
which breaks when imported from other locations; change the path resolution to
use the module's directory instead (use
Path(__file__).parent.joinpath("data","questionnaire_fixtures.json") or
equivalent) and open that resolved path so create_questionnaires always loads
the fixture file relative to its module rather than the caller's CWD.
In `@care/emr/management/commands/fixtures/users.py`:
- Around line 131-153: The loop currently always calls create_user with a
deterministic username and hits uniqueness errors on reruns; before calling
create_user (in the for i in range(count) block where username is built), check
whether a user with that username already exists and skip creation if so, only
appending existing username/password/role to created_users when appropriate;
reference the username generation, create_user invocation, and created_users
list to place the existence check (e.g., query the User model or the function
that looks up users by username) and continue to the next iteration if found.
- Around line 69-75: The call to Organization.objects.get(...) inside
attach_role_organization_user can raise if the role organization wasn't seeded;
change it to fetch-or-create the role org instead: use
Organization.objects.get_or_create(name=role.name,
org_type=OrganizationTypeChoices.role, defaults={...}) (or call
Organization.objects.get_or_create with appropriate defaults) before calling
attach_role_organization_user so the organization is guaranteed to exist; update
the code around attach_role_organization_user and create_user to call or inline
this get_or_create logic and ensure Organization and OrganizationTypeChoices
symbols are imported/available.
- Around line 109-110: The manifest currently stores plaintext passwords (e.g.,
in created_users entries appended to ctx.manifest), so stop serializing
passwords: remove the "password" field (or replace it with a redacted flag like
"password_redacted": true) from any manifest entries you add (see uses of
created_users.append and where ctx.manifest is populated, including the admin
branch that may skip resets). Keep printing/generated passwords only to
transient console output for newly created accounts, and ensure existing-user
branches never write a password into ctx.manifest.
🪄 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: 0b065ea8-0834-4237-9f4e-8d22fda545d3
📒 Files selected for processing (9)
care/emr/management/commands/fixtures/__init__.pycare/emr/management/commands/fixtures/clinical.pycare/emr/management/commands/fixtures/facilities.pycare/emr/management/commands/fixtures/inventory.pycare/emr/management/commands/fixtures/organizations.pycare/emr/management/commands/fixtures/patients.pycare/emr/management/commands/fixtures/questionnaires.pycare/emr/management/commands/fixtures/users.pycare/emr/management/commands/load_fixtures.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/management/commands/fixtures/organizations.py
Comment on lines
+99
to
+103
| for key, value in kwargs.items(): | ||
| setattr(obj, key, value) | ||
| if hasattr(obj, "calculate_slug"): | ||
| obj.slug = obj.calculate_slug() | ||
| obj.save() |
Contributor
There was a problem hiding this comment.
Don't recalculate slug when the caller already provided one.
create_object() applies kwargs and then overwrites slug with calculate_slug(). Callers in fixtures/clinical.py pass explicit slugs, so those identifiers are silently ignored here.
Possible fix
- if hasattr(obj, "calculate_slug"):
+ if "slug" not in kwargs and hasattr(obj, "calculate_slug"):
obj.slug = obj.calculate_slug()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/__init__.py` around lines 99 - 103, The
code currently overwrites any provided slug by always setting obj.slug =
obj.calculate_slug(); update the logic in create_object (the function containing
the loop over kwargs and the hasAttribute check) to only call
obj.calculate_slug() when no slug was provided by the caller (i.e., when 'slug'
is not a key in kwargs or kwargs.get('slug') is falsy); keep applying kwargs via
setattr(obj, key, value) and then, if hasattr(obj, "calculate_slug") and 'slug'
not supplied, set obj.slug = obj.calculate_slug() before calling obj.save().
Comment on lines
+87
to
+98
| def create_delivery_order( | ||
| status=None, origin=None, destination=None, supplier=None, name=None | ||
| ): | ||
| return DeliveryOrder.objects.create( | ||
| status=status or "in_progress", | ||
| origin=origin, | ||
| destination=destination, | ||
| supplier=supplier, | ||
| name=name, | ||
| created_by=origin.created_by if origin else destination.created_by, | ||
| updated_by=origin.created_by if origin else destination.created_by, | ||
| ) |
Contributor
There was a problem hiding this comment.
These "optional" relations aren't actually optional.
create_delivery_order() dereferences destination when origin is missing, and create_supply_delivery() dereferences order unconditionally even though both parameters default to None. Since these helpers are now part of the reusable fixture API, make those args required or fail fast with a clear ValueError.
Also applies to: 109-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/inventory.py` around lines 87 - 98,
create_delivery_order and create_supply_delivery currently assume non-None
related objects (they dereference destination and order) even though their
parameters default to None; update these helpers to either require the related
args or validate and fail fast: add explicit checks at the start of
create_delivery_order to raise a ValueError if both origin and destination are
None (or if destination is required, raise when destination is None) before
accessing destination.created_by, and in create_supply_delivery raise a
ValueError if order is None (or make order a required parameter) so the function
doesn't unconditionally dereference order; reference the create_delivery_order
and create_supply_delivery functions when making the changes.
Comment on lines
+34
to
+54
| for questionnaire in questionnaires: | ||
| questionnaire_slug = questionnaire["slug"] | ||
| if Questionnaire.objects.filter(slug=questionnaire_slug).exists(): | ||
| continue | ||
|
|
||
| questionnaire["organizations"] = facility_organizations | ||
| questionnaire["tags"] = [] | ||
|
|
||
| questionnaire_spec = QuestionnaireSpec(**questionnaire) | ||
|
|
||
| questionnaire_spec = questionnaire_spec.de_serialize() | ||
|
|
||
| questionnaire_spec.created_by = super_user | ||
| questionnaire_spec.updated_by = super_user | ||
| questionnaire_spec.save() | ||
|
|
||
| for role in roles: | ||
| QuestionnaireOrganization.objects.create( | ||
| questionnaire=questionnaire_spec, | ||
| organization=role, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't skip the facility links when the slug already exists.
The continue prevents duplicate rows, but it also skips attaching the current facility's organizations and role links. On a later fixture run, the new facility ends up without these questionnaires at all. Reuse the existing questionnaire and ensure its associations are up to date instead of bailing out early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/management/commands/fixtures/questionnaires.py` around lines 34 -
54, The loop currently bails when a Questionnaire with slug questionnaire_slug
exists, which skips attaching the current facility_organizations and role links;
instead, fetch the existing QuestionnaireSpec (or Questionnaire) by slug when
Questionnaire.objects.filter(slug=questionnaire_slug).exists() is true, reuse
that instance (e.g., questionnaire_spec =
QuestionnaireSpec.objects.get(slug=questionnaire_slug) or deserialize and map to
it), ensure its organizations/tags include facility_organizations and tags = [],
update its updated_by to super_user (preserve created_by), save the
questionnaire_spec, and then create any missing QuestionnaireOrganization rows
for each role in roles (only create if not already present) so associations are
idempotent on repeated fixture runs while avoiding duplicate questionnaire rows.
Greptile SummaryThis PR successfully decomposes the monolithic Key observations:
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+38
to
+42
| "Staff": "staff", | ||
| "Nurse": "nurse", | ||
| "Administrator": "administrator", | ||
| "Facility Admin": "administrator", | ||
| } |
There was a problem hiding this comment.
Missing # noqa on side-effectful import
The original load_fixtures.py had # noqa # isort:skip on this import, but the refactored version dropped the # noqa. Since this is a bare import used purely for its side effect (the sys.modules mutation on the lines that follow), most linters (flake8/ruff F401) will flag it as an unused import and may fail CI.
Suggested change
| "Staff": "staff", | |
| "Nurse": "nurse", | |
| "Administrator": "administrator", | |
| "Facility Admin": "administrator", | |
| } | |
| import care.emr.utils.valueset_coding_type # noqa: F401 # isort:skip |
Comment on lines
+380
to
+434
| "max": 99, | ||
| }, | ||
| {"interpretation": {"display": "High"}, "min": 100}, | ||
| ], | ||
| } | ||
| ], | ||
| ), | ||
| facility, | ||
| user, | ||
| slug="fasting_blood_glucose", | ||
| ) | ||
| cbc_observation_definition = create_object( | ||
| BaseObservationDefinitionSpec( | ||
| title="Complete Blood Count", | ||
| status="active", | ||
| description="A Complete Blood Count (CBC) is a common laboratory test that evaluates the overall health status by measuring multiple components of blood including red blood cells (RBC), white blood cells (WBC), hemoglobin, hematocrit, and platelets. This test is performed on whole blood using an automated hematology analyzer.", | ||
| category="laboratory", | ||
| code=code_loinc_cbc_panel, | ||
| permitted_data_type="quantity", | ||
| component=[ | ||
| { | ||
| "code": code_loinc_hemoglobin, | ||
| "permitted_unit": code_ucum_g_dl, | ||
| "permitted_data_type": "quantity", | ||
| "qualified_ranges": [ | ||
| { | ||
| "conditions": [], | ||
| "ranges": [ | ||
| {"interpretation": {"display": "Low"}, "max": 12}, | ||
| { | ||
| "interpretation": {"display": "Normal"}, | ||
| "min": 12, | ||
| "max": 16, | ||
| }, | ||
| {"interpretation": {"display": "High"}, "min": 16}, | ||
| ], | ||
| }, | ||
| { | ||
| "conditions": [], | ||
| "ranges": [ | ||
| {"interpretation": {"display": "Low"}, "max": 14}, | ||
| { | ||
| "interpretation": {"display": "Normal"}, | ||
| "min": 14, | ||
| "max": 18, | ||
| }, | ||
| {"interpretation": {"display": "High"}, "min": 18}, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| "code": code_loinc_hematocrit, | ||
| "permitted_unit": code_ucum_percent, | ||
| "permitted_data_type": "quantity", |
There was a problem hiding this comment.
Duplicate LOINC code constant
code_loinc_fasting_glucose and code_loinc_fasting_glucose_serum (defined ~50 lines apart in the same function) are byte-for-byte identical — same LOINC code "1558-6", same system, and same display string. code_loinc_fasting_glucose_serum is used only in diagnostic_report_codes for the fasting-glucose activity definition. Having two differently-named constants with the same value is misleading and could cause confusion when making future edits.
Consider removing code_loinc_fasting_glucose_serum and reusing code_loinc_fasting_glucose where it appears:
Suggested change
| "max": 99, | |
| }, | |
| {"interpretation": {"display": "High"}, "min": 100}, | |
| ], | |
| } | |
| ], | |
| ), | |
| facility, | |
| user, | |
| slug="fasting_blood_glucose", | |
| ) | |
| cbc_observation_definition = create_object( | |
| BaseObservationDefinitionSpec( | |
| title="Complete Blood Count", | |
| status="active", | |
| description="A Complete Blood Count (CBC) is a common laboratory test that evaluates the overall health status by measuring multiple components of blood including red blood cells (RBC), white blood cells (WBC), hemoglobin, hematocrit, and platelets. This test is performed on whole blood using an automated hematology analyzer.", | |
| category="laboratory", | |
| code=code_loinc_cbc_panel, | |
| permitted_data_type="quantity", | |
| component=[ | |
| { | |
| "code": code_loinc_hemoglobin, | |
| "permitted_unit": code_ucum_g_dl, | |
| "permitted_data_type": "quantity", | |
| "qualified_ranges": [ | |
| { | |
| "conditions": [], | |
| "ranges": [ | |
| {"interpretation": {"display": "Low"}, "max": 12}, | |
| { | |
| "interpretation": {"display": "Normal"}, | |
| "min": 12, | |
| "max": 16, | |
| }, | |
| {"interpretation": {"display": "High"}, "min": 16}, | |
| ], | |
| }, | |
| { | |
| "conditions": [], | |
| "ranges": [ | |
| {"interpretation": {"display": "Low"}, "max": 14}, | |
| { | |
| "interpretation": {"display": "Normal"}, | |
| "min": 14, | |
| "max": 18, | |
| }, | |
| {"interpretation": {"display": "High"}, "min": 18}, | |
| ], | |
| }, | |
| ], | |
| }, | |
| { | |
| "code": code_loinc_hematocrit, | |
| "permitted_unit": code_ucum_percent, | |
| "permitted_data_type": "quantity", | |
| code_loinc_fasting_glucose = { | |
| "code": "1558-6", | |
| "system": "http://loinc.org", | |
| "display": "Fasting glucose [Mass/volume] in Serum or Plasma", | |
| } |
and update the activity-definition call to use diagnostic_report_codes=[code_loinc_fasting_glucose].
Comment on lines
+72
to
81
| if options["output_json"]: | ||
| from pathlib import Path | ||
|
|
||
| self.stdout.write("=" * 30) | ||
| if created: | ||
| self.stdout.write("Superuser username: admin") | ||
| self.stdout.write("Superuser password: admin") | ||
| else: | ||
| with Path(options["output_json"]).open("w") as f: | ||
| json.dump(manifest, f, indent=2) | ||
| self.stdout.write( | ||
| "Superuser 'admin' already exists, not creating a new one." | ||
| ) | ||
| self.stdout.write("=" * 30) | ||
|
|
||
| geo_organization = self._create_organization(fake, super_user) | ||
| self.geo_organization = geo_organization | ||
| self.stdout.write(f"Created geo organization: {geo_organization.name}") | ||
|
|
||
| # create some product suppliers | ||
| for _ in range(2): | ||
| self._create_organization( | ||
| fake, | ||
| super_user, | ||
| org_type=OrganizationTypeChoices.product_supplier, | ||
| name=f"Supplier {fake.company()}", | ||
| ) | ||
| self.supplier = self._create_organization( | ||
| fake, | ||
| super_user, | ||
| org_type=OrganizationTypeChoices.product_supplier, | ||
| name=f"Supplier {fake.company()}", | ||
| ) | ||
|
|
||
| facility = self._create_facility( | ||
| fake, super_user, geo_organization, "FACILITY WITH PATIENTS" | ||
| ) | ||
| self.stdout.write(f"Created facility: {facility.name}") | ||
|
|
||
| self._create_inventory_items(facility) | ||
| self.stdout.write("Created inventory items for facility") | ||
|
|
||
| external_facility_organization = self._create_facility_organization( | ||
| fake, super_user, facility | ||
| ) | ||
| self.stdout.write( | ||
| f"Created facility organization (dept): {external_facility_organization.name}" | ||
| ) | ||
|
|
||
| self._create_facility(fake, super_user, geo_organization) | ||
| self.stdout.write("Created resource facility") | ||
|
|
||
| location = self._create_location( | ||
| super_user, | ||
| facility, | ||
| [external_facility_organization], | ||
| mode="kind", | ||
| form="wa", | ||
| ) | ||
| self.stdout.write(f"Created location: {location.name}") | ||
|
|
||
| self._create_lab_definition_objects_for_facility(facility, super_user) | ||
| self.stdout.write("Created lab objects for facility") | ||
|
|
||
| for i in range(1, 6): | ||
| bed = self._create_location( | ||
| super_user, | ||
| facility, | ||
| [external_facility_organization], | ||
| mode="instance", | ||
| form="bd", | ||
| parent=location.external_id, | ||
| name=f"Bed {i}", | ||
| ) | ||
| self.stdout.write(f"Created bed: {bed.name}") | ||
|
|
||
| for i in range(1, 6): | ||
| device = self._create_device( | ||
| fake, | ||
| super_user, | ||
| external_facility_organization, | ||
| name=f"Device {i}", | ||
| ) | ||
| self.stdout.write(f"Created device: {device.user_friendly_name}") | ||
|
|
||
| organizations = self._create_role_organizations(fake, super_user) | ||
|
|
||
| for organization in organizations: | ||
| self.stdout.write(f"Created organization: {organization.name}") | ||
|
|
||
| self._create_default_users(fake, super_user, external_facility_organization) | ||
|
|
||
| self._create_facility_users( | ||
| fake, | ||
| super_user, | ||
| external_facility_organization, | ||
| options["users"], | ||
| options["default_password"], | ||
| ) | ||
|
|
||
| patients = self._create_patients( | ||
| fake, super_user, geo_organization, options["patients"] | ||
| ) | ||
|
|
||
| self._create_encounters( | ||
| fake, | ||
| super_user, | ||
| patients, | ||
| facility, | ||
| [external_facility_organization], | ||
| options["encounter"], | ||
| ) | ||
|
|
||
| self._create_questionnaires(facility, super_user) | ||
|
|
||
| def _create_organization(self, fake, super_user, **kwargs): | ||
| data = { | ||
| "active": True, | ||
| "org_type": OrganizationTypeChoices.govt, | ||
| "name": fake.state(), | ||
| } | ||
| if kwargs: | ||
| data.update(kwargs) | ||
|
|
||
| org_spec = OrganizationWriteSpec(**data) | ||
| org = org_spec.de_serialize() | ||
| org.created_by = super_user | ||
| org.updated_by = super_user | ||
| org.save() | ||
| return org | ||
|
|
||
| def _create_facility(self, fake, super_user, geo_organization, name=None): | ||
| facility_spec = FacilityCreateSpec( | ||
| geo_organization=geo_organization.external_id, | ||
| name=name or fake.company(), | ||
| description=fake.text(max_nb_chars=200), | ||
| longitude=float(fake.longitude()), | ||
| latitude=float(fake.latitude()), | ||
| pincode=fake.random_int(min=100000, max=999999), | ||
| address=fake.address(), | ||
| phone_number=generate_unique_indian_phone_number(), | ||
| middleware_address=fake.address(), | ||
| facility_type="Private Hospital", | ||
| is_public=True, | ||
| features=[1], | ||
| ) | ||
| facility = facility_spec.de_serialize() | ||
| facility.created_by = super_user | ||
| facility.updated_by = super_user | ||
| facility.save() | ||
| return facility | ||
|
|
||
| def _create_facility_organization(self, fake, super_user, facility): | ||
| org_spec = FacilityOrganizationWriteSpec( | ||
| active=True, | ||
| name=fake.company(), | ||
| description=fake.text(max_nb_chars=200), | ||
| facility=facility.external_id, | ||
| org_type=FacilityOrganizationTypeChoices.dept, | ||
| ) | ||
| org = org_spec.de_serialize() | ||
| org.created_by = super_user | ||
| org.updated_by = super_user | ||
| org.save() | ||
| return org | ||
|
|
||
| def _create_role_organizations(self, fake, super_user): | ||
| orgs = [] | ||
| for role_name in ROLES_OPTIONS: | ||
| if Organization.objects.filter( | ||
| name=role_name, org_type=OrganizationTypeChoices.role | ||
| ).exists(): | ||
| self.stdout.write( | ||
| self.style.WARNING( | ||
| f"Organization '{role_name}' already exists, skipping." | ||
| ) | ||
| ) | ||
| continue | ||
| org_spec = OrganizationWriteSpec( | ||
| active=True, org_type=OrganizationTypeChoices.role, name=role_name | ||
| ) | ||
| org = org_spec.de_serialize() | ||
| org.created_by = super_user | ||
| org.updated_by = super_user | ||
| org.save() | ||
| orgs.append(org) | ||
| return orgs | ||
|
|
||
| def _attach_role_organization_user(self, organization, user, role): | ||
| return OrganizationUser.objects.create( | ||
| organization=organization, user=user, role=role | ||
| ) | ||
|
|
||
| def _attach_role_facility_organization_user( | ||
| self, facility_organization, user, role | ||
| ): | ||
| return FacilityOrganizationUser.objects.create( | ||
| organization=facility_organization, user=user, role=role | ||
| ) | ||
|
|
||
| def _create_user( | ||
| self, | ||
| fake, | ||
| username, | ||
| user_type, | ||
| super_user, | ||
| facility_organization=None, | ||
| geo_organization=None, | ||
| role=None, | ||
| password=None, | ||
| ): | ||
| password = password or fake.password(length=10, special_chars=False) | ||
| user_spec = UserCreateSpec( | ||
| first_name=fake.first_name(), | ||
| last_name=fake.last_name(), | ||
| phone_number=generate_unique_indian_phone_number(), | ||
| prefix=fake.prefix(), | ||
| suffix=fake.suffix(), | ||
| gender=secrets.choice(list(GenderChoices)).value, | ||
| password=password, | ||
| username=username, | ||
| email=str(uuid.uuid4()) + fake.email(), | ||
| user_type=user_type, | ||
| ) | ||
| user = user_spec.de_serialize() | ||
| user.geo_organization = geo_organization or self.geo_organization | ||
| user.created_by = super_user | ||
| user.updated_by = super_user | ||
| user.save() | ||
|
|
||
| if role: | ||
| if facility_organization: | ||
| self._attach_role_facility_organization_user( | ||
| facility_organization=facility_organization, | ||
| user=user, | ||
| role=role, | ||
| ) | ||
| if ( | ||
| user.user_type == "administrator" | ||
| and facility_organization.facility.default_internal_organization | ||
| ): | ||
| self._attach_role_facility_organization_user( | ||
| facility_organization=facility_organization.facility.default_internal_organization, | ||
| user=user, | ||
| role=role, | ||
| ) | ||
| if user.geo_organization: | ||
| self._attach_role_organization_user( | ||
| organization=user.geo_organization, | ||
| user=user, | ||
| role=role, | ||
| ) | ||
| self._attach_role_organization_user( | ||
| organization=Organization.objects.get( | ||
| name=role.name, org_type=OrganizationTypeChoices.role | ||
| ), | ||
| user=user, | ||
| role=role, | ||
| ) | ||
|
|
||
| def _create_facility_users( | ||
| self, | ||
| fake, | ||
| super_user, | ||
| facility_organization, | ||
| count, | ||
| default_password=None, | ||
| ): | ||
| self.stdout.write("=" * 50) | ||
| self.stdout.write("USER CREDENTIALS") | ||
| self.stdout.write("=" * 50) | ||
| self.stdout.write(f"{'ROLE':<15} {'USERNAME':<30} {'PASSWORD':<20}") | ||
| self.stdout.write("-" * 65) | ||
|
|
||
| for role_name, user_type in ROLES_OPTIONS.items(): | ||
| try: | ||
| role = RoleModel.objects.get(name=role_name) | ||
|
|
||
| for i in range(count): | ||
| password = default_password or fake.password( | ||
| length=10, special_chars=False | ||
| ) | ||
| username = ( | ||
| f"{role_name.lower()}_{facility_organization.id}_{i}".replace( | ||
| " ", "_" | ||
| ) | ||
| ) | ||
|
|
||
| self._create_user( | ||
| fake, | ||
| username=username, | ||
| user_type=user_type, | ||
| super_user=super_user, | ||
| facility_organization=facility_organization, | ||
| role=role, | ||
| password=password, | ||
| ) | ||
|
|
||
| self.stdout.write(f"{role_name:<15} {username:<30} {password:<20}") | ||
|
|
||
| except RoleModel.DoesNotExist: | ||
| self.stdout.write( | ||
| self.style.WARNING(f"Role '{role_name}' not found, skipping.") | ||
| self.style.SUCCESS( | ||
| f"Fixture manifest written to {options['output_json']}" | ||
| ) | ||
|
|
||
| self.stdout.write("=" * 50) | ||
|
|
||
| def _create_default_users(self, fake, super_user, facility_organization): | ||
| fixed_users = [ | ||
| ("Doctor", "care-doctor"), | ||
| ("Staff", "care-staff"), | ||
| ("Nurse", "care-nurse"), | ||
| ("Administrator", "care-admin"), | ||
| ("Volunteer", "care-volunteer"), | ||
| ("Facility Admin", "care-fac-admin"), | ||
| ] | ||
|
|
||
| password = "Ohcn@123" | ||
| for role_name, username in fixed_users: | ||
| try: | ||
| role = RoleModel.objects.get(name=role_name) | ||
|
|
||
| if User.objects.filter(username=username).exists(): | ||
| self.stdout.write( | ||
| self.style.WARNING(f"User {username} already exists. Skipping.") | ||
| ) | ||
| continue | ||
|
|
||
| self._create_user( | ||
| fake, | ||
| username=username, | ||
| user_type=ROLES_OPTIONS[role_name], | ||
| super_user=super_user, | ||
| facility_organization=facility_organization, | ||
| role=role, | ||
| password=password, | ||
| ) | ||
|
|
||
| self.stdout.write(f"{role_name:<15} {username:<30} {password:<20}") | ||
| except RoleModel.DoesNotExist: | ||
| self.stdout.write( | ||
| self.style.WARNING(f"Role '{role_name}' not found, skipping.") | ||
| ) | ||
|
|
||
| def _create_patients( | ||
| self, fake, super_user, geo_organization, count | ||
| ) -> list[Patient]: | ||
| patients = [] | ||
| self.stdout.write(f"Creating {count} patients...") | ||
|
|
||
| for _ in range(count): | ||
| patient_spec = PatientCreateSpec( | ||
| name=fake.name(), | ||
| gender=secrets.choice(list(GenderChoices)).value, | ||
| phone_number=generate_unique_indian_phone_number(), | ||
| emergency_phone_number=generate_unique_indian_phone_number(), | ||
| address=fake.address(), | ||
| permanent_address=fake.address(), | ||
| pincode=fake.random_int(min=100000, max=999999), | ||
| blood_group=secrets.choice(list(BloodGroupChoices)).value, | ||
| geo_organization=geo_organization.external_id, | ||
| date_of_birth=fake.date_of_birth(), | ||
| ) |
There was a problem hiding this comment.
JSON manifest written outside the transaction
The manifest dict is populated inside transaction.atomic(), but the file is written after the with block exits (i.e., after the DB transaction has already been committed). If the Path(...).open("w") call or json.dump fails (e.g., permission error, disk full), the database rows are already persisted but no manifest file is produced. Depending on how automated pipelines consume this file, that silent mismatch could be confusing.
If the manifest must be consistent with the DB state, consider either:
- wrapping the file write in a try/except with a clear error message, or
- writing the manifest file before committing (though that would require a two-phase approach).
This is a low-severity concern for a development-only fixture command, but worth noting given the stated goal of "programmatic access" via the manifest.
Comment on lines
+121
to
+132
| "created_by": facility.created_by, | ||
| "updated_by": facility.created_by, | ||
| "resource_sub_type": "other", | ||
| } | ||
| if kwargs: | ||
| _data.update(kwargs) | ||
| resource_category = ResourceCategory(**_data) | ||
| resource_category.slug = resource_category.calculate_slug() | ||
| resource_category.save() | ||
| return resource_category | ||
|
|
||
|
|
There was a problem hiding this comment.
Misleading parameter name: model should be spec
The first parameter is named model, which in a Django context strongly implies a model class. However, the callers always pass an already-instantiated Spec object (e.g. BaseSpecimenDefinitionSpec(**kwargs)), and the function immediately calls .de_serialize() on it. Renaming it to spec would make the contract clearer and avoid confusion for future contributors.
Suggested change
| "created_by": facility.created_by, | |
| "updated_by": facility.created_by, | |
| "resource_sub_type": "other", | |
| } | |
| if kwargs: | |
| _data.update(kwargs) | |
| resource_category = ResourceCategory(**_data) | |
| resource_category.slug = resource_category.calculate_slug() | |
| resource_category.save() | |
| return resource_category | |
| def create_object(spec, facility, user=None, **kwargs): | |
| """Shared helper to deserialize a Spec, attach facility/user, and save.""" | |
| obj = spec.de_serialize() |
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.
Proposed Changes
load_fixtures.pymanagement command (1400+ lines) into a modular structure with separate fixture modules for better maintainability and testabilityfixtures/__init__.py: Shared utilities (Faker setup, phone number generation, object creation helpers)fixtures/organizations.py: Organization and facility organization creationfixtures/facilities.py: Facility, location, and device creationfixtures/users.py: User creation and role assignmentfixtures/patients.py: Patient and encounter creationfixtures/inventory.py: Inventory items, products, and supply delivery creationfixtures/clinical.py: Lab definitions, specimen definitions, observation definitions, etc.fixtures/questionnaires.py: Questionnaire creation--output-jsonflag to export a manifest of all created fixture IDs (external_id references) for programmatic access_generate_fixtures()to build and return a manifest dictionary containing IDs and metadata for all created resources--output-jsonflag when loading fixturesBenefits
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
https://claude.ai/code/session_011i8in4FLq4renAvgodeJJu
Summary by CodeRabbit
New Features
Refactor