-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: 通知からロール参照時にロールが存在しないときの例外処理を追加 #14500
base: develop
Are you sure you want to change the base?
fix: 通知からロール参照時にロールが存在しないときの例外処理を追加 #14500
Conversation
このPRによるapi.jsonの差分 差分はこちら |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #14500 +/- ##
===========================================
- Coverage 41.37% 39.63% -1.74%
===========================================
Files 1543 1539 -4
Lines 196679 190971 -5708
Branches 3547 3504 -43
===========================================
- Hits 81371 75688 -5683
+ Misses 114714 114689 -25
Partials 594 594 ☔ View full report in Codecov by Sentry. |
packages/backend/src/core/entities/NotificationEntityService.ts
Outdated
Show resolved
Hide resolved
- catchブロックでのeという変数名は混同の危険性があるため利用しない - 保守性のため型チェックでのエラーハンドリングを行う
@@ -140,7 +139,10 @@ export class NotificationEntityService implements OnModuleInit { | |||
// #endregion | |||
|
|||
const needsRole = notification.type === 'roleAssigned'; | |||
const role = needsRole ? await this.roleEntityService.pack(notification.roleId) : undefined; | |||
const role = needsRole ? await this.roleEntityService.pack(notification.roleId).catch((err) => { | |||
if (err instanceof EntityNotFoundError) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここはApiではないのでApiErrorを使わない。
元々ロールが処理されておりnullが返ってきたときの処理が直後にあったのでその意向に従い、このreturnでnullはroleに入り、直後の if(needsRole && !role) でロールが削除されていた時の処理に入り、メソッドを抜ける
様々なPRは #13929 (comment) が終わってから入れます🙏 |
Off topic: リバートのリバートが用意できてもそれを入れるには #14529 での議論を完了させる必要がある気がする |
それはあまり関係なさそう |
入れてからというよりPRがある状態になってから |
→ #14546 |
roleEntityService.packがエラーをハンドリングしてEntityNotFoundError以外の方法でエラーを表現するようになった場合に壊れそうという懸念があるわね |
EntityNotFoundErrorはTypeORMのエラーなので実際にそうなる可能性は少なくない |
TypeScriptでその関数がどのようなエラーを発生させる可能性があるかを型定義できたりすれば解決するけどできない気がする |
What
このプルリクエストでは、通知に含まれるロールが削除されている場合に発生する例外を処理し、削除されたロールに対応した通知の処理をスキップする修正を加えました。
具体的には、roleEntityService.pack(notification.roleId)を実行する際に、ロールが存在しない場合にはEntityNotFoundErrorをキャッチして、nullを返すように変更しています。
これにより、通知が「エラーが発生しました」と表示される問題を解消します。
また、不要なESLintの注釈がエディタの機能により自動的に削除されています。
Why
現在、削除されたロールを参照しようとすると、DBからnullが返ってきて例外が発生し、ユーザーにはエラーメッセージしか表示されなくなってしまいます。
この問題を解消するために、削除されたロールに関連する通知の処理をスキップするようにしました。
これにより、ユーザーに対して不要なエラーメッセージが表示されることがなくなります。
Additional info (optional)
ローカル環境でのテストを行い、問題が解消されたことを確認しています。
Checklist