Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPOST /questionnaires/{questionnaireID}/close エンドポイントを追加し、ハンドラー→コントローラー→リポジトリでアンケートを「閉じる」処理（res_time_limit を現在時刻に設定または NULL に更新）を実装。既存の期限正規化トレランスは削除され、OpenAPI/spec、ルーティング、関連テストを追加／更新。 変更
シーケンス図sequenceDiagram
actor Client
participant Handler as "Handler\n(handler/questionnaire.go)"
participant Controller as "Controller\n(controller/questionnaire.go)"
participant Repository as "IQuestionnaire\n(model/questionnaires_impl.go)"
participant DB as "Database"
Client->>Handler: POST /api/questionnaires/{id}/close
Handler->>Handler: パスパラメータをバインド
Handler->>Controller: CloseQuestionnaire(ctx, questionnaireID)
activate Controller
Controller->>Controller: now := null.TimeFrom(time.Now())
Controller->>Controller: q.ITransaction.Do(txFn)
Controller->>Repository: UpdateQuestionnaireLimit(ctx, id, now)
activate Repository
Repository->>DB: SELECT ... WHERE id=?
DB-->>Repository: record / not found
alt record found
Repository->>DB: UPDATE questionnaires SET res_time_limit=?, modified_at=NOW() WHERE id=?
DB-->>Repository: rows affected > 0
Repository-->>Controller: nil
else not found
Repository-->>Controller: ErrNoRecordUpdated
end
deactivate Repository
alt ErrNoRecordUpdated
Controller-->>Handler: echo.HTTPError(404)
else other error
Controller-->>Handler: echo.HTTPError(500)
else success
Controller->>Controller: q.DeleteReminder(questionnaireID) (ログのみ)
Controller-->>Handler: nil
end
deactivate Controller
alt error
Handler-->>Client: HTTP Error (400/404/500)
else success
Handler-->>Client: 200 No Content
end
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~45 分 Suggested labels
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1536 +/- ##
==========================================
- Coverage 63.52% 63.49% -0.04%
==========================================
Files 26 26
Lines 4025 4054 +29
==========================================
+ Hits 2557 2574 +17
- Misses 1078 1086 +8
- Partials 390 394 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/questionnaire.go`:
- Around line 1068-1090: CloseQuestionnaire currently only updates the limit but
doesn't disable or remove scheduled reminders, causing post-close notifications;
inside the transaction callback in CloseQuestionnaire (after calling
UpdateQuestionnaireLimit and before returning nil) invoke the same reminder
cleanup used by EditQuestionnaire/DeleteQuestionnaire (e.g., the function that
removes/disables scheduled reminders for a questionnaire), propagate/log any
error similarly (respecting the model.ErrNoRecordUpdated handling), and ensure
failures return the appropriate error to the outer handler so reminders are
reliably cleared on close.
🪄 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: da265e0f-877b-46f1-810a-b5824b1ae2f4
📒 Files selected for processing (8)
controller/questionnaire.godocs/swagger/swagger.yamlhandler/questionnaire.gomain.gomodel/questionnaires.gomodel/questionnaires_impl.goopenapi/server.goopenapi/spec.go
Add a dedicated close endpoint that sets res_time_limit to the current server time, avoiding front/back clock skew issues that occurred when the client sent a timestamp near the tolerance boundary. - Add UpdateQuestionnaireLimit model method for targeted limit update - Add CloseQuestionnaire controller and handler - Require QuestionnaireAdministratorAuthenticate middleware - Update OpenAPI spec and regenerated server/spec code Co-authored-by: Eraxyso <130852025+Eraxyso@users.noreply.github.com>
Eraxyso
force-pushed
the
claude/questionnaire-close-endpoint-4ca4N
branch
from
38fa42b to
e0927ef
Compare
- CloseQuestionnaire: call DeleteReminder inside the transaction after updating the limit so scheduled notifications are cancelled when a questionnaire is closed immediately - Remove normalizeResponseDueDateTime and responseDueDateTimeTolerance; replace call sites with a direct past-time rejection now that the dedicated /close endpoint removes the need to accept near-past timestamps Co-authored-by: Eraxyso <130852025+Eraxyso@users.noreply.github.com>
…tion Remove the "within tolerance" success case and its normalizedDueDate expect field/assertion. Repurpose the test case to verify that even a slightly-past timestamp (formerly accepted within the 5s window) is now rejected with an error. Co-authored-by: Eraxyso <130852025+Eraxyso@users.noreply.github.com>
- TestCloseQuestionnaire: valid ID sets res_time_limit to ~now, invalid ID returns not-found error - updateQuestionnaireLimitTest: set limit to now, clear to null, and invalid ID returns ErrNoRecordUpdated Co-authored-by: Eraxyso <130852025+Eraxyso@users.noreply.github.com>
… test The "clear limit (null)" case previously created a questionnaire with no limit (NULL) and then updated it to NULL again. MariaDB reports RowsAffected=0 when no column actually changes, which made the implementation return ErrNoRecordUpdated and failed the test. Seed the questionnaire with a non-null limit in the past so every update is a real change. Also drop t.Parallel() to keep ordering consistent with the sibling tests in TestQuestionnaires. Co-authored-by: Eraxyso <130852025+Eraxyso@users.noreply.github.com>
…ed=0 false negative When res_time_limit is already NULL and we update to NULL (or when modified_at doesn't change within the same second), GORM may report RowsAffected=0 even though the questionnaire exists, causing ErrNoRecordUpdated to be returned incorrectly. Replace RowsAffected check with an explicit SELECT to verify existence before updating. https://claude.ai/code/session_01UVfZTySABd5YYU1LP2ULiZ
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controller/questionnaire_test.go (1)
1984-2075: クローズ後の回答拒否もこのテストで押さえておきたいです。今の
TestCloseQuestionnaireはres_time_limit更新しか見ていないので、将来PostQuestionnaireResponse側の期限チェックが壊れてもこのPRの主目的が検知できません。クローズ成功後に1件回答を投げて、422相当のエラーになることまで確認しておくと回帰に強くなります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/questionnaire_test.go` around lines 1984 - 2075, After closing the questionnaire in TestCloseQuestionnaire, add an assertion that attempting to submit a response via q.PostQuestionnaireResponse (or the HTTP POST to /questionnaires/{id}/responses) fails with a 422-equivalent error; specifically call q.PostQuestionnaireResponse (or invoke the handler) using the closed questionnaireID and assert the call returns an error/HTTP 422, so the test verifies both IQuestionnaire.GetQuestionnaireLimit updates and that PostQuestionnaireResponse enforces the closed-time rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/questionnaire.go`:
- Around line 1054-1068: The DeleteReminder call should be moved out of the
ITransaction.Do closure so you only remove the in-memory scheduler entry after
the DB transaction (UpdateQuestionnaireLimit) has committed; in the function
containing ITransaction.Do, call UpdateQuestionnaireLimit inside the transaction
(handling model.ErrNoRecordUpdated as before), return nil on success, and then
after ITransaction.Do returns without error invoke
DeleteReminder(questionnaireID) (logging any delete error) to avoid inconsistent
state between res_time_limit and the scheduler.
---
Nitpick comments:
In `@controller/questionnaire_test.go`:
- Around line 1984-2075: After closing the questionnaire in
TestCloseQuestionnaire, add an assertion that attempting to submit a response
via q.PostQuestionnaireResponse (or the HTTP POST to
/questionnaires/{id}/responses) fails with a 422-equivalent error; specifically
call q.PostQuestionnaireResponse (or invoke the handler) using the closed
questionnaireID and assert the call returns an error/HTTP 422, so the test
verifies both IQuestionnaire.GetQuestionnaireLimit updates and that
PostQuestionnaireResponse enforces the closed-time rejection.
🪄 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: 16f10560-c07b-4e49-8060-0f23ff9eb8e3
📒 Files selected for processing (10)
controller/questionnaire.gocontroller/questionnaire_test.godocs/swagger/swagger.yamlhandler/questionnaire.gomain.gomodel/questionnaires.gomodel/questionnaires_impl.gomodel/questionnaires_test.goopenapi/server.goopenapi/spec.go
✅ Files skipped from review due to trivial changes (1)
- model/questionnaires_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- main.go
- model/questionnaires.go
- openapi/spec.go
- model/questionnaires_impl.go
- docs/swagger/swagger.yaml
- openapi/server.go
Questionnaires created during the test were left in the DB, disrupting the GetQuestionnaires sort-by-title test due to collation differences between Go string comparison and MariaDB utf8mb4_general_ci for ASCII vs CJK character ordering. Use t.Cleanup to soft-delete them after the subtest completes, before parallel subtests run. https://claude.ai/code/session_01UVfZTySABd5YYU1LP2ULiZ
DeleteReminder modifies in-memory scheduler state. Calling it inside the ITransaction.Do closure means the scheduler entry could be removed before the DB transaction commits; if the commit were to fail, the DB and scheduler would be inconsistent. Move it after Do returns without error. https://claude.ai/code/session_01UVfZTySABd5YYU1LP2ULiZ
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.
概要
アンケートを即座に終了するための専用エンドポイント
POST /questionnaires/{questionnaireID}/closeを追加します。背景・問題
これまでアンケートを即座に終了するには、フロントエンドから
res_time_limitに現在時刻を設定してリクエストを送る必要がありました。しかし、フロントエンドとバックエンドの時刻がわずかにずれている場合、5秒の許容範囲（responseDueDateTimeTolerance）を超えてしまい、400 Bad Requestが返るという問題がありました。解決策
サーバー側の現在時刻を使って
res_time_limitを更新する専用の close エンドポイントを追加することで、クライアントの時刻に依存せずにアンケートを即座に終了できるようにしました。変更内容
POST /questionnaires/{questionnaireID}/closeエンドポイントを追加（管理者のみ）model.IQuestionnaireにUpdateQuestionnaireLimitメソッドを追加（res_time_limitのみを更新）QuestionnaireAdministratorAuthenticateミドルウェアを新エンドポイントに適用テスト方法
res_time_limitを設定するPOST /api/questionnaires/{questionnaireID}/closeを呼び出すres_time_limitがサーバー時刻で更新され、それ以降の回答が拒否されることを確認するSummary by CodeRabbit