メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする #4211

Saki-htr · 2022-02-12T10:58:47Z

issue #4110

変更前

これまで、ダッシュボードの学習時間の草の表示条件は以下のようになっていました。

学習時間が有るユーザーなら全員表示する
学習時間が無ければ、メンターでない場合表示する(メンター以外は学習時間の有無に関わらず表示する)

具体的には、以下の画面になっていました。

メンター(mentormentaro):学習時間があるメンターは表示 / ないメンターは非表示
(以下の画像はありの場合のものです)

アドバイザー(advijirou):学習時間の有無に関わらず表示

管理者(adminonly):学習時間の有無に関わらず表示

変更後

以下に変更いたしました。

メンター・アドバイザー・管理者の場合、(学習時間の有る無しに関わらず)非表示
それ以外のユーザーは(学習時間の有る無しに関わらず)表示

具体的には、以下の画面に変更いたしました。

メンター(mentormentaro)

アドバイザー(advijirou)

管理者(adminonly)

ご確認いただきたいこと3点

mentormentaroでログインした時、学習時間が有る時も無い時も、ダッシュボードに学習時間の草が無い
advijirouでログインした時、学習時間が有る時も無い時も、ダッシュボードに学習時間の草が無い
adminonlyでログインした時、学習時間が有る時も無い時も、ダッシュボードに学習時間の草が無い

Saki-htr · 2022-02-13T07:15:29Z

@tksmasaki さん

お疲れ様です〜
お手すきの際に、こちらのissueのレビューをしていただけましたら幸いです🙏

テストが落ちているのですが、今回のPRで追記したものは通っており、落ちているのは日報のコメントの投稿日時をクリックするとコメントURLがコピーされる機能のテストが落ちる · Issue #4201 · fjordllc/bootcampが原因と思われます。
machidaさんが、依頼時にこの旨を記載すればレビュー依頼してよいと仰っていましたので、こちらの状態でレビュー依頼させていただきます🙇‍♀️
よろしくお願いいたします。

tksmasaki

@Saki-htr
お疲れさまです。レビュー依頼ありがとうございます！
いくつか気になった点をコメントさせていただきました。分かりづらい点があれば、ご指摘よろしくお願いします🙇‍♂️

tksmasaki · 2022-02-16T09:01:12Z

app/views/home/index.html.slim

@@ -62,7 +62,7 @@ header.page-header
                    | 内容を更新
          - if (current_user.admin? || current_user.adviser?) && @job_seeking_users.present?
            = render 'job_seeking_users', users: @job_seeking_users
-          - if current_user.total_learning_time.positive? || !current_user.mentor?
+          - unless current_user.mentor? || current_user.adviser? || current_user.admin?


この条件を「 student もしくは trainee のみ草を表示する」と読み替えて問題なければ、
if current_user.student_or_trainee?
の方が簡潔かなと思いました。（ニコニコカレンダーの表示条件はそうなっているようです）

参考：bootcampのuserの roleまとめ | FJORD BOOT CAMP（フィヨルドブートキャンプ）

@tksmasaki さん
たしかにその方が簡潔で読みやすいコードになりますね✨ ありがとうございます!
念の為rails consoleで、想定通りにtrue / falseが返ってくるかを確認したところ、大丈夫でした。

# 管理者 User.find_by(login_name: "machida").student_or_trainee? #=>false User.find_by(login_name: "komagata").student_or_trainee? #=>false # アドバイザー User.find_by(login_name: "advijirou").student_or_trainee? #=> false User.find_by(login_name: "senpai").student_or_trainee? #=> false #メンター User.find_by(login_name: "yamada").student_or_trainee? #=> false #生徒 User.find_by(login_name: "kensyu").student_or_trainee? #=>true User.find_by(login_name: "sotugyou").student_or_trainee? #=>true User.find_by(login_name: "daimyo-kensyu").student_or_trainee? #=>true

