add attendee info to ical #445

iChemy · 2023-08-02T09:53:20Z

mailtoは一旦
mailto:@ユーザー名のような形にしています．参考

(確認したところ@が入ってないとappleのカレンダーではうまく参加者として表示されなかったため)

attendeeのユーザーIDからユーザー名や表示名を取るために
func (repo *Repository) GetAtendeeMap(events []*domain.Event, info *domain.ConInfo)
をusecase/production以下に作成しました．参考

ras0q

icalのライブラリを同時に2つ使うのは怖いので、置き換えるPRも出してもらえると助かります！🙏(別PRでも同PRでもいいです)

ras0q

幾つかこめんとしました！

ras0q · 2023-08-02T10:05:16Z

usecase/production/event.go

+	for _, event := range events {
+		for _, attendee := range event.Attendees {
+			user, err := repo.GetUser(attendee.UserID, info)


for文の(しかも2重の)中でGetUserを回すのは避けたいです
GetUserをたくさん回すならGetAllUsersを1回叩いてそれをmapにする方がパフォーマンスがよくなりますね

ras0q · 2023-08-02T12:19:33Z

presentation/event.go

@@ -214,3 +173,51 @@ func ConvdomainEventToEventDetailRes(src domain.Event) (dst EventDetailRes) {
 	dst.Model = Model(src.Model)
 	return
 }
+
+func iCalVeventFormat(e *domain.Event, host string, attendeeMap map[uuid.UUID]*domain.User) *ics.VEvent {


中身とは関係ないけど差分減らすために元と同じ位置に書くのがよさそう

ras0q · 2023-08-02T12:21:41Z

presentation/event.go

+	vevent.SetDtStampTime(time.Now())
+	vevent.SetStartAt(e.TimeStart)
+	vevent.SetEndAt(e.TimeEnd)
+	vevent.SetCreatedTime(e.CreatedAt)
+	vevent.SetModifiedAt(e.UpdatedAt)


FormatはしなくてよくなったんですがUTC()はつけたままでよさそうです

ras0q · 2023-08-02T12:58:26Z

presentation/event.go

+		case domain.Attendance:
+			vevent.AddAttendee(fmt.Sprintf("@%s", userName), ics.ParticipationStatusAccepted, ics.WithCN(userDisplayName))
+		case domain.Absent:
+			vevent.AddAttendee(fmt.Sprintf("@%s", userName), ics.ParticipationStatusDeclined, ics.WithCN(userDisplayName))
+		default:
+			vevent.AddAttendee(fmt.Sprintf("@%s", userName), ics.WithCN(userDisplayName))
+		}


ここらへんはfmt.Sprintfとics.WithCNを外に出してまとめておくと可読性が上がると思います

iChemy · 2023-08-18T11:07:30Z

改善してみました．
お時間あるときにレビューをしていただけると嬉しいです:bow:

ras0q

コメント保留したまま出し忘れてました、ごめんなさい
いくつか書きました🙏

presentation/event.go

iChemy · 2023-08-27T12:46:17Z

再度見てもらえると嬉しいです．何度もすみません

ras0q · 2023-08-27T14:07:06Z

router/events.go

 	var buf bytes.Buffer
-	_ = ical.NewEncoder(&buf).Encode(cal)
+	cal.SerializeTo(&buf)


ここlinterに怒られてるので変更前みたいに

Suggested change

cal.SerializeTo(&buf)

_ = cal.SerializeTo(&buf)

してほしいです:pray: (handlingしてもいいけど)

ras0q · 2023-08-27T14:08:51Z

router/events.go

+	userMap, err := h.Repo.GetUserMap(info)
+	if err != nil {
+		return judgeErrorResponse(err)
+	}


ココでしか使わないならGetUserMapは消してここにそのまま書く方がinterfaceへのメソッドの追加も防げていいのかなと思いました

ras0q

少しだけ書きました:eyes:

iChemy · 2023-08-28T07:47:04Z

再びご確認いただけますでしょうか.
お手数をおかけしております 😿

ras0q

いくつかお願いします!
多分これ終わったらマージできます

ras0q · 2023-08-28T08:40:04Z

router/events.go

+		if errors.Is(domain.ErrInvalidToken, err) {
+			return forbidden(err, message("token is invalid."), needAuthorization(true))
+		}


ここはjudgeErrorResponseに同じような記述があるのでそっちに処理を任せれば十分だと思います

ras0q · 2023-08-28T08:40:13Z

router/events.go

@@ -245,12 +246,25 @@ func (h *Handlers) HandleGetiCalByPrivateID(c echo.Context) error {
 	if err != nil {
 		return judgeErrorResponse(err)
 	}
-	userMap, err := h.Repo.GetUserMap(info)
+	users, err := h.Repo.GetAllUsers(false, true, info)
+


ココの空行いらないです

上の空行に関連して疑問に思ったのですが以下の部分(// この行に空行いれた方がいいですか?)に空行はいれた方がいいでしょうか?

events, err := h.Repo.GetEvents(expr, info) if err != nil { return judgeErrorResponse(err) } // この行に空行いれた方がいいですか? users, err := h.Repo.GetAllUsers(false, true, info) if err != nil { return judgeErrorResponse(err) }

エラーハンドリング周りのコードは空行無い方が見やすいのでいれない方がいいのはわかったのですが逆に空行を入れるべきときはいつなのか気になりました．

強い気持ちはないですが個人的には入れたほうがいいです
https://github.com/bombsimon/wsl とかを使うと指摘されます

golangci-lintにもあります
https://golangci-lint.run/usage/linters/#wsl

ras0q · 2023-08-28T08:41:03Z

router/events.go

 	if err != nil {
 		return judgeErrorResponse(err)
 	}


同じerrに対して2回エラーハンドリングしてるためこっちは消してOKです

ras0q · 2023-08-28T08:42:13Z

presentation/event.go

-	_ = vevent.AddProperty("summary", e.Name)
+func iCalVeventFormat(e *domain.Event, host string, userMap map[uuid.UUID]*domain.User) *ics.VEvent {
+	vevent := ics.NewEvent(e.ID.String())
+	vevent.SetDtStampTime(time.Now())


UTCにしとくのが無難かも

ras0q · 2023-08-28T08:55:07Z

presentation/event.go

+		userDisplayName := ics.WithCN(user.DisplayName)
+		switch v.Schedule {
+		case domain.Attendance:
+			vevent.AddAttendee(userName, ics.ParticipationStatusAccepted, userDisplayName)
+		case domain.Absent:
+			vevent.AddAttendee(userName, ics.ParticipationStatusDeclined, userDisplayName)
+		default:
+			vevent.AddAttendee(userName, userDisplayName)
+		}
+	}


デフォルトはics.ParticipationStatusNeedsActionでいい気がする
その上で↓のように書くと若干きれいになるかもです
participationStatusはgoならpsとかでもいい

var participationStatus ParticipationStatus switch v.Schedule { case domain.Attendance: participationStatus = ics.ParticipationStatusAccepted case domain.Absent: participationStatus = ics.ParticipationStatusDeclined default: participationStatus = ics.ParticipationStatusNeedsAction } vevent.AddAttendee(userName, ics.WithCN(user.DisplayName), participationStatus) }

ras0q · 2023-08-28T08:58:33Z

router/events.go

+	if err != nil {
+		return judgeErrorResponse(err)
+	}


上で既にハンドリングしてるのでここはいらないです

ras0q

良さそうです:+1:

add attendee info to ical

a422a9e

ras0q reviewed Aug 2, 2023

View reviewed changes

🔥 remove former ical formatter

7ac759e

ras0q requested changes Aug 2, 2023

View reviewed changes

🎨 improve readability and change some bad code

7ce4e25

ras0q requested changes Aug 20, 2023

View reviewed changes

presentation/event.go Outdated Show resolved Hide resolved

presentation/event.go Outdated Show resolved Hide resolved

presentation/event.go Outdated Show resolved Hide resolved

⚡ change variable name, etc.

8180095

ras0q reviewed Aug 27, 2023

View reviewed changes

ras0q requested changes Aug 27, 2023

View reviewed changes

🗑️ delete CreateUserMap and _

4490ab9

ras0q requested changes Aug 28, 2023

View reviewed changes

🎨 code formatting, delete bad code, and, add UTC

628d8cf

ras0q approved these changes Aug 28, 2023

View reviewed changes

iChemy merged commit e5ef51f into main Aug 28, 2023
4 checks passed

iChemy deleted the fix/issue-218 branch August 28, 2023 11:55

ras0q mentioned this pull request Aug 28, 2023

配信するiCalに"ATTENDEE"."PARTSTAT"を追加 #218

Closed

ras0q mentioned this pull request Feb 11, 2024

icalの参加者機能が機能していない #526

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add attendee info to ical #445

add attendee info to ical #445

iChemy commented Aug 2, 2023

ras0q left a comment

ras0q left a comment

ras0q Aug 2, 2023

ras0q Aug 2, 2023

ras0q Aug 2, 2023

ras0q Aug 2, 2023

iChemy commented Aug 18, 2023

ras0q left a comment

iChemy commented Aug 27, 2023

ras0q Aug 27, 2023

ras0q Aug 27, 2023

ras0q left a comment

iChemy commented Aug 28, 2023

ras0q left a comment

ras0q Aug 28, 2023

ras0q Aug 28, 2023

iChemy Aug 28, 2023

ras0q Aug 28, 2023

ras0q Aug 28, 2023

ras0q Aug 28, 2023

ras0q Aug 28, 2023

ras0q Aug 28, 2023

ras0q Aug 28, 2023

ras0q left a comment

add attendee info to ical #445

add attendee info to ical #445

Conversation

iChemy commented Aug 2, 2023

ras0q left a comment

Choose a reason for hiding this comment

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iChemy commented Aug 18, 2023

ras0q left a comment

Choose a reason for hiding this comment

iChemy commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0q left a comment

Choose a reason for hiding this comment

iChemy commented Aug 28, 2023

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0q left a comment

Choose a reason for hiding this comment