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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
12 changes: 12 additions & 0 deletions infra/db/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func (repo *GormRepository) UpsertEventSchedule(eventID, userID uuid.UUID, sched
return defaultErrorHandling(err)
}

func (repo *GormRepository) DeleteEventSchedule(eventID uuid.UUID, userID uuid.UUID) error {
err := deleteEventSchedule(repo.db, eventID, userID)
return defaultErrorHandling(err)
}

func (repo *GormRepository) GetEvent(eventID uuid.UUID) (*Event, error) {
es, err := getEvent(eventFullPreload(repo.db), eventID)
return es, defaultErrorHandling(err)
Expand Down Expand Up @@ -140,6 +145,13 @@ func upsertEventSchedule(tx *gorm.DB, eventID, userID uuid.UUID, schedule domain
}).Create(&eventAttendee).Error
}

func deleteEventSchedule(tx *gorm.DB, eventID uuid.UUID, userID uuid.UUID) error {
if eventID == uuid.Nil {
return NewValueError(gorm.ErrRecordNotFound, "eventID")
Luftalian marked this conversation as resolved.
Show resolved Hide resolved
}
return tx.Where("event_id = ? AND user_id = ?", eventID, userID).Delete(&EventAttendee{}).Error
}

func getEvent(db *gorm.DB, eventID uuid.UUID) (*Event, error) {
event := Event{}
err := db.Take(&event, eventID).Error
Expand Down
36 changes: 29 additions & 7 deletions usecase/production/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"github.com/traPtitech/knoQ/domain"
"github.com/traPtitech/knoQ/domain/filter"
"github.com/traPtitech/knoQ/infra/db"
"golang.org/x/exp/slices"
)

func (repo *Repository) CreateEvent(params domain.WriteEventParams, info *domain.ConInfo) (*domain.Event, error) {
Expand Down Expand Up @@ -49,22 +50,43 @@ func (repo *Repository) UpdateEvent(eventID uuid.UUID, params domain.WriteEventP
WriteEventParams: params,
CreatedBy: info.ReqUserID,
}

event, err := repo.GormRepo.UpdateEvent(eventID, p)
if err != nil {
return nil, defaultErrorHandling(err)
}

attendeesMap := make(map[uuid.UUID]domain.ScheduleStatus)
for _, attendee := range currentEvent.Attendees {
attendeesMap[attendee.UserID] = attendee.Schedule
}
Comment on lines +63 to +67
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.

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

とかにするといいのかな


count := 0
for _, groupMember := range group.Members {
exist := false
for _, currentAttendee := range currentEvent.Attendees {
if currentAttendee.UserID == groupMember.ID {
exist = true
}
}
if !exist {
if _, ok := attendeesMap[groupMember.ID]; !ok {
// 新しく主催者メンバーになった人をPendingにする
Luftalian marked this conversation as resolved.
Show resolved Hide resolved
_ = repo.GormRepo.UpsertEventSchedule(event.ID, groupMember.ID, domain.Pending)
count++
}
}

// 変更前の主催者メンバー全員が変更後の主催者メンバーであるとき
// (変更前主催者メンバーの数) = (変更後主催者メンバーの数) - (変更後主催者メンバーの中で新規主催者メンバーの数)
if len(attendeesMap) == (len(group.Members) - count) {
return repo.GetEvent(event.ID, info)
}

for attendeeUserId, schedule := range attendeesMap {
ok := slices.ContainsFunc(group.Members, func(m domain.User) bool {
return m.ID == attendeeUserId
})
// グループ外参加不可で主催者メンバーから外れた人を削除
// グループ外参加可で主催者メンバーから外れた人でPendingだった人を削除
if !ok && (!event.AllowTogether || schedule == domain.Pending) {
_ = repo.GormRepo.DeleteEventSchedule(event.ID, attendeeUserId)
}
}

return repo.GetEvent(event.ID, info)
}

Expand Down