fix: Event.removeExhibitMemberIdの未実装を修正#85
Conversation
展示からメンバーを削除し、他の展示にも所属していなければ イベントのメンバー一覧からも削除する。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Exhibit.hasMemberId()を追加しSet#hasベースで判定(Set→Array変換を回避) - removeExhibitMemberIdからEvent.memberIdsの削除を除去(展示所属とイベント参加の責務を分離) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| public removeExhibitMemberId(exhibitId: string, memberId: string): void { | ||
| this.getExhibitOrThrow(exhibitId).removeMemberId(memberId); | ||
| } |
There was a problem hiding this comment.
📝 Info: removeExhibitMemberId was previously a no-op — behavioral change for existing callers
The removeExhibitMemberId method previously had an empty body (Event.ts:138 old code), meaning RemoveMemberFromExhibit.ts:32 was calling it but nothing happened — members were never actually removed from exhibits. This PR fixes that, which is a behavioral change: previously the use case silently succeeded without modifying state, now it actually removes the member from the exhibit. This is clearly the intended fix, but it's worth noting as a non-trivial semantic change for any callers relying on the previous (broken) no-op behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.memberIds.delete(memberId); | ||
| } |
There was a problem hiding this comment.
🚩 Exhibit.removeMemberId silently succeeds for non-existent members
The Exhibit.removeMemberId method at Exhibit.ts:94-96 calls Set.delete() which silently returns false if the member doesn't exist. This means Event.removeExhibitMemberId will succeed without error even if the specified memberId was never a member of the exhibit. Depending on domain requirements, it may be desirable to throw an exception if the member is not found. This is a pre-existing design choice rather than a bug introduced by this PR, but worth considering for domain invariant enforcement.
(Refers to lines 94-96)
Was this helpful? React with 👍 or 👎 to provide feedback.
## Summary - `Event.removeExhibitMemberId`のボディが空で、展示からのメンバー削除がno-opだった問題を修正 - 該当Exhibitからメンバーを削除し、他のExhibitにも所属していなければEvent.memberIdsからも削除する - PR #81 のレビューで指摘された pre-existing issue への対応 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/core/pull/85" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Event.removeExhibitMemberIdのボディが空で、展示からのメンバー削除がno-opだった問題を修正Test plan
npx tsc --noEmitが通ることnpx biome checkが通ること🤖 Generated with Claude Code