feat: choose if an user is third party respondent or not #3072
Conversation
WalkthroughCentralizes cascade classification in backend core views (FORCED_CASCADE_FIELDS, add_with_classification) and unifies cascade-info display/processing; adds optional is_third_party to the user edit schema and an edit-mode checkbox in the UserForm; introduces thirdPartyRespondent and thirdPartyRespondentHelpText translation keys across locale files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)backend/core/views.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/views.py (1)
717-942: Cascade classification: messaging inconsistency and FORCED_CASCADE_FIELDS brittlenessThe refactor centralizes cascade classification effectively and resolves several issues (through-model handling, deduplication, consistent field update routing). However, two maintainability concerns warrant attention:
"Deleted" bucket messaging conflicts with SET_NULL behavior
Objects withon_delete=models.SET_NULLrelationships (e.g.,Representative.user) are routed to thedeletedbucket viaFORCED_CASCADE_FIELDS, but the payload message says "The following objects will be permanently deleted." This is factually incorrect—those objects are not deleted, only their foreign keys are cleared. Either adjust the messages in both the docstring andpayload["deleted"]["message"]to clarify "deleted or logically removed due to business rules," or introduce a separate classification so the UI can distinguish actual deletions from relationship removals.String-based FORCED_CASCADE_FIELDS is fragile
FORCED_CASCADE_FIELDS = {("Representative", "user")}relies on hard-coded model and field names matched viasource_model.__name__and string comparison. Renaming the model or field will silently break this behavior without static tooling to catch it. Consider using model metadata instead, such as comparing againstsource_model is Representative(if imports permit) or deriving tuples frommodel._meta.labelfor more robust matching.The
add_with_classificationlogic,_iter_objsiterator handling, and outgoing link classification (section 2c) all look solid.
🧹 Nitpick comments (1)
frontend/src/lib/utils/schemas.ts (1)
368-368: Consider adding a default value for consistency.The
is_third_partyfield is defined asz.boolean().optional()without a default value. Many similar boolean fields in this schema file use explicit defaults (e.g.,is_active,is_locked,is_enabledwith.default(false)or.default(true)).Without a default, the field can be
undefined, which might require additional handling in the backend. Consider whether.default(false)would be more appropriate for consistency.🔎 Apply this diff if a default is preferred:
- is_third_party: z.boolean().optional(), + is_third_party: z.boolean().optional().default(false),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/views.py(5 hunks)frontend/messages/en.json(1 hunks)frontend/src/lib/components/Forms/ModelForm/UserForm.svelte(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Frontend Linters
frontend/messages/en.json
[error] 2931-2931: Prettier check failed: SyntaxError in messages/en.json. Unexpected token, expected ','. (2931:1). Command: pnpm exec prettier --check .
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
frontend/src/lib/components/Forms/ModelForm/UserForm.svelte (1)
76-81: Theis_third_partyfield is intentionally available only during user editing, not creation.This aligns with the schema design—
UserCreateSchemaexcludes this field, while it remains available for updates. The system appears to follow a pattern where users are created as regular users and can be designated as third-party respondents through editing afterward. No changes needed.backend/core/views.py (1)
5158-5199: UserFilter: addingis_third_partyto filterable fieldsIncluding
"is_third_party"inUserFilter.Meta.fieldsis a straightforward way to expose the new flag for API‑level filtering and aligns with the frontend changes around third‑party respondents. AssumingUserindeed has a Booleanis_third_partyfield and default lookup (exact) is intended, this looks correct and non‑breaking.To double‑check that this filter is usable end‑to‑end, you can hit the list endpoint with a query like
?is_third_party=trueand verify that only third‑party users are returned.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
frontend/messages/ar.json (1)
1030-1032: Refine Arabic label to describe the user, not the responses
thirdPartyRespondentis currently "الردود الخارجية", which describes responses rather than the responding user. Consider changing it to something like "مجيب تابع لجهة خارجية" (or "مستجيب تابع لجهة خارجية") to match the user‑role semantics while keeping the existing help text.frontend/messages/nl.json (1)
2927-2929: Translations look correct; consider slightly more idiomatic Dutch and inclusive wording
cascadeAffectedHintis clear and matches the intended meaning of "items remain but links are removed".thirdPartyRespondent/thirdPartyRespondentHelpTextcorrectly convey the restricted-respondent concept.If you want slightly more natural and gender‑neutral Dutch, you could consider:
Optional wording tweak
- "thirdPartyRespondent": "Derdepartij-respondent", - "thirdPartyRespondentHelpText": "Beperk de gebruiker tot het reageren op vragenlijsten/audits waartoe hij toegang heeft; geen andere acties zijn beschikbaar." + "thirdPartyRespondent": "Respondent van derde partij", + "thirdPartyRespondentHelpText": "Beperk deze gebruiker tot het reageren op vragenlijsten/audits waartoe hij toegang heeft; geen andere acties zijn beschikbaar."frontend/messages/tr.json (1)
1795-1797: Polish Turkish label/help wording for ‘thirdPartyRespondent’ (optional)Anlam doğru ama “Üçüncü Taraflı Yanıtlayıcı” kulağa biraz yapay geliyor; genelde “taraflı” önyargılı anlamını çağrıştırıyor. Daha doğal bir ifade ve yardım metni için aşağıdakini düşünebilirsiniz:
Önerilen diff
- "thirdPartyRespondent": "Üçüncü Taraflı Yanıtlayıcı", - "thirdPartyRespondentHelpText": "Kullanıcıyı, erişebileceği anketler/denetimlere yanıt vermeye sınırlayın; başka bir işlem yoktur." + "thirdPartyRespondent": "Üçüncü taraf yanıtlayıcısı", + "thirdPartyRespondentHelpText": "Kullanıcıyı erişebileceği anketler/denetimlere yanıt vermekle sınırlandırın; başka işlemler yapamaz."frontend/messages/cs.json (1)
1050-1052: Tighten Czech label to better match ‘third‑party respondent’ (optional)Současný text „Třetí strana - odpovědná osoba“ evokuje spíš „odpovědná osoba“ než „respondent“. Pro přesnější význam můžete použít např.:
Navrhovaný diff
- "thirdPartyRespondent": "Třetí strana - odpovědná osoba", + "thirdPartyRespondent": "Respondent třetí strany",frontend/messages/ur.json (1)
1030-1032: Optional Urdu phrasing improvements for cascade hint and respondent helpپیغام سمجھ میں آتا ہے، لیکن اردو قدرے زیادہ فصیح ہو سکتی ہے؛ مثلاً:
مجوزہ diff
- "cascadeAffectedHint": "نیچے دیے گئے اشیاء نظام میں باقی ہیں لیکن ان کی اس اشیاء سے لنکس ہٹا دیے جائیں گے۔", - "thirdPartyRespondent": "تیسرے فریق کا جواب دہندہ", - "thirdPartyRespondentHelpText": "صارف کو صرف ان سوالناموں/آڈٹس کا جواب دینے کی اجازت دیں جن تک وہ رسائی حاصل کر سکتے ہیں؛ کوئی دوسرا عمل دستیاب نہیں ہے۔" + "cascadeAffectedHint": "نیچے دی گئی اشیاء نظام میں موجود رہیں گی لیکن ان کے اس شے سے روابط ہٹا دیے جائیں گے۔", + "thirdPartyRespondent": "تیسرے فریق کا جواب دہندہ", + "thirdPartyRespondentHelpText": "صارف کو صرف ان سوالناموں/آڈٹس کے جوابات تک محدود کریں جن تک اسے رسائی حاصل ہے؛ کوئی دیگر کارروائی دستیاب نہیں ہوگی۔"frontend/messages/de.json (1)
2928-2930: New German strings are correct; label could be phrased more naturallySemantics and behavior description look good:
cascadeAffectedHintclearly explains that objects remain but links are removed.thirdPartyRespondentHelpTextaccurately describes the restriction to answering questionnaires/audits only.If you want slightly more natural UI German for the role label, you could consider e.g.:
"thirdPartyRespondent": "Drittanbieter-Befragter"
or"thirdPartyRespondent": "Externer Antwortender"and optionally align the help text style with other toggles:
"thirdPartyRespondentHelpText": "Beschränkt diesen Benutzer darauf, nur Fragebögen/Audits zu beantworten, auf die er Zugriff hat; keine weiteren Aktionen sind verfügbar."These are purely wording tweaks; current text is acceptable as-is.
Optional diff for wording
- "thirdPartyRespondent": "Drittanbieter-Antwortverantwortlicher", - "thirdPartyRespondentHelpText": "Begrenzen Sie den Benutzer darauf, auf Fragebögen/Audits zu antworten, auf die er Zugriff hat; keine anderen Aktionen sind verfügbar." + "thirdPartyRespondent": "Drittanbieter-Befragter", + "thirdPartyRespondentHelpText": "Beschränkt diesen Benutzer darauf, nur Fragebögen/Audits zu beantworten, auf die er Zugriff hat; keine weiteren Aktionen sind verfügbar."frontend/messages/es.json (1)
2927-2929: Spanish strings are clear; only minor stylistic tweaks are optionalThe new messages read correctly and match the intended behavior:
cascadeAffectedHintexplains the impact on links vs. objects.thirdPartyRespondent/thirdPartyRespondentHelpTextcorrectly convey the “answer-only” third‑party role.If you want to polish phrasing slightly (optional):
- Add a comma before “pero” in the hint:
"cascadeAffectedHint": "Los elementos a continuación se mantendrán en el sistema, pero se eliminarán sus enlaces a este objeto."- Consider a slightly more common role label, e.g.:
"thirdPartyRespondent": "Respondedor de terceros"
or"thirdPartyRespondent": "Usuario de terceros (respondente)"Current text is acceptable; these are non-blocking refinements.
Optional diff for wording
- "cascadeAffectedHint": "Los elementos a continuación se mantendrán en el sistema pero se eliminarán sus enlaces a este objeto.", - "thirdPartyRespondent": "Respondente de Terceros", + "cascadeAffectedHint": "Los elementos a continuación se mantendrán en el sistema, pero se eliminarán sus enlaces a este objeto.", + "thirdPartyRespondent": "Respondedor de terceros",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/el.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hr.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/tr.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/en.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: lint-community (22)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
frontend/messages/ro.json (1)
1059-1061: New Romanian strings for third‑party respondent look correctThe hint and the new
thirdPartyRespondentlabel/help text are clear, grammatically sound, and consistent with the intended behavior. JSON structure is also valid.frontend/messages/sv.json (1)
1053-1055: Swedish translation for third‑party respondent is consistent and clearThe updated
cascadeAffectedHintplusthirdPartyRespondentlabel/help text read naturally in Swedish and accurately reflect the feature; JSON remains valid.frontend/messages/pt.json (1)
1062-1064: Portuguese third‑party respondent strings are well‑formedThe updated cascade hint and the new
thirdPartyRespondentlabel/help text are idiomatic PT‑BR and consistent with related terminology elsewhere in this file; JSON syntax is correct.frontend/messages/it.json (1)
1439-1441: Italian translations for third‑party respondent are accurateThe cascade hint plus
thirdPartyRespondentlabel/help text are clear, idiomatic, and consistent with existing third‑party terminology; JSON remains valid.frontend/messages/fr.json (1)
2928-2930: French keys align well with semantics and style
cascadeAffectedHintclearly mirrors the cascade notice behavior and reads naturally.thirdPartyRespondent/thirdPartyRespondentHelpTextaccurately express a third‑party, respondent‑only user with limited actions, consistent with the newis_third_partybehavior.No changes needed here.
frontend/messages/hi.json (1)
1031-1034: LGTM! Translation keys added correctly.The structural changes are sound:
- Trailing comma added to
cascadeAffectedHintto accommodate new keys- Two new translation keys (
thirdPartyRespondentandthirdPartyRespondentHelpText) added consistently- JSON formatting is correct
frontend/messages/hu.json (1)
1062-1064: LGTM! Translation keys added correctly.The changes are structurally correct:
- Trailing comma properly added to
cascadeAffectedHint- Two new translation keys added consistently with naming conventions
- JSON formatting is valid
frontend/messages/id.json (1)
1247-1249: LGTM! Translation keys added correctly.The structural changes are appropriate:
- Trailing comma added to
cascadeAffectedHintfor proper JSON syntax- Two new translation keys (
thirdPartyRespondentandthirdPartyRespondentHelpText) added consistently- Formatting aligns with the rest of the file
frontend/messages/hr.json (1)
2027-2029: LGTM! Translation additions are clean and consistent.The changes correctly add two new translation keys (
thirdPartyRespondentandthirdPartyRespondentHelpText) to support the new third-party respondent user attribute introduced in this PR. The trailing comma on line 2027 is properly added to maintain valid JSON syntax. The Croatian translations appear appropriate and align with the help text describing restricted permissions for third-party respondents.frontend/messages/da.json (1)
1342-1344: LGTM! Danish translations properly added.The changes mirror the same pattern applied across other locale files in this PR, adding the two new translation keys (
thirdPartyRespondentandthirdPartyRespondentHelpText) for the third-party respondent feature. The JSON syntax is correct with the trailing comma on line 1342, and the Danish translations ("Tredjeparts respondent" and the help text) appear appropriate for describing the restricted permission model.frontend/messages/pl.json (1)
1802-1802: Translation improvement looks good.The simplified wording removes unnecessary specificity ("Poniższe") and uses more appropriate future tense, making the hint clearer and more concise.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/views.py (1)
717-944: Forced cascades may display misleading "deleted" status for SET_NULL relationshipsThe refactor improves clarity with the centralized
add_with_classificationlogic. However, one UX issue needs attention:
FORCED_CASCADE_FIELDS = {(Representative, "user")}combined withadd_with_classificationplaces Representative objects in thedeletedbucket when a User is deleted. SinceRepresentative.userhason_delete=models.SET_NULL, the Representative row is not deleted—only the foreign key field is set to null. The payload message "The following objects will be permanently deleted." is therefore misleading for these cases.Consider either adjusting the message to reflect that these objects lose relationships (matching the "affected" bucket behavior), or separating "logically affected" objects into a distinct category to communicate the actual impact more clearly.
Additionally, the direct import of
Representativeat module level (line 140) hard-couplescoretotprm. While tprm is guaranteed in INSTALLED_APPS, consider usingapps.get_model("tprm", "Representative")under a try/except if you want to reduce inter-app dependencies or enable optional app configurations in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/views.py (1)
140-140: Representative import usage is consistent with new cascade classificationImporting
Representativefromtprm.modelsis needed forFORCED_CASCADE_FIELDSincascade_info, and the symbol is used there, so this addition is correct and necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/views.py (1)
721-742: FORCED_CASCADE_FIELDS condition prevents (Representative, "user") from ever taking effectThe centralized classification and unified display in
cascade_infoare good improvements, but the current guard aroundFORCED_CASCADE_FIELDSmakes the new(Representative, "user")entry effectively dead:
- In the
field_updatesloop (2b),source_modelisfield.model, i.e.Representativefor theRepresentative.userFK, whileinstance.__class__isUserwhen deleting a user.add_with_classificationonly treats a forced pair as deleted whensource_model == instance.__class__, so(Representative, "user")is never matched there.- Reverse relations from
UsertoRepresentativedon’t go through the 2c “outgoing links” branch either, because those auto‑created reverse fields are skipped.Net effect: representatives whose
useris set to NULL during user deletion are still classified as “affected”, not “deleted”, despite being listed inFORCED_CASCADE_FIELDS.You can fix this by dropping the
source_model == instance.__class__constraint and relying solely on the(source_model, field_name)pair:Suggested fix for forced‑cascade routing
@@ def cascade_info(self, request, pk=None): - def is_forced_cascade(source_model, field_name): - return (source_model, field_name) in FORCED_CASCADE_FIELDS + def is_forced_cascade(source_model, field_name): + return (source_model, field_name) in FORCED_CASCADE_FIELDS @@ - def add_with_classification(obj, source_model=None, source_field=None): + def add_with_classification(obj, source_model=None, source_field=None): @@ - if ( - source_model == instance.__class__ - and source_field - and is_forced_cascade(source_model, source_field) - ): + if source_model and source_field and is_forced_cascade( + source_model, source_field + ): add_grouped(deleted_bucket, obj) returnThis will allow the
(Representative, "user")rule to apply tofield_updateswhen deleting aUser, and any future forced pairs keyed by the referencing model.Also applies to: 744-748, 749-755, 769-771, 795-812, 826-848, 849-882, 884-899, 902-908, 915-942
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (1)
backend/tprm/models.py (2)
Entity(35-168)Representative(218-239)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/views.py (1)
140-140: Representative import usage looks correctImporting
Representativehere is appropriate for the newFORCED_CASCADE_FIELDSusage incascade_info, and introduces no obvious coupling or correctness issues.
Mohamed-Hacene
requested review from
ab-smith,
eric-intuitem and
nas-tabchiche
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.