Skip to content
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.

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

イベントの主催の変更後にも以前の主催に#services/knoQで通知が飛ぶ #404

Open
sapphi-red opened this issue May 29, 2023 · 19 comments · May be fixed by #447
Open

イベントの主催の変更後にも以前の主催に#services/knoQで通知が飛ぶ #404

sapphi-red opened this issue May 29, 2023 · 19 comments · May be fixed by #447
Assignees
Labels
bug Something isn't working

Comments

@sapphi-red
Copy link

https://q.trap.jp/messages/9b602aa3-d266-4f09-bba6-c4c226b4bc06

@sapphi-red sapphi-red added the bug Something isn't working label May 29, 2023
@Luftalian Luftalian self-assigned this Jul 18, 2023
@Luftalian
Copy link
Member

for _, groupMember := range group.Members {
exist := false
for _, currentAttendee := range currentEvent.Attendees {
if currentAttendee.UserID == groupMember.ID {
exist = true
}
}
if !exist {
_ = repo.GormRepo.UpsertEventSchedule(event.ID, groupMember.ID, domain.Pending)
}
}

knoQ/router/middleware.go

Lines 218 to 225 in cb9fc32

for _, attendee := range e.Attendees {
if attendee.Schedule == presentation.Pending {
user, ok := usersMap[attendee.ID]
if ok {
nofiticationTargets = append(nofiticationTargets, user.Name)
}
}
}

  • 主催が変わったとき、変更前の主催のメンバーが変更後の主催に含まれない場合、そのメンバーは出席状況が未定に戻される。
  • Webhookは出席状況が未定の全てのユーザーにメンションが飛ぶ

上のような状況になっているため、このIssueが起きているようです。

@Luftalian
Copy link
Member

Luftalian commented Jul 19, 2023

変更案としては、

  1. Webhookの通知を飛ばすのを出席状況が未定の全てのユーザーではなく、Groupのメンバーの中で出席状況が未定のユーザーのみにする。
  2. 主催が変わったとき、変更前の主催のメンバーが変更後の主催に含まれない場合、そのメンバーをイベントのメンバーから削除する

が考えられそうです。
@ras0q どっちのほうがいいですか?

@ras0q
Copy link
Member

ras0q commented Jul 20, 2023

調査ありがとうございます!

個人的には2がよさそうなんですが、新グループに含まれないかつ予定を記入してないことを条件にすると足りそうなきがします

@Luftalian
Copy link
Member

新グループに含まれないかつ予定を記入してないことメンバーのみ削除するということですか?

@Luftalian
Copy link
Member

グループ外からの参加ができないイベントの場合は、新グループに含まれないメンバーは問答無用で削除でいいですか?

@ras0q
Copy link
Member

ras0q commented Jul 24, 2023

この仕様忘れてました、これでOKです

グループ外からの参加ができないイベントの場合は、新グループに含まれないメンバーは問答無用で削除でいいですか?

グループ外からもOKな場合はこれでお願いします:pray:

新グループに含まれないかつ予定を記入してないことメンバーのみ削除するということですか?

@ras0q
Copy link
Member

ras0q commented Jul 24, 2023

グループ外NGのときorグループ外NGに変更するときはUIからwarning出るようにしたいかも
フロントエンドの方にissue建てておきます

@Luftalian
Copy link
Member

Luftalian commented Aug 2, 2023

変更方針

前の主催チームのメンバーが後の主催チームのメンバーでもあった場合、出席状況はそのまま追加する
前の主催チームのメンバーが後の主催チームのメンバーでなかった場合、出席状況は以下の通りにする

  1. チーム外参加不可の場合、問答無用で出席状況を削除する。(Pendingではない)
  2. {チーム外参加可}かつ{前の主催チームのメンバーの出席状況が出席Attendance}の場合はそのまま追加する。
  3. {チーム外参加可}かつ{前の主催チームのメンバーの出席状況が欠席Absent}の場合はそのまま追加する。(or 出席状況を削除する。(Pendingではない))???
  4. {チーム外参加可}かつ{前の主催チームのメンバーの出席状況が未定Pending}の場合は出席状況を削除する。(Pendingではない)

現状

前の主催チームのメンバーが後の主催チームのメンバーでもあった場合、出席状況はそのまま追加する
前の主催チームのメンバーが後の主催チームのメンバーでなかった場合、前の主催チームのメンバーの出席状況を全てPendingにしている

現状の問題の原因

WebhookではPendingのメンバー全てにメンションするため、出席状況を書いていなかった前の主催チームのメンバーには必ずメンションが飛ぶ

@ras0q
Copy link
Member

ras0q commented Aug 2, 2023

        for _, groupMember := range group.Members {
                exist := false
+               schedule := domain.Pending
                for _, currentAttendee := range currentEvent.Attendees {
                        if currentAttendee.UserID == groupMember.ID {
                                exist = true
                        }
+                       schedule = currentAttendee.Schedule
                }
-               if !exist {
-                       _ = repo.GormRepo.UpsertEventSchedule(event.ID, groupMember.ID, domain.Pending)
+               if !exist && schedule == domain.Pending {
+                       _ = repo.GormRepo.DeleteMemberOfGroup(eventID, groupMember.ID)
                }
 
        }

@ras0q
Copy link
Member

ras0q commented Aug 2, 2023

出席または欠席にしていたらそのまま残す
何も入力していないorPending(内部ではどちらもPendingとして扱われている)の場合は削除

@ras0q
Copy link
Member

ras0q commented Aug 2, 2023

DeleteMemberOfGroupは別のメソッドなので別途作る必要がある

@ras0q
Copy link
Member

ras0q commented Aug 2, 2023

	attendeeMap := make(map[uuid.UUID]domain.ScheduleStatus)
	for _, attendee := range currentEvent.Attendees {
		attendeeMap[attendee.UserID] = attendee.Schedule
	}

	for _, groupMember := range group.Members {
		_, ok := attendeeMap[groupMember.ID]
		if !ok {
			_ = repo.GormRepo.UpsertEventSchedule(event.ID, groupMember.ID, domain.Pending)
		}
	}

	for aid, schedule := range attendeeMap {
		ok := slices.ContainsFunc(group.Members, func(m domain.User) bool { return m.ID == aid })
		if !ok && schedule == domain.Pending {
			_ = repo.GormRepo.DeleteEventSchedule(event.ID, aid)
		}
	}

@Luftalian
Copy link
Member

Luftalian commented Aug 2, 2023

event.AllowTogetherを考慮した

attendeeMap := make(map[uuid.UUID]domain.ScheduleStatus)
for _, attendee := range currentEvent.Attendees {
	attendeeMap[attendee.UserID] = attendee.Schedule
}

for _, groupMember := range group.Members {
	_, ok := attendeeMap[groupMember.ID]
	if !ok {
		_ = repo.GormRepo.UpsertEventSchedule(event.ID, groupMember.ID, domain.Pending)
	}
}

for aid, schedule := range attendeeMap {
	ok := slices.ContainsFunc(group.Members, func(m domain.User) bool { return m.ID == aid })
	if !ok && (!event.AllowTogether || schedule == domain.Pending) {
		_ = repo.GormRepo.DeleteEventSchedule(event.ID, aid)
	}
}

@Luftalian
Copy link
Member

image (3)

@Luftalian
Copy link
Member

8個ぐらい試してみないといけないパターンがあると思っているんですが、ローカル環境でどうやって管理者外の場合とかを試せばいいかわからないです。

@ras0q
Copy link
Member

ras0q commented Aug 14, 2023

手で直接確かめるのは2人必要なのでなかなか難しそうですね
UpdateEventのテストを書けると良さそう
詰まったら教えてください

@Luftalian
Copy link
Member

テストのためにproductionパッケージのRepository構造体を用意するところでGormRepo db.GormRepositoryをどうやって作ればいいのかわかりません。

@ras0q
Copy link
Member

ras0q commented Aug 15, 2023

https://github.com/DATA-DOG/go-sqlmock を使ってモックを書くか https://github.com/ory/dockertest とかで一時的にテスト用のDBを立てると良いと思います
どっちもtraPortfolioに書いてあるので参考にしてもらえると

@Luftalian Luftalian linked a pull request Aug 28, 2023 that will close this issue
@Luftalian
Copy link
Member

テストは書くのが難しそうなのでテスト作成は保留となった

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants