Skip to content

fix: Event.removeExhibitMemberIdの未実装を修正 #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KinjiKawaguchi merged 4 commits into from
Mar 14, 2026
Merged

fix: Event.removeExhibitMemberIdの未実装を修正 #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d285451
fix: Event.removeExhibitMemberIdが未実装だった問題を修正
KinjiKawaguchi Mar 12, 2026
eee1c6e
fix: レビュー指摘に対応
KinjiKawaguchi Mar 14, 2026
f64dfb8
fix: Exhibit.toSnapshot()にmemberIdsを追加
KinjiKawaguchi Mar 14, 2026
1f61d81
docs: removeExhibitMemberIdの非対称性の意図をコメントで明示
KinjiKawaguchi Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/domain/aggregates/event/Event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export class Event {

public removeMemberId(memberId: string): void {
for (const exhibit of this.exhibits) {
if (exhibit.getMemberIds().includes(memberId)) {
if (exhibit.hasMemberId(memberId)) {
throw new ExhibitHasMemberException(exhibit.id, memberId);
}
}
Expand All @@ -135,7 +135,12 @@ export class Event {
this.memberIds.add(memberId);
}

public removeExhibitMemberId(exhibitId: string, memberId: string): void {}
// NOTE: 展示からの削除のみ行い、Event.memberIdsは操作しない。
// 「展示に所属していないがイベント参加者」という状態が有効なため、
// イベント参加者からの削除はremoveMemberId()で明示的に行う。
public removeExhibitMemberId(exhibitId: string, memberId: string): void {
this.getExhibitOrThrow(exhibitId).removeMemberId(memberId);
}
Comment thread
KinjiKawaguchi marked this conversation as resolved.
Comment thread
KinjiKawaguchi marked this conversation as resolved.
Comment thread
KinjiKawaguchi marked this conversation as resolved.
Comment on lines +141 to +143

@devin-ai-integration devin-ai-integration Bot Mar 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

toSnapshot() {
return {
id: this.id,
Expand Down
5 changes: 5 additions & 0 deletions src/domain/aggregates/event/Exhibit.ts
View file
Open in desktop
Comment thread
KinjiKawaguchi marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ export class Exhibit {
this.memberIds.delete(memberId);
}
Comment on lines 95 to 96

@devin-ai-integration devin-ai-integration Bot Mar 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 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)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


public hasMemberId(memberId: string): boolean {
return this.memberIds.has(memberId);
}
Comment thread
KinjiKawaguchi marked this conversation as resolved.

private getLightningTalkOrThrow(): LightningTalk {
if (!this.lightningTalk) {
throw new LightningTalkNotFoundException(
Expand All @@ -112,6 +116,7 @@ export class Exhibit {
markdownContent: this.markdownContent,
url: this.url,
lightningTalk: this.lightningTalk?.toSnapshot(),
memberIds: Array.from(this.memberIds),
Comment thread
KinjiKawaguchi marked this conversation as resolved.
};
}
}