企業に所属しているアドバイザーのトップ画面に同じ企業の研修生リストを表示する #5438
Conversation
| .card-list-item | ||
| .card-list-item__inner | ||
| .card-list-item__user | ||
| = render 'users/icon', user: user, image_class: 'card-list-item__user-icon' | ||
| .card-list-item__rows | ||
| .card-list-item__row | ||
| header.card-list-item-title | ||
| h2.card-list-item-title__title(itemprop='name') | ||
| = link_to user, itemprop: 'url', class: 'card-list-item-title__link a-text-link' do | ||
| | #{user.login_name} （#{user.name}） | ||
| .card-list-item__row | ||
| .card-list-item-meta | ||
| .card-list-item-meta__items | ||
| - if user.reports.present? | ||
| .card-list-item-meta__item | ||
| = link_to user_reports_path(user), class: 'card-list-item-meta__item-link a-text-link' do | ||
| | 日報一覧（#{user.reports.count}） | ||
| - if user.products.present? | ||
| .card-list-item-meta__item | ||
| = link_to user_products_path(user), class: 'card-list-item-meta__item-link a-text-link' do | ||
| | 提出物一覧（#{user.products.count}） |
There was a problem hiding this comment.
app/views/home/_jobseeking_users.html.slimの8~31行目と同じ内容を書いてしまっていますが、
ひとまずデータを引っ張って来れればOKということでしたのでそのままにしております。
| .a-card | ||
| header.card-header.is-sm | ||
| h2.card-header__title | ||
| | 研修生 |
There was a problem hiding this comment.
テストを書くために仮でh2のコンテントを「研修生」にしてあります。
machidaさんに確認していただいた後で変更が必要な場合はテストと合わせて変更します。
|
@keiz1213 こちらのレビューをお願いいたします🙏 |
There was a problem hiding this comment.
@yuma-matsui
お疲れさまです！レビュー依頼ありがとうございます😄
動作確認できました！コードも問題ないと思います！
ただ今回書いていただいたテストではカバー率が高くてテストが重くなってしうのかなと思いました。
先日自分のissueでシステムテストについて質問したのですが次のようなコメントを頂きました。
特にsystem testはすごく遅いのでsystem testでは重要度の低いテストはやりたくない感じです。
もしどうしてもやるのであればviewだけのテストなどでできればいいな〜という感じです。
bootcampでは見た目上の変更は重要度が低いためテストはあまり追加しないほうがいいみたいです。
今回のissueは見た目上の変更であり、特別に個人情報などが含まれているわけでもないので重要度が低いように感じられました。
なので提案として、
- 企業に所属するアドバイザーのダッシュボードには研修生が表示されている
- 一般の生徒のダッシュボードには表示されない
これくらいで大丈夫かなと思いましたがどうでしょうか？
test/system/home_test.rb
Outdated
| @@ -351,4 +351,40 @@ class HomeTest < ApplicationSystemTestCase | |||
| assert_text I18n.l products(:product5).updated_at | |||
| end | |||
| end | |||
|
|
|||
| test 'show trainee lists for adviser belonging a company' do | |||
| assert users(:senpai).adviser? | |||
There was a problem hiding this comment.
bootcampのシステムテストを全体的に確認してみたのですが、bootcampではこのようなユーザーのロールを確認するアサーションは不要のようです。以下のテストについても同様です。
There was a problem hiding this comment.
ご指摘ありがとうございます。
他のテストを見よう見まねで書いてしましたが、おっしゃる通り不要そうですね。
上記ご指摘もあるのでテストケース自体を削除しようと思います。
|
@keiz1213
確かにテスト回していてかなり遅いなと思いました😅 今回はご提案の通り下記2点のテストのみ残して他は削除しようと思います。
また、モデルのテストを書くことをすっかり忘れていたので上記修正と合わせて追加します。 |
|
@keiz1213 |
test/system/home_test.rb
Outdated
| test 'not show trainee lists for adviser' do | ||
| visit_with_auth '/', 'advijirou' | ||
| assert_no_selector 'h2.card-header__title', text: '研修生' | ||
| end |
There was a problem hiding this comment.
今回はご提案の通り下記2点のテストのみ残して他は削除しようと思います。
- 企業に所属するアドバイザーのダッシュボードには研修生が表示されている
- 一般の生徒のダッシュボードには表示されない
アドバイザーだけど所属企業が無い場合についてはテストを残した方が良いと思ったのですがいかがでしょうか🤔
所属企業がある && アドバイザー 以外は全ユーザー非表示という仕様だとやはり不要そうな気もしますが....
ご意見をいただければと思います🙇🏻♂️
There was a problem hiding this comment.
アドバイザーだけど所属企業が無い場合についてはテストを残した方が良いと思ったのですがいかがでしょうか🤔
確かに少し不安定かな？と僕も思いました🤔ですがモデルのテストでbelongs_company_and_adviser?をしっかり追加されていて、所属企業有りのアドバイザーと所属企業無しのアドバイザー、が振り分けられていることをテストできているのでその点のシステムテストは不要かな？と思いました🤔
まだ僕もはっきり「どのテストが必要で、どのテストが不要か？」という明確な基準を理解していないくて自信がないので間違ってたらすみません😅
There was a problem hiding this comment.
ご確認いただきありがとうございます！！
モデルのテストでbelongs_company_and_adviser?をしっかり追加されていて、所属企業有りのアドバイザーと所属企業無しのアドバイザー、が振り分けられていることをテストできているのでその点のシステムテストは不要かな？と思いました🤔
こちらおっしゃる通りですね💡
僕もご指摘いただいてすごい納得できたのでadvijirouのテストケースは削除する方向でいこうと思います！
|
@keiz1213 |
|
@komagata |
app/models/user.rb
Outdated
| scope :same_company, lambda { |user| | ||
| where(company_id: user.company_id) | ||
| } | ||
| scope :my_subordinates, lambda { |user| |
There was a problem hiding this comment.
my_subordinatesだと私の部下的な意味になって意味合いが違っちゃうかもなのと、ちょっと直訳っぽいので、colleague_traineeとかfellow_traineeとかの方がいいかも？
There was a problem hiding this comment.
アドバイザーと研修生の関係に上限関係があると勘違いしていました。というより決めつけていました。
今回はcollegue_traineeでいこうと思います！
|
@komagata 一点確認ですが、修正コミットを作る際に |
|
はい、PRは必要なmainとの差分のみの状態でお願いします〜（PRを削除しなくても、修正で大丈夫です。） |
yuma-matsui
force-pushed
the
feature/add-training-subordinate-lists-at-adviser-page
branch
from
246a348
to
fe0a0db
Compare
|
@komagata OKの場合はmachidaさんにデザインの依頼をさせていただきます！ |
app/models/user.rb
Outdated
| scope :same_company, lambda { |user| | ||
| where(company_id: user.company_id) | ||
| } | ||
| scope :collegue_trainees, lambda { |user| |
There was a problem hiding this comment.
User.collegue_trainees(user) だったら、user.collegue_traineesという形にできるのではないかと思いました。（引数が不要になった方が自然なAPIなので）
yuma-matsui
force-pushed
the
feature/add-training-subordinate-lists-at-adviser-page
branch
from
7f3b09d
to
305d117
Compare
yuma-matsui
force-pushed
the
feature/add-training-subordinate-lists-at-adviser-page
branch
from
305d117
to
6912782
Compare
|
@komagata ご確認をお願いします🙏 |
app/models/user.rb
Outdated
| def belongs_company? | ||
| !company.nil? | ||
| end |
There was a problem hiding this comment.
こちら、いろんな書き方ができますが、メソッドを作るよりもそのまま書いた方が短いしわかりやすいかもです。（他のプログラマーが独自のメソッドを理解する必要があるため）
company_id?
とかでも一緒かなと思うので。
There was a problem hiding this comment.
?が使えるのはデータ型がboolean型のカラムのみと勘違いしていました。
StringであろうとIntegerであろうとnilまたはfalse以外の場合はtrueを返すんですね💡
There was a problem hiding this comment.
@yuma-matsui ちょっと話が違うかも？です。
company_id?というのはActiveRecordが自動で作成するメソッドです。
すべてのDBのカラム名?というメソッドを自動で作っています。そのカラムのデータがあるかどうかをbooleanで返すメソッドです。
そのメソッドが最初からあるので、上記のメソッドをわざわざ用意しなくてもいいかな〜と思ったという意味になります。
There was a problem hiding this comment.
ありがとうございます！
ActiveRecordが全てのカラムに対してカラム名?を用意してくれるのは知りませんでした。
Boolean型のものだけがカラム名?を用意してくれる対象なのだと思っていました...。
そのメソッドが最初からあるので、上記のメソッドをわざわざ用意しなくてもいいかな〜と思ったという意味になります。
こちらも理解ができました。
ありがとうございます。
|
@komagata |
test/models/user_test.rb
Outdated
| test '#collegue_trainees' do | ||
| target = users(:senpai).collegue_trainees | ||
| assert_includes(target, users(:kensyu)) | ||
|
|
|
@komagata |
komagata
deleted the
feature/add-training-subordinate-lists-at-adviser-page
branch
|
ご確認いただきありがとうございます！！ @machida |
Issue
Description
企業に所属しているアドバイザーのトップページに同じ企業の研修生のリストを表示。
確認方法
feature/add-training-subordinate-lists-at-adviser-pageブランチをローカルに取り込むrails sで立ち上げる変更点
変更前
変更後