feat: use respondent mode for third parties#3783
Open
Mohamed-Hacene wants to merge 9 commits intomainfrom
Open
feat: use respondent mode for third parties#3783Mohamed-Hacene wants to merge 9 commits intomainfrom
Mohamed-Hacene wants to merge 9 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request extends third-party actor functionality across the system by expanding their permissions, simplifying questionnaire URLs, automating requirement assignment creation for audits, and reconfiguring navigation and routing to accommodate third-party users as distinct actors in the platform. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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 |
Mohamed-Hacene
added
the
db-migration
![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: 2
🧹 Nitpick comments (2)
backend/tprm/migrations/0016_assign_third_parties_to_audits.py (1)
48-52: Noop reverse operation prevents rollback.Using
migrations.RunPython.noopas the reverse means this migration cannot be rolled back, which will leave orphanedRequirementAssignmentrecords if needed. Consider whether a reverse function that deletes assignments created by this migration would be appropriate for your deployment strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tprm/migrations/0016_assign_third_parties_to_audits.py` around lines 48 - 52, The migration uses migrations.RunPython(assign_third_parties_to_existing_audits, migrations.RunPython.noop) which prevents rollback; implement a proper reverse function (e.g., delete_assignments_created_by_assign_third_parties_to_existing_audits) that finds and deletes only the RequirementAssignment records created by assign_third_parties_to_existing_audits (use a reversible marker such as a specific metadata flag, creation timestamp window, or querying by the same criteria used for creation), add that function to the file, and replace migrations.RunPython.noop with that reverse function so the migration can be rolled back safely.frontend/tests/functional/detailed/tprm.test.ts (1)
247-264: Consider reducing reliance on hardcoded timeouts.The
waitForTimeoutcalls (lines 250, 261) are documented as flakiness workarounds, but hardcoded waits are generally fragile in E2E tests. If this continues to be flaky, consider waiting for a specific condition (e.g., network idle, element state change) instead.That said, the current approach is pragmatic and the comment explains the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/functional/detailed/tprm.test.ts` around lines 247 - 264, The test currently uses hardcoded waits in clickAndPause and after clicking nextButton which causes fragility; replace the timeouts by waiting for specific UI conditions: modify the clickAndPause helper and the loop that advances requirements to wait for the target element/state instead of page.waitForTimeout — e.g., after clicking use Locator.waitFor({ state: 'visible' | 'attached' }) or page.waitForLoadState('networkidle') and use nextButton.waitFor({ state: 'enabled' }) or check element.isEnabled() before interacting; update references to clickAndPause and nextButton in the test to use these conditional waits so interactions wait for the actual element visibility/enabled state rather than fixed delays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/core/views.py`:
- Around line 9888-9890: The bulk update on instance.requirement_assignments
(changing RequirementAssignment.Status.DRAFT -> IN_PROGRESS) bypasses the
RequirementAssignment transition path and may promote assignments whose actors
were never notified (e.g., when instance.authors is empty or an author lacks
mailing()), so replace the raw update() with the same per-assignment transition
used by RequirementAssignmentViewSet.set_status(): iterate only the assignments
whose actor was actually notified (use the mailing() check on each
author/actor), call the existing transition method on each RequirementAssignment
instance (instead of bulk update) so permissions, events and notification hooks
run, and reuse the shared set_status logic to keep behavior consistent.
In `@backend/tprm/serializers.py`:
- Around line 212-225: The _create_requirement_assignment method creates
RequirementAssignment instances without explicitly setting the status, causing
inconsistency with the migration which sets status="in_progress"; update the
RequirementAssignment.objects.create call in _create_requirement_assignment to
pass status="in_progress" so new assignments match the migration's state,
referencing the _create_requirement_assignment function and the
RequirementAssignment model to locate the change.
---
Nitpick comments:
In `@backend/tprm/migrations/0016_assign_third_parties_to_audits.py`:
- Around line 48-52: The migration uses
migrations.RunPython(assign_third_parties_to_existing_audits,
migrations.RunPython.noop) which prevents rollback; implement a proper reverse
function (e.g.,
delete_assignments_created_by_assign_third_parties_to_existing_audits) that
finds and deletes only the RequirementAssignment records created by
assign_third_parties_to_existing_audits (use a reversible marker such as a
specific metadata flag, creation timestamp window, or querying by the same
criteria used for creation), add that function to the file, and replace
migrations.RunPython.noop with that reverse function so the migration can be
rolled back safely.
In `@frontend/tests/functional/detailed/tprm.test.ts`:
- Around line 247-264: The test currently uses hardcoded waits in clickAndPause
and after clicking nextButton which causes fragility; replace the timeouts by
waiting for specific UI conditions: modify the clickAndPause helper and the loop
that advances requirements to wait for the target element/state instead of
page.waitForTimeout — e.g., after clicking use Locator.waitFor({ state:
'visible' | 'attached' }) or page.waitForLoadState('networkidle') and use
nextButton.waitFor({ state: 'enabled' }) or check element.isEnabled() before
interacting; update references to clickAndPause and nextButton in the test to
use these conditional waits so interactions wait for the actual element
visibility/enabled state rather than fixed delays.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f5f4fa-23ab-4797-9d0e-ad7d77800482
📒 Files selected for processing (15)
backend/core/startup.pybackend/core/templates/tprm/third_party_email.htmlbackend/core/views.pybackend/iam/models.pybackend/locale/fr/LC_MESSAGES/django.pobackend/tprm/migrations/0016_assign_third_parties_to_audits.pybackend/tprm/serializers.pybackend/tprm/test/test_serializers.pyfrontend/src/lib/components/SideBar/navData.tsfrontend/src/routes/(app)/(internal)/+layout.server.tsfrontend/src/routes/(app)/(third-party)/auditee-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/assignments/+page.sveltefrontend/tests/functional/detailed/tprm.test.tsfrontend/tests/utils/mailer.tsfrontend/tests/utils/test-utils.ts
Comment on lines
+9888
to
+9890
| instance.requirement_assignments.filter( | ||
| status=RequirementAssignment.Status.DRAFT | ||
| ).update(status=RequirementAssignment.Status.IN_PROGRESS) |
Contributor
There was a problem hiding this comment.
Bulk status promotion here can activate unnotified assignments and skips workflow guards.
If instance.authors is empty—or an author was skipped above because they have no mailing() implementation—you still return success and move every draft assignment to in_progress. The raw update() also bypasses the normal RequirementAssignment transition path, so the dedicated permission, event creation, and notification hooks never run. Please transition only the assignments whose actors were actually notified, via shared logic extracted from RequirementAssignmentViewSet.set_status().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/core/views.py` around lines 9888 - 9890, The bulk update on
instance.requirement_assignments (changing RequirementAssignment.Status.DRAFT ->
IN_PROGRESS) bypasses the RequirementAssignment transition path and may promote
assignments whose actors were never notified (e.g., when instance.authors is
empty or an author lacks mailing()), so replace the raw update() with the same
per-assignment transition used by RequirementAssignmentViewSet.set_status():
iterate only the assignments whose actor was actually notified (use the
mailing() check on each author/actor), call the existing transition method on
each RequirementAssignment instance (instead of bulk update) so permissions,
events and notification hooks run, and reuse the shared set_status logic to keep
behavior consistent.
Comment on lines
+212
to
+225
| def _create_requirement_assignment(self, audit, representatives): | ||
| """Create a RequirementAssignment linking all requirement assessments to representative actors.""" | ||
| actors = [rep.actor for rep in representatives if hasattr(rep, "actor")] | ||
| if not actors: | ||
| return | ||
| requirement_assessments = audit.requirement_assessments.all() | ||
| if not requirement_assessments.exists(): | ||
| return | ||
| assignment = RequirementAssignment.objects.create( | ||
| compliance_assessment=audit, | ||
| folder=audit.folder, | ||
| ) | ||
| assignment.actor.set(actors) | ||
| assignment.requirement_assessments.set(requirement_assessments) |
Contributor
There was a problem hiding this comment.
Missing status field creates inconsistency with migration.
The migration (0016_assign_third_parties_to_audits.py) explicitly sets status="in_progress" when creating RequirementAssignment records, but this method relies on the model's default. This could lead to inconsistent states between newly created assignments and migrated ones.
Proposed fix to align status with migration
assignment = RequirementAssignment.objects.create(
compliance_assessment=audit,
folder=audit.folder,
+ status="in_progress",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tprm/serializers.py` around lines 212 - 225, The
_create_requirement_assignment method creates RequirementAssignment instances
without explicitly setting the status, causing inconsistency with the migration
which sets status="in_progress"; update the RequirementAssignment.objects.create
call in _create_requirement_assignment to pass status="in_progress" so new
assignments match the migration's state, referencing the
_create_requirement_assignment function and the RequirementAssignment model to
locate the change.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes