fix: delete all the related info when a questionnaire is deleted#1441
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Eraxyso
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/openapi #1441 +/- ##
===============================================
- Coverage 67.16% 66.72% -0.44%
===============================================
Files 25 25
Lines 3706 3751 +45
===============================================
+ Hits 2489 2503 +14
- Misses 851 874 +23
- Partials 366 374 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tions, scale labels, validations, and responses
Eraxyso
force-pushed
the
fix/delete-target-and-user-when-questionnaire-is-deleted
branch
from
ecf3331 to
8d23446
Compare
…ionnaire-is-deleted
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure that deleting a questionnaire also deletes its related data (targets/admin metadata, questions’ auxiliary records, and respondents’ data) so the system doesn’t retain orphaned records.
Changes:
- Add deletion of target user/group selections and administrator user/group selections when deleting a questionnaire.
- Add deletion of question-related auxiliary records (options, scale labels, validations) before deleting questions.
- Add deletion of responses and respondents associated with the questionnaire.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| respondentDetails, err := q.GetRespondentDetails(ctx, questionnaireID, "", false, "", nil) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to get respondent details: %+v", err) | ||
| return err | ||
| } | ||
| for _, respondentDetail := range respondentDetails { | ||
| err = q.IResponse.DeleteResponse(ctx, respondentDetail.ResponseID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete responses: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| err = q.DeleteRespondent(ctx, respondentDetail.ResponseID) |
There was a problem hiding this comment.
GetRespondentDetails currently filters respondents with submitted_at IS NOT NULL (see model/respondents_impl.go), so this will only delete submitted responses and will leave draft respondents (submitted_at NULL) and their related records behind. For questionnaire deletion, consider fetching all respondent responseIDs for the questionnaire (draft + submitted) via a dedicated repository method (e.g., ListRespondentResponseIDsByQuestionnaireID) or by updating the underlying query to not exclude drafts when isDraft == nil.
| respondentDetails, err := q.GetRespondentDetails(ctx, questionnaireID, "", false, "", nil) | |
| if err != nil { | |
| c.Logger().Errorf("failed to get respondent details: %+v", err) | |
| return err | |
| } | |
| for _, respondentDetail := range respondentDetails { | |
| err = q.IResponse.DeleteResponse(ctx, respondentDetail.ResponseID) | |
| if err != nil { | |
| c.Logger().Errorf("failed to delete responses: %+v", err) | |
| return err | |
| } | |
| err = q.DeleteRespondent(ctx, respondentDetail.ResponseID) | |
| responseIDs, err := q.ListRespondentResponseIDsByQuestionnaireID(ctx, questionnaireID) | |
| if err != nil { | |
| c.Logger().Errorf("failed to list respondent response IDs: %+v", err) | |
| return err | |
| } | |
| for _, responseID := range responseIDs { | |
| err = q.IResponse.DeleteResponse(ctx, responseID) | |
| if err != nil { | |
| c.Logger().Errorf("failed to delete responses: %+v", err) | |
| return err | |
| } | |
| err = q.DeleteRespondent(ctx, responseID) |
| c.Logger().Errorf("failed to delete responses: %+v", err) | ||
| return err |
There was a problem hiding this comment.
IResponse.DeleteResponse returns an error when RowsAffected == 0. Since posting/editing a draft can create a respondent with len(responseMetas)==0 (no rows in the response table yet), deleting the questionnaire can fail here with ErrNoRecordDeleted. Treat "no response rows" as a successful deletion in this flow (e.g., ignore model.ErrNoRecordDeleted/model.ErrRecordNotFound for responses, or add a repository method that deletes by response_id without requiring affected rows).
| c.Logger().Errorf("failed to delete responses: %+v", err) | |
| return err | |
| // Treat "no response rows" as a successful deletion for this flow. | |
| if errors.Is(err, model.ErrNoRecordDeleted) || errors.Is(err, model.ErrRecordNotFound) { | |
| // nothing to delete; continue with next respondent | |
| } else { | |
| c.Logger().Errorf("failed to delete responses: %+v", err) | |
| return err | |
| } |
| err = q.DeleteTargetUsers(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete target users: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| err = q.DeleteTargetGroups(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete target groups: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| err = q.DeleteAdministrators(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete administrators: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| err = q.DeleteAdministratorUsers(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete administrator users: %+v", err) | ||
| return err | ||
| } | ||
| err = q.DeleteAdministratorGroups(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to delete administrator groups: %+v", err) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
This endpoint now performs a lot of additional cleanup (targets_users/groups, administrator_users/groups, options/scale_labels/validations, respondents/responses), but controller/questionnaire_test.go's DeleteQuestionnaire coverage appears to only assert that the questionnaire itself is deleted. Please add/update tests to verify the related records are removed as well (including draft respondents).
| questions, err := q.GetQuestions(ctx, questionnaireID) | ||
| if err != nil { | ||
| c.Logger().Errorf("failed to get questions: %+v", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
GetQuestions uses GORM's default scope, so it will not return soft-deleted questions (questions.deleted_at IS NULL). If a questionnaire previously had questions removed (soft-deleted via DeleteQuestion), this loop won’t delete their dependent rows (options / scale_labels / validations / responses), leaving data behind even after the questionnaire is deleted. Consider deleting question-related rows by questionnaire_id (via a join/subquery) or adding a repository method to fetch question IDs Unscoped() for cleanup.
kaitoyama
left a comment
kaitoyama
left a comment
There was a problem hiding this comment.
論理削除に持っていくでもいいかもとは思いますが、これでもいいと思います。ただ、下書き削除されるかだけ確認お願いします
|
@copilot 確認をお願いします |
Eraxyso
force-pushed
the
fix/delete-target-and-user-when-questionnaire-is-deleted
branch
from
bf1dbcc to
a9e7c73
Compare
…e Swagger documentation
…ngths and sort indices
…ionnaire-is-deleted
Eraxyso
force-pushed
the
fix/delete-target-and-user-when-questionnaire-is-deleted
branch
from
74ee40b to
705d59d
Compare
Eraxyso
deleted the
fix/delete-target-and-user-when-questionnaire-is-deleted
branch
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.