また動作確認も行ったところOKでしたので、仰るとおりif current_user.student_or_trainee?に変更いたしました🙆‍♀️
こちらのコミットで修正しました：823567d(草の表示条件を簡潔なものに修正)
条件を忠実に満たすために「管理者・アドバイザー・メンター以外」をそのままコードにすることばかり考えていましたが、読みやすく切り出されたメソッドがないかをモデルで確認することが大事なのですね〜
勉強になりました🙏✨

tksmasaki · 2022-02-16T09:10:08Z

test/system/home_test.rb

+
+  test 'not show the grass for administrators' do
+    visit_with_auth '/', 'komagata'
+    assert_no_text 'ニコニコカレンダー'


assert_no_text 'ニコニコカレンダー' → これは '学習時間' との間違えでしょうか？

また、このテストケースは show the grass except for mentors, advisers, and administrators のテストケースと重複している気がしました。別途、管理者の場合のテストを作った目的が何かありますか？

申し訳ありません🙇‍♀️
既存のニコニコカレンダーのテストをコピーして今回のテストを書いたのですが、「ニコニコカレンダー」を「学習時間」に変更するのを失念しておりました。こちらは削除いたしました🙏

tksmasaki · 2022-02-16T10:18:53Z

test/system/home_test.rb

@@ -89,9 +89,26 @@ class HomeTest < ApplicationSystemTestCase
    assert_current_path(/niconico_calendar=#{1.month.ago.strftime('%Y-%m')}/)
  end

-  test 'show the grass' do
+  test 'show the grass except for mentors, advisers, and administrators' do


個人的な好みかもしれないですが、草が表示される場合と表示されない場合にテストを分けたほうが、わかりやすいと思いました。
また、そのユーザーに対して、なぜ草が表示 or 非表示なのかがテストだけ見ても伝わりにくいので、草が表示されていることを確認するアサーションの前に、そのユーザーのロールを assert したほうが良いのではないかと思いました。（コメントでも十分かもしれないです）

テストのイメージとしては以下のような感じです。

show the grass （名前は適当です）

草が表示されるユーザーで該当ページへ

そのユーザーのロールをアサーション（もしくはコメント）

表示されることを確認

not show the grass

草が表示されないユーザーで該当ページへ

そのユーザーのロールをアサーション（もしくはコメント）

表示されないことを確認

草が表示される場合と表示されない場合にテストを分けたほうが、わかりやすい

たしかにその方が分かりやすいですね〜😄 仰るとおり表示される場合と表示されない場合に分けました!

なぜ草が表示 or 非表示なのかがテストだけ見ても伝わりにくいので、草が表示されていることを確認するアサーションの前に、そのユーザーのロールを assert したほうが良いのではないか

たしかに補足がないと「なぜか」が分かりにくいですね〜
tksmasakiさんの日報が0件の場合、日報ダウンロードボタンを非表示に修正 by tksmasaki · Pull Request #4128を参考にさせていただき、コメントで補足を入れてみました! いかがでしょうか〜☺️

ユーザーのロールを assert

すみません、「ユーザーのロールを assert 」とは、具体的にはどのようなコードか分かりませんでした🙏💦
ダッシュボードの左上に表示されるロールをassert_text 'メンター'のように書くということでしょうか??

すみませんが、教えていただけるとありがたいです〜🙇‍♀️

修正ありがとうございます！

ユーザーのロールを assert

これは、assert users(:kimura).student? のように書いて、ログインするユーザーのロールを確認するという意図でした。初めからはっきり伝えられず、申し訳ありません。

こうすることでログインするユーザーのロールが明確になってよいと思ったのですが、コメントでもいいかな？それとも、そもそもこれはいらないかな？と、レビュー時に迷った点でもありました。

はっきりとこうした方がよいと提示できず申し訳ありませんが、テスト内容が明確になればよいと思うので、@Saki-htr さんがロールをアサーションする方法のほうが良いと思われたら、そちらに修正していただければと思います🙇‍♂️
このままでもよい思いましたが、確認よろしくお願いいたします🙏

教えてくださってありがとうございます!
アサーションとはそういう意味だったのですね〜
勉強になります✨

ロールをアサーションした方が、読んだ方に意図が伝わることに加えて、テストも行えて一石二鳥と考えましたので、ロールをアサーションする方法に変更いたしました🙏

Saki-htr · 2022-02-16T12:44:24Z

@tksmasaki さん
体調が悪い中とても丁寧にレビューしてくださって、ありがとうございます🙏
確認いたします!

Saki-htr · 2022-03-01T11:52:41Z

@tksmasaki さん
レビューしていただいてからとても日が経ってしまい、大変申し訳ありません🙇‍♀️
コメントしていただいた点を修正しましたので、ご確認をお願いいたします〜🙏

tksmasaki

再度コメントしたので、確認よろしくお願いいたします🙏

tksmasaki · 2022-03-04T23:37:06Z

test/system/home_test.rb

+  test 'not show the grass for mentor, adviser, and admin' do
+    # メンターは学習時間の草を表示しない
+    visit_with_auth '/', 'mentormentaro'
+    assert_no_text '学習時間'


表示されないこと以外に、表示されるはずのものなども確認するとよさそうです。
例：ダッシュボードが表示されていることを確認する
assert_selector 'h2.page-header__title', text: 'ダッシュボード'

（前回のコメントから時間が空いての追加コメントになってしまい、申し訳ありません🙇‍♂️ 後になってこのことに気づきました...）

参考：システムスペックやフィーチャスペックで「〜が表示されないこと」だけを検証するのはちょっと危険、という話 - Qiita

最近別のissueで、"無い"ことを確認するテストを書く難しさを感じたので勉強になります〜
気づいてくださってありがとうございます🙇‍♀️✨

assert_selector 'h2.page-header__title', text: 'ダッシュボード'に変更させていただきました🙏
assert_no_text '学習時間'をチェックする前にダッシュボードのページにアクセスできているか確認すべきと思ったので、`assert_no_selector 'h2.card-header__title', text: '学習時間'の前に追加しました。

tksmasaki · 2022-03-05T00:58:12Z

test/system/home_test.rb

+  test 'show the grass for student and trainee' do
+    # 生徒は学習時間の草を表示する
+    visit_with_auth '/', 'kimura'
+    assert_text '学習時間'


「学習時間」という文字列は、他の箇所にも表示される可能性（お知らせやユーザーの日報の下書きに、「学習時間」が含まれるものが作成されるなど）があるかもしれないので、「学習時間」がカードのタイトルとして使われていることを確認すると良いと思いました。

思いついた修正例

visit_with_auth '/', 'kimura' assert_selector 'h2.card-header__title', text: '学習時間'

（前回のレビューで言えず申し訳ありません🙇‍♂️）

（前回のレビューで言えず申し訳ありません🙇‍♂️）

とんでもないです!ありがとうございます〜🙏✨

「学習時間」という文字列は、他の箇所にも表示される可能性（お知らせやユーザーの日報の下書きに、「学習時間」が含まれるものが作成されるなど）があるかもしれない

たしかに、セレクタを指定すると確実に「学習時間」を指定できますね。
仰るとおりassert_selector 'h2.card-header__title', text: '学習時間'に変更いたしました。

学習時間の草が「無い」テストにも使った方がより確実だと思いましたので、そちらもassert_no_selector 'h2.card-header__title', text: '学習時間'に変更いたしました。

Saki-htr · 2022-03-07T13:28:37Z

@tksmasaki さん
お疲れさまです〜
ご指摘いただいた点を修正しましたので、お手すきの際にご確認いただけますと幸いです🙏

tksmasaki

@Saki-htr
確認しました！ LGTMです🙆‍♂️

#4211 (comment)
説明内のメンターのユーザー例が今は存在しない yamada のままなので、ここは直しておくといいと思います！

Saki-htr · 2022-03-07T22:55:11Z

@tksmasaki
丁寧にレビューしてくださり、ありがとうございました🙇‍♀️

説明内のメンターのユーザー例が今は存在しない yamada のままなので、ここは直しておくといいと思います！

こちら、修正いたしました!

@komagata
お疲れさまです。
チームメンバーのレビューを通りましたので、お手すきの際にレビューしていただけますと幸いです。
よろしくお願いいたします🙏

komagata · 2022-03-09T12:57:30Z

test/system/home_test.rb

+  test 'show the grass for student and trainee' do
+    assert users(:kimura).student?
+    visit_with_auth '/', 'kimura'
+    assert_selector 'h2.card-header__title', text: '学習時間'
+    logout
+
+    assert users(:kensyu).trainee?
+    visit_with_auth '/', 'kensyu'
+    assert_selector 'h2.card-header__title', text: '学習時間'
+  end


検証するユーザーごとにテスト分けた方がわかりやすいかな〜と思いました。
処理速度としては一回ログアウトしてるのであまり変わらないと思うので。

@komagata
レビューしてくださり、ありがとうございます☺️
仰るとおり、ユーザーごとにテストを分けたほうが読みやすいと思いましたので、ユーザーごとに分けました。
ご確認をお願いいたします〜🙏

komagata

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

komagata · 2022-03-14T14:45:35Z

test/system/home_test.rb

+  test 'show the grass for student' do
+    assert users(:kimura).student?
+    visit_with_auth '/', 'kimura'
+    assert_selector 'h2.card-header__title', text: '学習時間'
+  end
+
+  test 'show the grass for trainee' do
+    assert users(:kensyu).trainee?
+    visit_with_auth '/', 'kensyu'
+    assert_selector 'h2.card-header__title', text: '学習時間'
+  end
+
+  test 'not show the grass for mentor' do
+    assert users(:mentormentaro).mentor?
+    visit_with_auth '/', 'mentormentaro'
+    assert_selector 'h2.page-header__title', text: 'ダッシュボード'
+    assert_no_selector 'h2.card-header__title', text: '学習時間'
+  end
+
+  test 'not show the grass for adviser' do
+    assert users(:advijirou).adviser?
+    visit_with_auth '/', 'advijirou'
+    assert_selector 'h2.page-header__title', text: 'ダッシュボード'
+    assert_no_selector 'h2.card-header__title', text: '学習時間'
+  end
+
+  test 'not show the grass for admin' do
+    assert users(:komagata).admin?
+    visit_with_auth '/', 'komagata'
+    assert_selector 'h2.page-header__title', text: 'ダッシュボード'
+    assert_no_selector 'h2.card-header__title', text: '学習時間'


とってもわかりやすくなりました〜👍

Saki-htr marked this pull request as ready for review February 13, 2022 06:59

Saki-htr self-assigned this Feb 13, 2022

Saki-htr requested a review from tksmasaki February 13, 2022 07:09

tksmasaki reviewed Feb 16, 2022

View reviewed changes

Saki-htr force-pushed the feature/not-display-grass-when-mentors-advisors-and-admins-login branch from 60175bd to 297b328 Compare March 1, 2022 10:08

tksmasaki requested review from tksmasaki March 1, 2022 13:54

tksmasaki reviewed Mar 5, 2022

View reviewed changes

Saki-htr force-pushed the feature/not-display-grass-when-mentors-advisors-and-admins-login branch from 0e765d9 to e73ed35 Compare March 7, 2022 12:25

tksmasaki approved these changes Mar 7, 2022

View reviewed changes

Saki-htr requested a review from komagata March 7, 2022 22:54

komagata reviewed Mar 9, 2022

View reviewed changes

Saki-htr added 12 commits March 13, 2022 21:23

メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする

851ce2d

メンター・アドバイザ・管理者のダッシュボードに学習時間の草を表示しないテストを追記

85a1af2

草の表示条件を簡潔なものに修正

94c514b

誤って作成した重複しているテストを削除

4acb219

テストを学習時間の草を表示する場合としない場合に分け、「そのユーザーに対してなぜ草が表示or非表示なのか」をコメントで追記

5566847

yamadaは存在しないため、mentormentaroに変更

fa4f683

コメントの並びを修正

e0d7fdb

コメントを削除し、ユーザーのロールをアサーションする処理を追加

19acc71

assert_selectorを使って、ダッシュボードのページにアクセスできているかを確認する処理を追加

358c872

学習時間が表示されているかのテストを、より確実にするためにassert_selectorに変更した

b57726c

学習時間が表示されていないかのテストを、assert_no_selectorを使ってより確実にした

d098ed1

テストを検証するユーザーごとに分けた

b2bdbf3

Saki-htr force-pushed the feature/not-display-grass-when-mentors-advisors-and-admins-login branch from 303731f to b2bdbf3 Compare March 13, 2022 12:23

komagata approved these changes Mar 14, 2022

View reviewed changes

komagata merged commit b6a932f into main Mar 14, 2022

komagata deleted the feature/not-display-grass-when-mentors-advisors-and-admins-login branch March 14, 2022 14:45

github-actions bot mentioned this pull request Mar 14, 2022

Release 2022-03-18 01:41:12 #4380

Merged

32 tasks

Saki-htr added the 1 label Jul 5, 2022

Saki-htr removed the 1 label Aug 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする #4211

メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする #4211

Saki-htr commented Feb 12, 2022 •

edited

Loading

Saki-htr commented Feb 13, 2022

tksmasaki left a comment •

edited

Loading

tksmasaki Feb 16, 2022

Saki-htr Feb 28, 2022 •

edited

Loading

tksmasaki Feb 16, 2022

Saki-htr Mar 1, 2022

tksmasaki Feb 16, 2022

Saki-htr Mar 1, 2022

tksmasaki Mar 1, 2022 •

edited

Loading

Saki-htr Mar 7, 2022

Saki-htr commented Feb 16, 2022 •

edited

Loading

Saki-htr commented Mar 1, 2022

tksmasaki left a comment

tksmasaki Mar 4, 2022

Saki-htr Mar 7, 2022 •

edited

Loading

tksmasaki Mar 5, 2022

Saki-htr Mar 7, 2022

Saki-htr commented Mar 7, 2022

tksmasaki left a comment •

edited

Loading

Saki-htr commented Mar 7, 2022

komagata Mar 9, 2022

Saki-htr Mar 13, 2022

komagata left a comment

komagata Mar 14, 2022

メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする #4211

メンター・アドバイザー・管理者はダッシュボードの学習時間の草を非表示にする #4211

Conversation

Saki-htr commented Feb 12, 2022 • edited Loading

変更前

変更後

ご確認いただきたいこと3点

Saki-htr commented Feb 13, 2022

tksmasaki left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saki-htr Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tksmasaki Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saki-htr commented Feb 16, 2022 • edited Loading

Saki-htr commented Mar 1, 2022

tksmasaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saki-htr Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saki-htr commented Mar 7, 2022

tksmasaki left a comment • edited Loading

Choose a reason for hiding this comment

Saki-htr commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saki-htr commented Feb 12, 2022 •

edited

Loading

tksmasaki left a comment •

edited

Loading

Saki-htr Feb 28, 2022 •

edited

Loading

tksmasaki Mar 1, 2022 •

edited

Loading

Saki-htr commented Feb 16, 2022 •

edited

Loading

Saki-htr Mar 7, 2022 •

edited

Loading

tksmasaki left a comment •

edited

Loading