Ce/audit generation#1156
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
calellowitz
requested review from
Charl1996,
ajeety4,
hemant10yadav,
mkangia,
pxwxnvermx,
sravfeyn and
zandre-eng
WalkthroughThis pull request introduces a complete audit reporting system as a new Django app ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
commcare_connect/audit/tests/test_services.py (1)
11-17: ⚡ Quick win
isolated_registryfixture duplicates manual_REGISTRYmanipulation intest_calculations.py— move toconftest.pyThis fixture implements the same save/clear/restore pattern that
test_calculations.py:test_register_calculation_appends_instancealso does manually (lines 65–81 of that file). As per coding guidelines: "Prefer fixtures over factories to avoid duplication. Checkconftest.pyfiles (global and per-app) for existing fixtures." Movingisolated_registrytocommcare_connect/audit/tests/conftest.pymakes it available to both test modules and removes the duplication.(See the companion comment on
test_calculations.pyfor the proposed refactor.)🤖 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 `@commcare_connect/audit/tests/test_services.py` around lines 11 - 17, Move the isolated_registry pytest fixture out of commcare_connect/audit/tests/test_services.py into commcare_connect/audit/tests/conftest.py so it can be reused; specifically, keep the same save/clear/restore logic that manipulates calculations._REGISTRY and remove the duplicate manual registry handling in test_calculations.py (the code in test_register_calculation_appends_instance) to use the shared isolated_registry fixture instead. Ensure the fixture name remains isolated_registry and that tests import nothing extra so pytest auto-discovers it from conftest.py.commcare_connect/audit/migrations/0002_periodic_task.py (1)
30-42: 💤 Low valueRuff RUF012 on
dependencies/operations— Django migration false positive; suppress if neededThe
dependenciesandoperationsclass-level lists always trigger RUF012 ("Mutable default value for class attribute") in Django migrations because they're not annotated withClassVar. This is a well-known Ruff false positive for the standardmigrations.Migrationpattern. If your CI enforces this rule, add# noqa: RUF012to each list or configure a per-path ignore for migration files.💡 One-line suppression option
- dependencies = [ + dependencies: ClassVar = [ # or add # noqa: RUF012 ("audit", "0001_initial"), ("django_celery_beat", "0001_initial"), ] - operations = [ + operations: ClassVar = [ # or add # noqa: RUF012🤖 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 `@commcare_connect/audit/migrations/0002_periodic_task.py` around lines 30 - 42, Ruff flags the class-level mutable lists in the Migration class (dependencies and operations) as RUF012; suppress it by adding a per-line noqa comment to each list (e.g., append "# noqa: RUF012" to the dependencies = [...] and operations = [...] declarations) or alternatively add a per-path/filename ignore for migration files in your Ruff config; target the Migration class and its dependencies and operations symbols when applying the suppression.commcare_connect/audit/tests/test_calculations.py (1)
64-81: ⚡ Quick winManual
_REGISTRYsave/clear/restore duplicates theisolated_registryfixture intest_services.py
test_register_calculation_appends_instancereplicates the same pattern asisolated_registryintest_services.py(save original → clear → manipulate → restore in finally). Per project coding guidelines, prefer fixtures over ad-hoc setup. Movingisolated_registryto the app-levelconftest.pywould let this test use it directly and eliminate the duplication.♻️ Proposed refactor
In
commcare_connect/audit/tests/conftest.py(create or add to existing):import pytest from commcare_connect.audit import calculations `@pytest.fixture` def isolated_registry(): original = list(calculations._REGISTRY) calculations._REGISTRY.clear() yield calculations._REGISTRY[:] = originalThen in
test_calculations.py:-def test_register_calculation_appends_instance(): - original = list(calculations._REGISTRY) - try: - calculations._REGISTRY.clear() - - `@register_calculation` - class Dummy(AuditCalculation): - ... - - registered = calculations.get_registered_calculations() - assert len(registered) == 1 - assert isinstance(registered[0], Dummy) - finally: - calculations._REGISTRY[:] = original +def test_register_calculation_appends_instance(isolated_registry): + `@register_calculation` + class Dummy(AuditCalculation): + ... + + registered = calculations.get_registered_calculations() + assert len(registered) == 1 + assert isinstance(registered[0], Dummy)🤖 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 `@commcare_connect/audit/tests/test_calculations.py` around lines 64 - 81, The test test_register_calculation_appends_instance duplicates manual save/clear/restore of calculations._REGISTRY; create a reusable pytest fixture named isolated_registry that saves original = list(calculations._REGISTRY), clears calculations._REGISTRY, yields, and restores it after the test, then change test_register_calculation_appends_instance to accept isolated_registry as a parameter and remove the try/finally block so the fixture handles setup/teardown for calculations._REGISTRY.commcare_connect/audit/tests/test_tasks.py (1)
9-9: ⚡ Quick win
AuditReportFactoryside-effect import should live inconftest.py, not a test moduleThe
# noqa: F401 (keeps factory importable)comment implies this import exists purely for its side effect (registering the factory with pytest or the factory-boy framework). Per coding guidelines, checkconftest.pyfiles for existing fixtures and prefer that location for registration-style imports so they apply across all tests in the app, not just this module.🤖 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 `@commcare_connect/audit/tests/test_tasks.py` at line 9, Move the side-effect import of AuditReportFactory out of the test module and into the package-level conftest.py so the factory registration applies globally; remove the line "from commcare_connect.audit.tests.factories import AuditReportFactory # noqa: F401" from commcare_connect/audit/tests/test_tasks.py and add an equivalent import statement in the app's conftest.py (or create one) to perform the import for side-effect registration instead.
🤖 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 `@commcare_connect/audit/calculations.py`:
- Around line 57-67: The run() method may call _in_range(None) if compute()
returns (None, sample_size) with sample_size >= min_sample_size, causing
TypeError; update run (around the compute/opportunity_access logic) to
explicitly guard for value is None after compute() and before calling _in_range:
when value is None, return a CalculationResult (same name/label) that sets
value=None, has_sufficient_data=False (or another explicit flag per existing
semantics) and in_range=True/False as appropriate so _in_range is never invoked
with None; reference the run, compute, _in_range, min_sample_size, and
CalculationResult symbols when making the change.
In `@commcare_connect/audit/services.py`:
- Around line 32-36: Replace the unconditional creation of AuditReport with an
idempotent lookup/create: use AuditReport.objects.get_or_create(...) keyed on
opportunity, period_start, and period_end instead of
AuditReport.objects.create(...). Update the surrounding logic that expects a
newly created instance (the variable report) to handle the (instance, created)
tuple returned by get_or_create and avoid duplicating reports when the task
retries or runs multiple times.
In `@commcare_connect/audit/tests/factories.py`:
- Line 25: AuditReportEntryFactory defines a mutable class-scope default results
= {} which is shared across instances; change the factory attribute to create a
new dict per instance by using factory.LazyFunction(dict) for the results field
(update the AuditReportEntryFactory definition to replace the class-scope
results = {} with results = factory.LazyFunction(dict) so each generated
instance gets its own dict).
---
Nitpick comments:
In `@commcare_connect/audit/migrations/0002_periodic_task.py`:
- Around line 30-42: Ruff flags the class-level mutable lists in the Migration
class (dependencies and operations) as RUF012; suppress it by adding a per-line
noqa comment to each list (e.g., append "# noqa: RUF012" to the dependencies =
[...] and operations = [...] declarations) or alternatively add a
per-path/filename ignore for migration files in your Ruff config; target the
Migration class and its dependencies and operations symbols when applying the
suppression.
In `@commcare_connect/audit/tests/test_calculations.py`:
- Around line 64-81: The test test_register_calculation_appends_instance
duplicates manual save/clear/restore of calculations._REGISTRY; create a
reusable pytest fixture named isolated_registry that saves original =
list(calculations._REGISTRY), clears calculations._REGISTRY, yields, and
restores it after the test, then change
test_register_calculation_appends_instance to accept isolated_registry as a
parameter and remove the try/finally block so the fixture handles setup/teardown
for calculations._REGISTRY.
In `@commcare_connect/audit/tests/test_services.py`:
- Around line 11-17: Move the isolated_registry pytest fixture out of
commcare_connect/audit/tests/test_services.py into
commcare_connect/audit/tests/conftest.py so it can be reused; specifically, keep
the same save/clear/restore logic that manipulates calculations._REGISTRY and
remove the duplicate manual registry handling in test_calculations.py (the code
in test_register_calculation_appends_instance) to use the shared
isolated_registry fixture instead. Ensure the fixture name remains
isolated_registry and that tests import nothing extra so pytest auto-discovers
it from conftest.py.
In `@commcare_connect/audit/tests/test_tasks.py`:
- Line 9: Move the side-effect import of AuditReportFactory out of the test
module and into the package-level conftest.py so the factory registration
applies globally; remove the line "from commcare_connect.audit.tests.factories
import AuditReportFactory # noqa: F401" from
commcare_connect/audit/tests/test_tasks.py and add an equivalent import
statement in the app's conftest.py (or create one) to perform the import for
side-effect registration instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ee5109a-58ce-40eb-a7b8-af67b454c430
📒 Files selected for processing (19)
commcare_connect/audit/__init__.pycommcare_connect/audit/admin.pycommcare_connect/audit/apps.pycommcare_connect/audit/calculations.pycommcare_connect/audit/migrations/0001_initial.pycommcare_connect/audit/migrations/0002_periodic_task.pycommcare_connect/audit/migrations/__init__.pycommcare_connect/audit/models.pycommcare_connect/audit/services.pycommcare_connect/audit/tasks.pycommcare_connect/audit/tests/__init__.pycommcare_connect/audit/tests/factories.pycommcare_connect/audit/tests/test_calculations.pycommcare_connect/audit/tests/test_models.pycommcare_connect/audit/tests/test_services.pycommcare_connect/audit/tests/test_tasks.pycommcare_connect/flags/flag_names.pycommcare_connect/opportunity/migrations/0026_create_send_inactive_notification_periodic_task.pyconfig/settings/base.py
| def run(self, opportunity_access, period_start, period_end) -> CalculationResult: | ||
| value, sample_size = self.compute(opportunity_access, period_start, period_end) | ||
| has_sufficient_data = sample_size >= self.min_sample_size | ||
| if not has_sufficient_data: | ||
| return CalculationResult( | ||
| name=self.name, | ||
| label=self.label, | ||
| value=None, | ||
| has_sufficient_data=False, | ||
| in_range=True, | ||
| ) |
There was a problem hiding this comment.
_in_range called with None value when compute returns (None, n≥min_sample_size)
run() only short-circuits when sample_size < min_sample_size. If a compute() implementation legitimately returns (None, sample_size) where sample_size >= min_sample_size, the code proceeds to _in_range(None), which raises TypeError on either bound comparison.
The docstring says "value may be None when sample_size == 0" — but there's no enforcement. If a future subclass's compute can return None for value with sample_size > 0, this will crash silently in production.
Consider an explicit guard:
🛡️ Proposed guard
def run(self, opportunity_access, period_start, period_end) -> CalculationResult:
value, sample_size = self.compute(opportunity_access, period_start, period_end)
has_sufficient_data = sample_size >= self.min_sample_size
if not has_sufficient_data:
return CalculationResult(
name=self.name,
label=self.label,
value=None,
has_sufficient_data=False,
in_range=True,
)
+ if value is None:
+ raise ValueError(
+ f"{type(self).__name__}.compute() returned value=None with sample_size={sample_size} "
+ f">= min_sample_size={self.min_sample_size}; value must not be None when data is sufficient."
+ )
return CalculationResult(
name=self.name,
label=self.label,
value=value,
has_sufficient_data=True,
in_range=self._in_range(value),
)🤖 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 `@commcare_connect/audit/calculations.py` around lines 57 - 67, The run()
method may call _in_range(None) if compute() returns (None, sample_size) with
sample_size >= min_sample_size, causing TypeError; update run (around the
compute/opportunity_access logic) to explicitly guard for value is None after
compute() and before calling _in_range: when value is None, return a
CalculationResult (same name/label) that sets value=None,
has_sufficient_data=False (or another explicit flag per existing semantics) and
in_range=True/False as appropriate so _in_range is never invoked with None;
reference the run, compute, _in_range, min_sample_size, and CalculationResult
symbols when making the change.
| report = AuditReport.objects.create( | ||
| opportunity=opportunity, | ||
| period_start=period_start, | ||
| period_end=period_end, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether AuditReport has a unique constraint on (opportunity, period_start, period_end)
fd -e py '0001_initial' --search-path commcare_connect/audit/migrations | xargs catRepository: dimagi/commcare-connect
Length of output: 4729
🏁 Script executed:
#!/bin/bash
# Check for any migrations after 0001_initial that might modify AuditReport constraints
fd -e py 'migrations/.*\.py$' --search-path commcare_connect/audit/migrations | sort | head -20Repository: dimagi/commcare-connect
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Examine the actual services.py file at lines 32-36 and surrounding context
sed -n '25,50p' commcare_connect/audit/services.pyRepository: dimagi/commcare-connect
Length of output: 737
🏁 Script executed:
#!/bin/bash
# List all migrations in the audit directory
ls -la commcare_connect/audit/migrations/ | grep -E '\.py$'Repository: dimagi/commcare-connect
Length of output: 264
🏁 Script executed:
#!/bin/bash
# Check the second migration to see if it modifies AuditReport
cat commcare_connect/audit/migrations/0002_periodic_task.pyRepository: dimagi/commcare-connect
Length of output: 1256
AuditReport.objects.create() lacks idempotency guard — duplicate reports will silently accumulate if task retries or fires multiple times
The AuditReport model has no UniqueConstraint or unique_together on (opportunity, period_start, period_end). The unconditional .create() call means that if the task fires more than once for the same period (manual retrigger, a second beat instance, or worker restart with acks_late), fresh AuditReport rows are created for each opportunity without any deduplication.
Robust periodic Celery tasks must be idempotent. Use get_or_create() instead:
Proposed fix
- report = AuditReport.objects.create(
- opportunity=opportunity,
- period_start=period_start,
- period_end=period_end,
- )
+ report, created = AuditReport.objects.get_or_create(
+ opportunity=opportunity,
+ period_start=period_start,
+ period_end=period_end,
+ )
+ if not created:
+ # Already generated for this period; skip to avoid duplicates.
+ return report🤖 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 `@commcare_connect/audit/services.py` around lines 32 - 36, Replace the
unconditional creation of AuditReport with an idempotent lookup/create: use
AuditReport.objects.get_or_create(...) keyed on opportunity, period_start, and
period_end instead of AuditReport.objects.create(...). Update the surrounding
logic that expects a newly created instance (the variable report) to handle the
(instance, created) tuple returned by get_or_create and avoid duplicating
reports when the task retries or runs multiple times.
|
|
||
| audit_report = factory.SubFactory(AuditReportFactory) | ||
| opportunity_access = factory.SubFactory(OpportunityAccessFactory) | ||
| results = {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mutable dict/list class attributes in factory files.
# Expected: this should report the current Line 25 occurrence at minimum.
rg -nP --type=py '^\s*[A-Za-z_][A-Za-z0-9_]*\s*=\s*(\{\}|\[\])\s*$' --iglob '**/tests/factories.py'Repository: dimagi/commcare-connect
Length of output: 129
🏁 Script executed:
cat -n commcare_connect/audit/tests/factories.py | head -50Repository: dimagi/commcare-connect
Length of output: 1010
🏁 Script executed:
# Verify there are no other similar issues in this file
wc -l commcare_connect/audit/tests/factories.pyRepository: dimagi/commcare-connect
Length of output: 112
🏁 Script executed:
# Check if there are other factory files in the tests directory to see if LazyFunction pattern is already used
rg -n 'LazyFunction' --type=py --iglob '**/tests/factories.py'Repository: dimagi/commcare-connect
Length of output: 976
Use factory.LazyFunction(dict) instead of mutable class-scope default.
Line 25 assigns results = {} at class scope in AuditReportEntryFactory. This mutable object is shared across factory instances and can leak state between tests. Use factory.LazyFunction(dict) to create a new dict for each instance.
Suggested fix
class AuditReportEntryFactory(DjangoModelFactory):
class Meta:
model = AuditReportEntry
audit_report = factory.SubFactory(AuditReportFactory)
opportunity_access = factory.SubFactory(OpportunityAccessFactory)
- results = {}
+ results = factory.LazyFunction(dict)
flagged = False🧰 Tools
🪛 Ruff (0.15.12)
[warning] 25-25: Mutable default value for class attribute
(RUF012)
🤖 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 `@commcare_connect/audit/tests/factories.py` at line 25,
AuditReportEntryFactory defines a mutable class-scope default results = {} which
is shared across instances; change the factory attribute to create a new dict
per instance by using factory.LazyFunction(dict) for the results field (update
the AuditReportEntryFactory definition to replace the class-scope results = {}
with results = factory.LazyFunction(dict) so each generated instance gets its
own dict).
| assert AuditReportEntry.objects.filter(audit_report=report).count() == 0 | ||
|
|
||
|
|
||
| def test_period_for_monday_returns_previous_week(): |
There was a problem hiding this comment.
Nit: This and the following two tests look like they can all be parametrized as they all share a very similar structure.
3 participants
Technical Summary
This adds the celery task for creating the weekly audit report, building on the base models and calculation classes from #1147. It is only the backend and does not include any of the UI changes, which will come in the next two PRs.
Safety Assurance
Safety story
Backend only and flag turned off
Automated test coverage
Tests are included for the relevant pieces
Labels & Review