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

MFM의 shouldNyaize관련 논의 #409

Open
kdh8219 opened this issue Jan 14, 2024 · 2 comments
Open

MFM의 shouldNyaize관련 논의 #409

kdh8219 opened this issue Jan 14, 2024 · 2 comments

Comments

@kdh8219
Copy link

kdh8219 commented Jan 14, 2024

const shouldNyaize = props.nyaize ? props.nyaize === 'respect' ? props.author?.isCat : false : false;

nyize는 true, false, 'respect'를 가질 수 있는데
각각 nyize 강제적용, 강제 미적용, 계정주의 isCat 여부에 따름 으로 되야 할 것으로 예상했는데
실 동작은 강제 미적용, 강제 미적용, 계정주의 isCat 여부에 따름 으로 되어있었습니다

const shouldNyaize = props.nyaize ? props.nyaize === 'respect' ? props.author?.isCat : true : false;

로 바꾸는건 어떨까요?

@noridev
Copy link
Collaborator

noridev commented Jan 15, 2024

해당 코드는props.nyaize === 'respect' 인 경우 사용자가 isCat이면 true를. 아니면 false를 반환하는 코드로 보입니다.
말씀하신 부분을 true로 변경하게 되면, 'respect'가 아닌 상태(nyaize를 true 또는 false로 강제하는 경우)에서는 무조건 true를 반환하므로 적합하지 않은 변경으로 보입니다.

@kdh8219
Copy link
Author

kdh8219 commented Jan 15, 2024

nyaize == 'respect' && isCat을 의도했으면 &&로 두 조건을 묶어뒀을 것 같아서 든 생각이었습니다

그리고 제가 건의드린대로 바꾸더라도 nyaize가 false일때도 true가 반환되는 일은 없습니다(nyaize가 false면 첫 번째 조건에서 false로 직행합니다)
IMG_5754

슈이로가 실수해놓은 부분 같아서 이슈 열었던건데 진짜 실수가 맞더라도 미스키측에 이슈 여는게 맞을 것 같기도 하네요

noridev pushed a commit that referenced this issue Mar 20, 2024
* Never return broken notifications #409

Since notifications are stored in Redis, we can't expect relational
integrity: deleting a user will *not* delete notifications that
mention it.

But if we return notifications with missing bits (a `follow` without a
`user`, for example), the frontend will get very confused and throw an
exception while trying to render them.

This change makes sure we never expose those broken notifications. For
uniformity, I've applied the same logic to notes and roles mentioned
in notifications, even if nobody reported breakage in those cases.

Tested by creating a few types of notifications with a `notifierId`,
then deleting their user.

(cherry picked from commit 421f8d49e5d7a8dc3a798cc54716c767df8be3cb)

* Update Changelog

* Update CHANGELOG.md

* enhance: 通知がミュートを考慮するようにする

* enhance: 通知が凍結も考慮するようにする

* fix: notifierIdがない通知が消えてしまう問題

* Add tests (通知がミュートを考慮しているかどうか)

* fix: notifierIdがない通知が消えてしまう問題 (grouped)

* Remove unused import

* Fix: typo

* Revert "enhance: 通知が凍結も考慮するようにする"

This reverts commit b1e57e571dfd9a7d8b2430294473c2053cc3ea33.

* Revert API handling

* Remove unused imports

* enhance: Check if notifierId is valid in NotificationEntityService

* 通知作成時にpackしてnullになったらあとの処理をやめる

* Remove duplication of valid notifier check

* add filter notification is not null

* Revert "Remove duplication of valid notifier check"

This reverts commit 239a6952f717add53d52c3e701e7362eb1987645.

* Improve performance

* Fix packGrouped

* Refactor: 判定部分を共通化

* Fix condition

* use isNotNull

* Update CHANGELOG.md

* filterの改善

* Refactor: DONT REPEAT YOURSELF
Note: GroupedNotificationはNotificationの拡張なのでその例外だけ書けば基本的に共通の処理になり複雑な個別の処理は増えにくいと思われる

* Add groupedNotificationTypes

* Update misskey-js typedef

* Refactor: less sql calls

* refactor

* clean up

* filter notes to mark as read

* packed noteがmapなのでそちらを使う

* if (notesToRead.size > 0)

* if (notes.length === 0) return;

* fix

* Revert "if (notes.length === 0) return;"

This reverts commit 22e2324f9633bddba50769ef838bc5ddb4564c88.

* 🎨

* console.error

* err

* remove try-catch

* 不要なジェネリクスを除去

* Revert  (既読処理をpack内で行うものを元に戻す)

* Clean

* Update packages/backend/src/core/entities/NotificationEntityService.ts

* Update packages/backend/src/core/entities/NotificationEntityService.ts

* Update packages/backend/src/core/entities/NotificationEntityService.ts

* Update packages/backend/src/core/entities/NotificationEntityService.ts

* Update packages/backend/src/core/NotificationService.ts

* Clean

---------

Co-authored-by: dakkar <[email protected]>
Co-authored-by: kakkokari-gtyih <[email protected]>
Co-authored-by: かっこかり <[email protected]>
Co-authored-by: tamaina <[email protected]>
Co-authored-by: syuilo <[email protected]>
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

No branches or pull requests

2 participants