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

UpdateEventの動作を変更する #447

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

UpdateEventの動作を変更する #447

wants to merge 4 commits into from

Conversation

Luftalian
Copy link
Member

Close #404

@Luftalian Luftalian requested a review from ras0q August 28, 2023 11:41
Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

いくつか書きました
処理の流れが少し複雑なので、新規メンバーはpendingで追加することと条件に合わないメンバーは予定を削除することをコメントで書いておくと良いかもです

usecase/production/event.go Outdated Show resolved Hide resolved
usecase/production/event.go Outdated Show resolved Hide resolved
Comment on lines +58 to +62

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

Choose a reason for hiding this comment

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

これより後ろの処理はグループが変わらなかったら行わなくても良い処理だと思うので、ここらへんで早期リターンするとよさそうです

Copy link
Member Author

Choose a reason for hiding this comment

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

変更前の主催者メンバー全員が変更後の主催者メンバーであるとき早期リターンします

具体的には
(変更前主催者メンバーの数) = (変更後主催者メンバーの数) - (変更後主催者メンバーの中で新規主催者メンバーの数)
で取ります

Copy link
Member

Choose a reason for hiding this comment

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

currentEvent.GroupID == params.GroupIDで取ることを想定していました

Copy link
Member Author

Choose a reason for hiding this comment

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

たしかにそっちのほうがわかりやすそうです

一方で、
(変更前主催者メンバーの数) = (変更後主催者メンバーの数) - (変更後主催者メンバーの中で新規主催者メンバーの数)だと

変更前 Aさん Bさん
変更後 Aさん Bさん Cさん

みたいな状況のときもできそうです。

Copy link
Member

@ras0q ras0q Sep 2, 2023

Choose a reason for hiding this comment

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

なるほど
ちょっと考えたけど本質的には

  • 新しいメンバーが追加されていないこと
  • 削除するべきメンバーがいないこと

をチェックしたいから1つ目をboolでもって
if !newMemberAdded && len() == len()みたいに書くと分かりやすいかも?です

Copy link
Member Author

Choose a reason for hiding this comment

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

if !newMemberAdded && len() == len()if currentEvent.Group.ID == params.GroupIDは同じ動作になりそうだと思ったのですがどこが違いますでしょうか。

あと、一番最初にras0qさんが変更を入れてほしいとおっしゃられた場所と、実際に私が(変更前グループメンバーの数) = (変更後グループメンバーの数) - (変更後グループメンバーの中で新規グループメンバーの数)の変更を入れていた場所が違いました。

Copy link
Member Author

Choose a reason for hiding this comment

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

今の変更点で不味そうなところがあったら、教えていただきたいです。

Copy link
Member

Choose a reason for hiding this comment

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

返信が遅くなってすみません

(変更前グループメンバーの数) = (変更後グループメンバーの数) - (変更後グループメンバーの中で新規グループメンバーの数)
これなぜかずっと新規メンバーがいるかどうかを確認してると思ってたんですけど削除するメンバーがいるかどうかを確認してるんですね

Copy link
Member

Choose a reason for hiding this comment

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

- // 変更前のグループメンバー全員が変更後のグループメンバーであるとき
+ // 変更後に参加者から除外されるメンバーがいないとき

とかにするといいのかな

@Luftalian Luftalian requested a review from ras0q August 28, 2023 13:02
Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

いくつか書きました
それ以外は動作確認したら良さそう👍

usecase/production/event.go Outdated Show resolved Hide resolved
infra/db/event.go Show resolved Hide resolved
@ras0q
Copy link
Member

ras0q commented Sep 15, 2023

そういえば直接関係ないけどこの前こんなissueがあるのを見つけた
#100

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

コメントだけ返しました!
動作はいいと思います
一応動作確認だけして欲しいです🙏

@Luftalian
Copy link
Member Author

#404 (comment)
#404 (comment)
#404 (comment)
こういう経緯で動作確認できていないんですけど、どうすればいいですか?

@ras0q
Copy link
Member

ras0q commented Dec 21, 2023

ここではテストは書かなくていいので手元でテストができればOKです
ローカルでやるならDBを直接書き替えていろいろ確認してみるといいと思います
Webhook IDはテスト用チャンネルに投稿されるものを作って設定すると良いです

主催をA&BグループからAグループに変更したとき

  • Aの出欠状況が変わらない
  • Bが未定のとき変更後イベントメンバーから消える
  • 外部参加OKかつBが入力済みのとき変更後も消えない
  • 外部参加NGかつBが入力済みのとき変更後は消える

くらいを確かめればいいかな

@Luftalian
Copy link
Member Author

@ras0q
This pull request is what I was referring to the other day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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