feat: Botが自分のメッセージについた他人のスタンプを削除する - #3079
Conversation
📝 WalkthroughWalkthroughThe message stamp deletion flow now accepts ChangesSelective stamp removal
Sequence Diagram(s)sequenceDiagram
participant RemoveMessageStamp
participant MessageManager
participant UserRepository
participant MessageRepository
participant Cache
RemoveMessageStamp->>MessageManager: RemoveStamps(includeMe, includeOther)
alt includeOther is true
MessageManager->>UserRepository: GetUser(userID)
alt user is bot and owns the message
MessageManager->>MessageRepository: RemoveOtherStampFromMessage
else not permitted
MessageManager-->>RemoveMessageStamp: ErrCannotRemoveStamp
end
end
alt includeMe is true
MessageManager->>MessageRepository: RemoveStampFromMessage
end
MessageManager->>Cache: Forget(id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/v3-api.yaml (1)
705-733: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the new
403 Forbiddenresponse.
RemoveMessageStampnow mapsmessage.ErrCannotRemoveStamptoherror.Forbidden(403) when a caller lacks permission to remove others' stamps, but this operation only documents204and404. Add a403response so the contract reflects the new behavior.📝 Proposed addition
"404": description: |- Not Found メッセージ、またはスタンプが見つかりません。 + "403": + description: |- + Forbidden + スタンプを削除する権限がありません。 operationId: removeMessageStamp🤖 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 `@docs/v3-api.yaml` around lines 705 - 733, The RemoveMessageStamp operation is missing the new forbidden case in its API contract. Update the delete response definitions for removeMessageStamp in the v3 API spec to include a 403 Forbidden response alongside the existing 204 and 404 entries, matching the behavior where message.ErrCannotRemoveStamp is mapped to herror.Forbidden when the caller lacks permission to remove others’ stamps.
🧹 Nitpick comments (2)
router/v3/messages.go (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer binding booleans directly instead of strings + manual defaults.
IncludeMe/IncludeOtherare modeled asstringand then coerced viaisTrue, with empty-string defaulting done by hand. Binding to*bool(orboolwith explicit default handling) lets the framework parse the boolean and removes the stringly-typed indirection, keeping the handler aligned with the OpenAPIbooleanschema.Also applies to: 298-303
🤖 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 `@router/v3/messages.go` around lines 18 - 21, Update DeleteStampsQuery to bind IncludeMe and IncludeOther as booleans instead of strings, and adjust the corresponding handler logic to use the parsed values directly rather than calling isTrue or manually defaulting empty strings. Make the same change in the other affected query struct mentioned by the review so the router types stay aligned with the OpenAPI boolean schema and the existing query binding flow.service/message/manager_impl_test.go (1)
558-731: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThorough coverage of the permission matrix.
The subtests correctly exercise the bot-vs-non-bot × own-vs-other-message gating, the combined
includeMe/includeOtherpath, andGetUsererror propagation, matching theRemoveStampsimplementation.One gap worth considering: there is no subtest asserting that
RemoveOtherStampFromMessageis not invoked when the permission check fails (the failing cases rely on gomock's absence of an expectation). gomock already fails on unexpected calls, so this is optional, but an explicitTimes(0)would make intent clearer.🤖 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 `@service/message/manager_impl_test.go` around lines 558 - 731, Add an explicit negative assertion for the failing RemoveStamps permission paths so it is clear that RemoveOtherStampFromMessage is never called when the bot/non-bot or own/other-message check rejects the request. Update the relevant RemoveStamps subtests in manager_impl_test.go to set a Times(0) expectation on MockMessageRepository.RemoveOtherStampFromMessage for the cases that should fail, while keeping the existing success-path expectations for the includeMe/includeOther cases.
🤖 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 `@repository/gorm/message.go`:
- Line 556: Remove the leftover debug artifacts from the message repository
flow: the bare fmt.Println() in the affected function and the
repo.logger.Info(userID.String()) call that logs the caller UUID on every
invocation. Locate the cleanup in the message handling logic around the same
path as the existing userID and logger usage, and delete both statements without
changing the surrounding behavior.
In `@repository/message.go`:
- Line 128: The godoc above RemoveOtherStampFromMessage uses the wrong leading
symbol name, so update the comment to start with RemoveOtherStampFromMessage
instead of RemoveStampFromMessage. Keep the rest of the Japanese description
intact, and ensure the comment matches the method name exactly to satisfy Go
documentation conventions and align with the sibling method naming in
message.go.
- Line 133: Remove the debug artifact and privacy-unsafe logging from
RemoveOtherStampFromMessage in the repository/message.go implementation. Delete
the active fmt.Println call and replace the repo.logger.Info(userID.String())
usage with no user identifier logging at all, keeping only production-safe
context if needed. Make sure the cleanup is applied in the
RemoveOtherStampFromMessage flow and any related helper logic that currently
emits these messages.
In `@service/message/manager_impl.go`:
- Around line 325-328: The error wrapping in the includeOther branch uses the
wrong function name, which will mislead debugging in the
RemoveOtherStampFromMessage path. Update the fmt.Errorf message inside
manager_impl.go’s includeOther handling to match the actual repository call
m.R.RemoveOtherStampFromMessage, keeping the wrapped error context accurate for
that branch.
---
Outside diff comments:
In `@docs/v3-api.yaml`:
- Around line 705-733: The RemoveMessageStamp operation is missing the new
forbidden case in its API contract. Update the delete response definitions for
removeMessageStamp in the v3 API spec to include a 403 Forbidden response
alongside the existing 204 and 404 entries, matching the behavior where
message.ErrCannotRemoveStamp is mapped to herror.Forbidden when the caller lacks
permission to remove others’ stamps.
---
Nitpick comments:
In `@router/v3/messages.go`:
- Around line 18-21: Update DeleteStampsQuery to bind IncludeMe and IncludeOther
as booleans instead of strings, and adjust the corresponding handler logic to
use the parsed values directly rather than calling isTrue or manually defaulting
empty strings. Make the same change in the other affected query struct mentioned
by the review so the router types stay aligned with the OpenAPI boolean schema
and the existing query binding flow.
In `@service/message/manager_impl_test.go`:
- Around line 558-731: Add an explicit negative assertion for the failing
RemoveStamps permission paths so it is clear that RemoveOtherStampFromMessage is
never called when the bot/non-bot or own/other-message check rejects the
request. Update the relevant RemoveStamps subtests in manager_impl_test.go to
set a Times(0) expectation on MockMessageRepository.RemoveOtherStampFromMessage
for the cases that should fail, while keeping the existing success-path
expectations for the includeMe/includeOther cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a19549f4-28bc-4e98-b5a9-03c10aebe4a5
📒 Files selected for processing (9)
docs/v3-api.yamlrepository/gorm/message.gorepository/message.gorepository/mock_repository/mock_message.gorouter/v3/messages.goservice/message/manager.goservice/message/manager_impl.goservice/message/manager_impl_test.goservice/message/manager_test.go
…to feat/bot-remove-stamp
Takeno-hito
left a comment
Takeno-hito
left a comment
There was a problem hiding this comment.
実装ありがとうございます！コメント確認お願いします
| in: query | ||
| name: include-me | ||
| description: "自分が押したスタンプを削除します" | ||
| - schema: |
There was a problem hiding this comment.
q: パラメーターが2つある理由はある？ （include-me, include-other 片方だけじゃだめ？）
| type: boolean | ||
| default: false | ||
| in: query | ||
| name: include-other |
There was a problem hiding this comment.
imo: スキーマの部分事前に乗れなくてごめんだったけど、利用用途を考えるならどちらかというと "user_id" を指定して消せるようにする、の方が綺麗じゃないかなぁ…？ （ include-me, include-other はちょっとむずかしそうな使い方に見える）
MUST: これ見てるだけだと Bot 以外も使えそうだけど Bot 以外も使えるようにするのは微妙な気もするから、Bot 限定ってしたいかも！
There was a problem hiding this comment.
命名でどうにかするとしたら "remove-mine" "remove-others" とかの方がいいかも (include って入ってるのが変かもと思いました）
| } | ||
|
|
||
| // RemoveOtherStampFromMessage implements MessageRepository interface. | ||
| func (repo *Repository) RemoveOtherStampFromMessage(ctx context.Context, messageID, stampID, userID uuid.UUID) (err error) { |
There was a problem hiding this comment.
MUST* 命名が微妙かも せめて RemoveOtherUsersStamp と他の "USER" であることがわかるようにしてほしい！
もしくは、 API の形式を自分の提案通りにしたら、こういう例外処理を作る必要がそもそもなくなるはず！
|
|
||
| if result.RowsAffected > 0 { | ||
| for _, stamp := range ms { | ||
| if stamp.UserID == userID { |
| return herror.BadRequest(err) | ||
| } | ||
|
|
||
| if len(q.IncludeMe) == 0 { |
There was a problem hiding this comment.
q: bindAndValidate でこのバインド処理ってできない？（デフォ値って設定できない？）ちょっと実装が不思議かも…？
せめて、 `q.IncludeMe == "" かな len==0 の書き方はちょっと見づらい
| if includeMe { | ||
| if err := m.R.RemoveStampFromMessage(ctx, id, stampID, userID); err != nil { | ||
| return fmt.Errorf("failed to RemoveStampFromMessage: %w", err) | ||
| } | ||
| } | ||
| if includeOther { | ||
| if err := m.R.RemoveOtherStampFromMessage(ctx, id, stampID, userID); err != nil { | ||
| return fmt.Errorf("failed to RemoveOtherStampFromMessage: %w", err) | ||
| } |
There was a problem hiding this comment.
memo: transaction って traQ ないんだっけ
3 participants
close: #3021
DELETE /messages/:messageID/stamps/:stampIDにクエリパラメータを追加：include-metrueinclude-otherfalseservice/message/manager_impl.goincludeMe=trueincludeOther=trueSummary by CodeRabbit
New Features
include-meandinclude-otheroptions, letting you delete your stamps, others’ stamps, or both.Bug Fixes
Documentation