授業追加時の重複チェックを実装 #665
Merged
授業追加時の重複チェックを実装 #665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
HosokawaR
force-pushed
the
feature/lecture-addition-duplicate-check
branch
from
e35aa06 to
48d4bcc
Compare
HosokawaR
force-pushed
the
feature/lecture-addition-duplicate-check
branch
from
48d4bcc to
64baddb
Compare
HosokawaR
force-pushed
the
feature/lecture-addition-duplicate-check
branch
from
64baddb to
75b20cb
Compare
HosokawaR
commented
Apr 2, 2023
Comment on lines
-476
to
-480
| await Promise.all( | ||
| selectedSearchResults.filter( | ||
| ({ schedules }) => | ||
| !Usecase.checkScheduleDuplicate(ports)(year.value, schedules) | ||
| ) |
There was a problem hiding this comment.
以下の理由で常に !Usecase.checkScheduleDuplicate が false になっていました
await Promise.all(
selectedSearchResults.filter(
({ schedules }) =>
!Usecase.checkScheduleDuplicate(ports)(year.value, schedules) // !Boolean(Promise<T>) なので常に false になる
)
)
HosokawaR
commented
Apr 2, 2023
Comment on lines
+482
to
+489
| const duplicatedCourses = selectedSearchResults | ||
| .filter(({ course: { code: selectedCourseCode } }) => { | ||
| return registeredCourse.some( | ||
| ({ code: registeredCourseCode }) => | ||
| registeredCourseCode === selectedCourseCode | ||
| ); | ||
| }) | ||
| .map(({ course }) => course); |
There was a problem hiding this comment.
ここらへんを checkScheduleDuplicate のように Usecase にするべきかどうかは迷ったので意見があればぜひ！
There was a problem hiding this comment.
ありがとうございます！エラー数が増えてきたので一旦これで Merge しますが、後ほど UseCase に直します。
Ref: #666
hayato24s
approved these changes
Apr 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #664
変更点
この PR の変更点は以下です。
重複スケジュール検知のロジックがバグっていたので修正しました ( 7c9c727 )
Promiseを待たずに.filterを適応しているなどのミスで、常に重複スケジュールがないと判断されていました。このロジックは今回の機能追加と関連がある領域だったので、この PR で直しました。
↓が出てくるように修正した
すでに登録した講義がある場合に Toast でエラーを通知するようにしました ( 45fb9e3 )
1.はあくまで重複スケジュールがあったときの警告で、さらに通常講義のみに限定されます。したがって 1 とは別に同じ講義を追加しようとしたらエラー Toast を出現させる仕組みが必要です。これをこのコミットで追加しました。また Toast の表示時間も調整しました。
前述の
2.のエラーに遭遇したユーザ数を数えるための Tracking を追加しました ( 75b20cb )Sentry を見ると同様のエラーに遭遇したユーザは多く、そもそも重複した講義を登録しようとさせない UX が必要かもしれません（登録済みの講義はチェックボックスが押せないなど）
今回は暫定的な対応で修正しますが、今後上記のような改善をする場合、どれぐらいのユーザがこのエラー Toast を見たのかを知るために、Tracking を追加しました。