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

管理者でログインしたとき、相談部屋個別ページで相談部屋の主の直近の日報が表示される。 #4389

Merged
merged 6 commits into from
Mar 16, 2022

Conversation

chung3011
Copy link
Contributor

@chung3011 chung3011 commented Mar 12, 2022

#4208

要件

管理者でログインしたとき、相談部屋個別ページで相談部屋の主の直近の日報が表示される。

変更前

Screen Shot 2022-03-12 at 23 00 26

変更後

Screen Shot 2022-03-13 at 23 59 25

確認手順

  • feature/show-daily-report-in-talk-page-for-adminブランチを手元に持ってくる。
  • 管理者でログインして、相談部屋個別ページを確認する。

@chung3011
Copy link
Contributor Author

chung3011 commented Mar 12, 2022

@garammasala29 さん、このPRをチェックしていただけませんか?

@chung3011 chung3011 requested a review from garammasala29 March 12, 2022 16:21
Copy link
Contributor

@garammasala29 garammasala29 left a comment

Choose a reason for hiding this comment

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

@chung3011 さん
お疲れ様です。レビュー依頼ありがとうございます😄
日報一覧はVue化されていて、そちらを使えばよさそうです(ユーザー日報ページのサイドバーを参照していただければ)
コントローラーやモデルのコードを追加しなくても日報が表示されると思いました👍

@@ -610,6 +610,10 @@ def mark_all_as_read_and_delete_cache_of_unreads(target_notifications: nil)
Cache.delete_mentioned_and_unread_notification_count(id)
end

def latest_reports
reports.order(reported_on: :DESC).first(3)
Copy link
Contributor

Choose a reason for hiding this comment

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

latest_reportsという名前だと最新の日報1つとなるような気がしました。
また、issueの画像にて「最新の日報を10件表示する」と書かれていますが、いかがですか?

Copy link
Contributor Author

@chung3011 chung3011 Mar 14, 2022

Choose a reason for hiding this comment

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

@garammasala29 さん、ご確認ありがとうございます。
メソッドの名前と日報数を更新しました。
もう一度チェックしていただけませんか?

Copy link
Contributor

Choose a reason for hiding this comment

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

@chung3011 さん
確認いただきありがとうございました🙏
せっかく修正していただいたんですが、大枠のコメントにも書きましたようにVue化されている日報一覧を再利用する形にすると、コントローラーとモデル部分のコードは全て必要なくなるかと思います!
少し紛らわしかったですね💦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garammasala29 さん、コードを更新しました。もう一度確認お願い致します。

.side-tabs-contents
.side-tabs-contents__item#side-tabs-content-1
.user-info
= render 'users/user_secret_attributes', user: @user
= render 'users/metas', user: @user
#js-user-mentor-memo(data-user-id="#{@user.id}" data-products-mode="#{true}")
.side-tabs-contents__item#side-tabs-content-2
Copy link
Contributor

Choose a reason for hiding this comment

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

デザイン部分はよいと思います😄

Copy link
Contributor

@garammasala29 garammasala29 left a comment

Choose a reason for hiding this comment

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

@chung3011 さん
修正いただきありがとうございました!意図が伝わり安心しました😄
1点だけコメントしましたので、確認よろしくお願いします🙏

i.far.fa-sad-tear
.o-empty-message__text
| 日報はまだありません。
#js-reports(data-user-id="#{@talk.user.id}" data-limit="10")
Copy link
Contributor

Choose a reason for hiding this comment

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

@talk.user.idととってくる情報は変わりませんが、コントローラーを見ると@user.idでもidは設定できるようです。
上のjs-user-mentor-memoでも@user.idが使われているので合わせた方が無難なのかな?と思いましたがどうでしょうか🤔?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garammasala29 さん、コードを更新しました。もう一度確認お願い致します。

Copy link
Contributor

@garammasala29 garammasala29 left a comment

Choose a reason for hiding this comment

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

@chung3011 さん
修正ありがとうございました🙏LGTMです!

@chung3011
Copy link
Contributor Author

@komagataさん、レビューをしていただきたいです。よろしくお願いいたします🙏

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.

最近の日報が表示されていることを確認するテストがあるといいかもです〜

@chung3011
Copy link
Contributor Author

@komagataさん、テストを追加しました。もう一度確認お願い致します🙏

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.

"edit code"や"add test"というコミットメッセージがありますが、後から見た人がわかりやすいように具体的に書いてみて下さい〜。

@chung3011
Copy link
Contributor Author

@komagata さん
はい、わかりました。今から注意します。

@chung3011
Copy link
Contributor Author

@komagata さんから「Changes requested」を受け取りました。 今何を修正するか教えていただけますか? 「コミットメッセージを書く」について、次回注意します。

@garammasala29
Copy link
Contributor

@chung3011 さん
過去のコミットメッセージをわかりやすいものに変更するといいと思いますよ👍

@chung3011 chung3011 force-pushed the feature/show-daily-report-in-talk-page-for-admin branch from feb0633 to c5c2d59 Compare March 16, 2022 05:20
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 3d03ba3 into main Mar 16, 2022
@komagata komagata deleted the feature/show-daily-report-in-talk-page-for-admin branch March 16, 2022 14:02
@github-actions github-actions bot mentioned this pull request Mar 16, 2022
32 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