-
Notifications
You must be signed in to change notification settings - Fork 71
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
お問い合わせがあったら管理者に通知する #7510
お問い合わせがあったら管理者に通知する #7510
Conversation
8716a26
to
3b45d6b
Compare
a5c1102
to
eb8a791
Compare
598cb59
to
a25728e
Compare
0dd8784
to
1c34557
Compare
29da23b
to
056f3c7
Compare
213b741
to
ff06db0
Compare
@@ -58,30 +59,6 @@ class Notification < ApplicationRecord | |||
after_update NotificationCallbacks.new | |||
after_destroy NotificationCallbacks.new | |||
|
|||
class << self | |||
def checked(check) |
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.
モデル定義のクラスメソッドは使っていないようでしたので削除しました。
現状は、ActivityNotifier
クラスのメソッドを使っています。
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.
既存コードに不要なものがないかしっかり確認されていて素晴らしいと思いました!✨
私が確認する限りでも使ってなさそうでした👀
ff06db0
to
994be36
Compare
@unikounio お疲れ様です🙇♂️お手数をおかけして申し訳ないですがレビューいただけないでしょうか?ご都合悪いようであれば、遠慮なくおっしゃっていただければと思います🙇♂️ |
@goruchanchan さん |
@unikounio ありがとうございます!急ぎではないですのでお手すきの時によろしくお願いします🙇♂️ |
994be36
to
c73f35f
Compare
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.
お待たせしております!
気になった点をいくつかコメントさせていただきました~
pjord
アカウント関連もコメントさせていただいたのですが、こちらも本Issueに含まれるという理解でよろしかったでしょうか?
教えていただけますと幸いです🙏
【コード以外の箇所へのコメント】
- PRの変更確認方法に、
http://localhost:3000/notifications
にアクセスする前にログインが必要である旨を書いていただくとより丁寧になるのかなと思いました - ScreenshotのGIFを画面分割で撮っているのが見やすかったです!✨
参考にさせていただきます😄
if result && @inquiry.save | ||
notify_inquiry |
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.
Newspaper Gemを使っていないのって何か理由があるのでしょうか?
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.
特に理由などありませんでした🙇♂️おっしゃる通り、コントローラにあるべき処理でないと思いましたので、Newpaper Gem を用いようと思います!
@@ -58,30 +59,6 @@ class Notification < ApplicationRecord | |||
after_update NotificationCallbacks.new | |||
after_destroy NotificationCallbacks.new | |||
|
|||
class << self | |||
def checked(check) |
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.
既存コードに不要なものがないかしっかり確認されていて素晴らしいと思いました!✨
私が確認する限りでも使ってなさそうでした👀
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.
pjord
にはアバター画像が設定されていないように見えました。
(私の環境の問題だったら申し訳ないです💦)
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.
main ブランチから rebase した際に pjord
のfixture が上書きされたことによる影響でした🙇♂️修正しました🙇♂️
@@ -963,3 +963,21 @@ marumarushain<%= i %>: # ページネーション確認のためのユーザー | |||
last_activity_at: "2014-01-01 00:00:0<%= 2 + (i - 1) / 2 %>" | |||
sent_student_followup_message: true | |||
<% end %> | |||
|
|||
pjord: | |||
login_name: pjord |
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.
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.
pjord
でのログインは試していなかったので助かりました🙇♂️既存の fixture 用の相談部屋が用意されていなかったことによるものだったので、pjord
の相談部屋 fixture を用意して対応しました
(こちらはテスト用の fixture でしたので、develop 環境用の fixture の方に対応しておきました🙇♂️)
@unikounio お疲れ様です!指摘事項再現できましたので対応いたします🙇♂️取り急ぎのご連絡まで🙇♂️ |
c73f35f
to
1eef40a
Compare
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.
@unikounio お疲れ様です!お手数をおかけして申し訳ないですが、指摘事項を修正しましたので、再度ご確認お願いいたします🙇♂️
pjordアカウント関連もコメントさせていただいたのですが、こちらも本Issueに含まれるという理解でよろしかったでしょうか?
含まれます🙇♂️ログイン確認などしていただきありがとうございました🙇♂️
PRの変更確認方法に、http://localhost:3000/notificationsにアクセスする前にログインが必要である旨を書いていただくとより丁寧になるの
ご指摘の通りと思いましたので修正いたしました🙇♂️
@@ -963,3 +963,21 @@ marumarushain<%= i %>: # ページネーション確認のためのユーザー | |||
last_activity_at: "2014-01-01 00:00:0<%= 2 + (i - 1) / 2 %>" | |||
sent_student_followup_message: true | |||
<% end %> | |||
|
|||
pjord: | |||
login_name: pjord |
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.
pjord
でのログインは試していなかったので助かりました🙇♂️既存の fixture 用の相談部屋が用意されていなかったことによるものだったので、pjord
の相談部屋 fixture を用意して対応しました
(こちらはテスト用の fixture でしたので、develop 環境用の fixture の方に対応しておきました🙇♂️)
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.
main ブランチから rebase した際に pjord
のfixture が上書きされたことによる影響でした🙇♂️修正しました🙇♂️
@@ -1369,7 +1369,7 @@ nagai-kyuukai: | |||
|
|||
pjord: | |||
login_name: pjord | |||
email: | |||
email: [email protected] |
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.
アイコン設定は attach_mentor_profile_image
にて実施しており、メンター条件が必要でした。
また、プロフィール表示にメールアドレスも必要だったので設定しました🙇♂️
if result && @inquiry.save | ||
notify_inquiry |
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.
特に理由などありませんでした🙇♂️おっしゃる通り、コントローラにあるべき処理でないと思いましたので、Newpaper Gem を用いようと思います!
@@ -285,7 +285,7 @@ discord_profile_nagai-kyuukai: | |||
account_name: | |||
times_url: | |||
|
|||
discord_profire_pjord: | |||
discord_profile_pjord: |
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.
誤字見つけたので修正しています🙇♂️discord_profire_pjord
で検索かけてもここ以外見つからなかったので、特に影響はないかなと思っています🙇♂️
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.
@goruchanchan さん
修正ありがとうございました!
コメントさせていただいた点について、すべて改善されていることが確認できました😄
修正の際に原因を示していただいているのでわかりやすく、こちらとしても勉強になりました🙏✨
他に気になる点もなさそうなので、私からはApproveとさせていただきます。
@unikounio @komagata |
c8d2b44
to
5e18955
Compare
app/models/inquiry_notifier.rb
Outdated
inquiry = payload[:inquiry] | ||
return if inquiry.nil? | ||
|
||
sender = User.find_by(login_name: 'pjord') |
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.
マジックナンバーなので定数か変数にするといいかもと思いました~。
書いた人本人にとっては自明でも「ぴ・・じょるど・・・って何だ?」ってなる人は多そうです。
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.
conflictの修正をお願い致します~。
8dddae7
to
0746790
Compare
@komagata ご確認ありがとうございます!コンフリクト対応しましたのでお手隙の時にご確認お願いいたします🙇♂️ |
0746790
to
c5ccb3d
Compare
@komagata お疲れ様です!こちらのご確認よろしくお願いします 🙇♂️ |
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.
conflictの修正をお願い致します~。
アイコン設定は attach_mentor_profile_image にて実施。メンター条件が必要。また、プロフィール表示にメールアドレスも必要だった
c5ccb3d
to
897ce23
Compare
@komagata お疲れ様です🙇♂️コンフリクト修正しましたので、お手数ですがご確認よろしくお願いします。 |
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.
確認させていただきました。OKです〜🙆♂️
@goruchanchan 本番で確認しましたー🙆🏻♂️ |
ご確認ありがとうございました🙇♂️ |
Issue
概要
問い合わせがあったら、右上のベル通知で管理者に通知がいくようにしました。
変更確認方法
feature/add-bell-notification-for-inquiry
をローカルに取り込むrails db:reset
で『ピヨルド』アカウントを取り込むforeman start -f Procfile.dev
でアプリを立ち上げるScreenshot
変更前
問い合わせをしても『通知』はされませんでした。
変更後
問い合わせをすると『通知』されるようになりました。