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

Q&Aで質問者にメンション付き回答を送ると、「メンション」と「Watch」両方の通知(+メール)が来ないように修正した #3529

Conversation

Ichiyo14
Copy link
Contributor

@Ichiyo14 Ichiyo14 commented Nov 7, 2021

issue:#2696

Q&Aで質問者にメンション付き回答を送ると、「メンション」と「Watch」両方の通知(+メール)が来ないようにした。

通知及びメールを1回答に1つのみ届くようにした。

回答者の通知状況

質問者:kimura
回答者:hatuno
質問タイトル:通知テスト

質問者が質問をwatchしている

メンションの宛先 メンション メール
なし kimuraさんの【 「通知テスト」のQ&A 】にhatsunoさんがコメントしました。 「通知テスト」のQ&Aにhatsunoさんがコメントしました。
質問者あて hatsunoさんからメンションがきました。 hatsunoさんからメンションがありました
他ユーザーあて kimuraさんの【 「通知テスト」のQ&A 】にhatsunoさんがコメントしました。 「通知テスト」のQ&Aにhatsunoさんがコメントしました。

質問者が質問をwatchしていない

メンションの宛先 メンション メール
なし hatsunoさんから回答がありました。 質問「通知テスト」にhatsunoさんが回答しました。
質問者あて hatsunoさんからメンションがきました。 hatsunoさんからメンションがありました
他ユーザーあて hatsunoさんから回答がありました。 質問「通知テスト」にhatsunoさんが回答しました。

@Ichiyo14 Ichiyo14 self-assigned this Nov 7, 2021
@Ichiyo14 Ichiyo14 marked this pull request as ready for review November 7, 2021 12:29
@Ichiyo14 Ichiyo14 requested a review from kotakawase November 7, 2021 12:30
@Ichiyo14
Copy link
Contributor Author

Ichiyo14 commented Nov 7, 2021

@kawase-k
お疲れ様です。お時間がある時にレビューをお願いします🙏

@kotakawase
Copy link
Contributor

@Ichiyo14 さん
お疲れさまです。幾つか認識合わせさせてください🙏

  • Watchしているかどうかについては質問者が自分の質問をWatchしているかどうかで判断すればよいでしょうか?
  • Watchありなしの通知表についてまとめてくださりありがとうございます!
    今回の実装では関係ない部分になりそうですが、先ほど確認したところそれぞれの通知は下の文面できていると思ったのですがご確認していただけないでしょうか。

watchあり

メンションの宛先 メンション メール
なし hatsunoさんから回答がありました。 kimuraさんの【 「通知テスト」のQ&A 】にhatsunoさんがコメントしました。 「通知テスト」にhatsunoさんが回答しました。 「通知テスト」のQ&Aにhatsunoさんがコメントしました。
質問者あて hatsunoさんからメンションがきました。 hatsunoさんからメンションがありました
他ユーザーあて kimuraさんの【 「通知テスト」のQ&A 】にhatsunoさんがコメントしました。 「通知テスト」のQ&Aにhatsunoさんがコメントしました。

watchなし

こちらは問題なさそうでした!


お手数おかけしますがご確認のほどよろしくお願いいたします🙇‍♂️

@Ichiyo14
Copy link
Contributor Author

Ichiyo14 commented Nov 9, 2021

@kawase-k

Watchしているかどうかについては質問者が自分の質問をWatchしているかどうかで判断すればよいでしょうか?

そのとおりです。わかりにくくてすみません💦 上部のコメントを修正しました!


通知文の誤っていた箇所はご指摘どおりでした! これに関しても上記のコメントに修正を反映いたしました🙇‍♂️

@kotakawase
Copy link
Contributor

@Ichiyo14 さん
上記の件ご対応いただきありがとうございます🙏
動作について問題ないと思います👌

一応参考までに自分の視点でリファクタできるかも?と思ったのでご確認していただけると幸いです。
(修正はしなくて大丈夫です)

おつかれさまでした!🎉

return unless answer.sender != answer.receiver
return if mention_user_ids.include?(answer.receiver.id)
return if watcher_ids.include?(answer.receiver.id)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自分ならreturnを多用せずにinclude?の対になるexclude?を使って書いたかもしれません。

..
  def notify_answer(answer)
    question = answer.question
    watcher_ids = Watch.where(watchable_id: question.id).pluck(:user_id)
    mention_user_ids = answer.new_mention_users.ids

    return unless answer.sender != answer.receiver

    NotificationFacade.came_answer(answer) if watcher_ids.exclude?(answer.receiver.id) && mention_user_ids.exclude?(answer.receiver.id)
  end
..

Copy link
Member

@komagata komagata Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ichiyo14 kawase-kさんのおっしゃってる件についてはいかがでしょう?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kawase-k @komagata

return unless answer.sender != answer.receiver

との整合性のため、リファクタリングを行う必要が無いかもしれないと思っていたのですが、コミットの通り、先にanswer.sender == answer.receiverの判定を行うようにしました!
加えてexclude? を使うよう変更しました。

@kawase-k
お手数ですが再度ご確認をお願いします🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ichiyo14 さん
リファクタリングありがとうございます🙇‍♂️
concatを使われているの大変勉強になりました。いいと思います🎉

自分のリファクタリングの提案については、修正して欲しいのか参考に留めて欲しいのかきちんとお伝えするべきでした🙏

@Ichiyo14
Copy link
Contributor Author

@kawase-k
レビューありがとうございます!

@komagata
こちらのレビューをお願いします🙏

@Ichiyo14 Ichiyo14 force-pushed the bug/send-only-a-mention-when-sending-an-answer-with-a-mention-to-a-questioner-in-QA branch from f71e851 to e8f6449 Compare November 11, 2021 08:39
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認しました、OKですー🙆‍♂️

@komagata komagata merged commit 406c28f into main Nov 12, 2021
@komagata komagata deleted the bug/send-only-a-mention-when-sending-an-answer-with-a-mention-to-a-questioner-in-QA branch November 12, 2021 15:20
@github-actions github-actions bot mentioned this pull request Nov 12, 2021
22 tasks
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

Successfully merging this pull request may close these issues.

3 participants