bootcamp
bootcamp copied to clipboard
fjordllc
企業一覧ページから各企業のアドバイザー・現役生のページへ飛べるようにした
Issue
- Issue番号 #7473
概要
変更前
企業様が自社のアドバイザー、現役生(研修生)を確認するには自社のページ → ユーザータブ → 現役生/アドバイザー の順に飛ばなければいけなかった。
変更後
企業一覧ページから各企業のアドバイザー・現役生のページへ飛べるようにした。
変更確認方法
-
feature/Add-link-to-target-users-for-company-on-companies-pageをローカルに取り込む -
foreman start -f Procfile.devでサーバーを起動する - komagata(どのユーザーでも構いません)でログインする
- /companiesにアクセスする。
- リンクをクリックしてみて、企業一覧ページから各企業のユーザータブに飛べることを確認する。
Screenshot
変更前
変更後
追加
企業のユーザーの管理者タブが正しく動くようにしました。合わせて企業一覧ページのリンクも対応させました。
@komagata 質問させていただきたいです🙏 2つあります🙇♂️
-
今回企業一覧ページに各企業のアドバイザー・現役生へのリンクを追加したのですが、 企業一覧ページと企業のshowページは共通のパーシャルを使っているため、企業のshowページにもアドバイザー・現役生へのリンクが追加されてしまっています。（かといって機能に問題があるわけではありません） これは問題なかったでしょうか？
-
ちょっと配置がおかしなことになってしまっているので直したいのですが、こういった場合は変更が少なければこちらで対応してしまって良いのでしょうか？（この場合
.mb-2~3をつけるとかでしょうか。） それともmachidaさんにお願いした方がいいでしょうか？
よろしくお願いします🙏
変更点
下記コードを追加しています。
# app/views/companies/_user_counts.html.slim
= link_to_company_users_path(company, 'adviser')
...
= link_to_company_users_path(company, 'student_and_trainee')
# app/helpers/companies_helper.rb
def link_to_company_users_path(company, target)
link_to t("target.#{target}"), company_users_path(target:, company_id: company.id), class: 'tab-nav__item-link'
end
メソッドの内部でlink_toを使ってaタグを作成しています。
現在の表示はこのようになっています
@mousu-a 追加コードについては差分（Files changed）でわかるので書かなくて大丈夫ですよ〜（基本的にFiles changedを提出側もレビュー側も必ず見るので）
https://github.com/fjordllc/bootcamp/pull/7479/files
@mousu-a
イメージと違ったのでデザインを修正しました。 カウントが 0 ではないときは、以下で示した部分がリンクになるようにしてあります。
ここで、追加でお願いがあります。
現在、管理者が0ではない企業のとき、管理者をクリックすると、その企業の管理者ユーザーの一覧に遷移したいです。
ところが、今は users?target=adminにアクセスすると、その企業の管理者ユーザーの一覧ではなく、現役生が表示されてしまいます。
他にも表示がおかしいところがあります。以下のキャプチャを参照してください。
管理者の数字をクリックしたら、
その企業に所属する管理者の一覧が表示されるようにしてください。
@unikounio
お疲れ様です！こちらのレビューをお願いできないでしょうか🙏 全く急ぎではないです！ お忙しければ遠慮なくおっしゃってください🙇♂️
@mousu-a さん お疲れ様です！ ３日以内にお返事できるよう進めさせていただきます～ ３日を超えそうあればその旨ご連絡させていただきますね。 よろしくお願いいたします🐈
@unikounio
お待たせいたしました🙇♂️ 返信になります！
併せてヘルパーメソッドを定義するとDRYに書けそうです。
こちらに関しては前述の通りです🙏
このelse節は0を表示することになると思うので、直接書いた方がDBアクセスがない分効率が良いのかなと思いました。
ご指摘いただいた箇所を日本語に直してみました！
users.unretired.advisersがあれば
users.unretired.advisers.sizeをリンクにする
なければ
users.unretired.advisers.sizeを表示する
users.unretired.advisersがあれば
users.unretired.advisers.sizeをリンクにする
なければ
0を表示する
前者はコンテキストが統一されていて「リンクにするかしないか」だけを判断すれば良さそうです。 後者はいきなり0が出てきたので「ん？なんで0？」と多少引っかかる感じがします（ニュアンスで申し訳ないです🙏）
「0を表示する」というのは、実装について多少なりとも把握していないと出てこない発想な気がします。🤔 なのでここで「0」がいきなり出てくると、前述したようにマジックナンバーとなってしまいそうです。👀 （理解するために多少コードを読まなければいけなくなりそう）
ですが反面、正直すぐに分かるようなことである気もしていますが...🤔 これらの自分の意見に関してunikounioさんの見解も伺えたらなと思っています🙇♂️
個人的には「最終的な判断はkomagataさんにお伺いする」という形でも良いのかな？と思っているのですがどうでしょうか👀
@mousu-a さん 丁寧にご説明いただきありがとうございます🙏✨
0がマジックナンバー化してしまう可能性についてのご指摘、なるほどです。 マジックナンバーに対しては定数やメソッドを用いて対策ができると思います。 しかし、これらの対策にもデメリットがあります。 私としては、後学のためにも、ヘルパーメソッドの件も含めて町田さんのご意見をお伺いした上で進められるとよいのかなと思っています。いかがでしょうか？ （もしお忙しいようであれば私から町田さんに連絡させていただきますのでおっしゃっていただけますと💡）
@unikounio
マジックナンバーに対しては定数やメソッドを用いて対策ができると思います。
なるほどです、こちらに関しては全く頭にありませんでした😅
私としては、後学のためにも、ヘルパーメソッドの件も含めて町田さんのご意見をお伺いした上で進められるとよいのかなと思っています。いかがでしょうか？ （もしお忙しいようであれば私から町田さんに連絡させていただきますのでおっしゃっていただけますと💡）
すみません、ご連絡の方お願い出来ますでしょうか😭🙏 ありがとうございます🙇♂️
@machida さん お疲れ様です！ 本PRで町田さんが書かれたコードについてお伺いさせてください🙏
私がレビューする中で、町田さんが変更された部分に次のようにコメントさせていただきました。 → https://github.com/fjordllc/bootcamp/pull/7479#discussion_r1527431475 このコメントにおける提案と、ヘルパーメソッドの活用に関する部分について、町田さんのご意見をお伺いさせていただけますと幸いです🙏
@machida ご確認ありがとうございますー！
@unikounio
ご連絡までありがとうございます！🙏 確認していただいてよかったですー！ 修正に入りますので今しばらくお待ちください🙇♂️
@unikounio
お待たせいたしましたー！🙏 ヘルパーメソッドを再導入しDRYにしました。 サイドレビューをお願いいたします🙇♂️
0がマジックナンバー化してしまう可能性についてのご指摘、なるほどです。 マジックナンバーに対しては定数やメソッドを用いて対策ができると思います。 しかし、これらの対策にもデメリットがあります。
こちらに関してはkomagataさんにお聞きしようと考えていますがどうでしょうか？
@komagata
メンバーの方からApproveいただけましたのでこちらレビューをお願いいたします🙏
一点質問があります🙏
# app/views/companies/_user_counts.html.slim
- if users.unretired.advisers.present?
= link_to_company_users_path(company, target: 'advisers')
- else
= users.unretired.advisers.size
こちらのコードですが、if users.unretired.advisers.present?がfalseの時は0なのが確定しているため、else節は0でいいのではないか？（わざわざusers.unretired.advisers.sizeとする必要はなさそう）ということについて議論しています。
komagataさんのご意見を伺いたいです🙏
こちらのやりとりを参照していただけますとわかりやすいと思います。 https://github.com/fjordllc/bootcamp/pull/7479#discussion_r1527431475 https://github.com/fjordllc/bootcamp/pull/7479#issuecomment-2002721906 https://github.com/fjordllc/bootcamp/pull/7479#discussion_r1528017146 https://github.com/fjordllc/bootcamp/pull/7479#issuecomment-2015235946
@komagata
すみません、このプルリクにmainブランチの他の人のコミットが混ざってしまっているため今後の対応をお伺いしたいです🙇♂️
現状の報告
- 現状どうなっているのか？
- mainブランチのコミット内容がプルリクに混ざってしまっている
- mainブランチとのコンフリクトが発生している
- ログに怪しい記述はないか？
- 見てみましたが怪しいログはなさそうでした。。下にログを記述します。
- 推測
- komagataさん, unikounioさんにレビューいただいた時にはこの状態にはなっていなかったと思います。
ログ
以下git reflogのログになります。
512a88eb7 (HEAD -> feature/Add-link-to-target-users-for-company-on-companies-page, origin/feature/Add-link-to-target-users-for-company-on-companies-page) HEAD@{0}: checkout: moving from feature/introduce-of-link-card to feature/Add-link-to-target-users-for-company-on-companies-page
bc84ed752 (feature/introduce-of-link-card) HEAD@{1}: checkout: moving from feature/Add-link-to-target-users-for-company-on-companies-page to feature/introduce-of-link-card
512a88eb7 (HEAD -> feature/Add-link-to-target-users-for-company-on-companies-page, origin/feature/Add-link-to-target-users-for-company-on-companies-page) HEAD@{2}: pull --rebase origin main (finish): returning to refs/heads/feature/Add-link-to-target-users-for-company-on-companies-page
512a88eb7 (HEAD -> feature/Add-link-to-target-users-for-company-on-companies-page, origin/feature/Add-link-to-target-users-for-company-on-companies-page) HEAD@{3}: pull --rebase origin main (pick): ローカル変数target_allowlistを導入、sendメソッドを実行する前にチェックを入れるようにした
d082e26bf HEAD@{4}: pull --rebase origin main (pick): if文が二重になっていたため削除
7d94d94eb HEAD@{5}: pull --rebase origin main (pick): ヘルパーメソッドを再導入し企業の各usersへのリンクの呼び出しをDRYに
4e28b6332 HEAD@{6}: pull --rebase origin main (pick): 企業の管理者ユーザー一覧を表示できるようにした。同じく企業一覧ページからも飛べるようにした。
4a136410f HEAD@{7}: pull --rebase origin main (pick): 企業ユーザーのカウント部分にリンクを設定。ユーザーroleの絞り込みに管理者を追加。
e9fbf733f HEAD@{8}: pull --rebase origin main (pick): 余計な空白を削除
35aa789e4 HEAD@{9}: pull --rebase origin main (pick): 企業一覧ページから各企業のアドバイザー・現役生のページへ飛べるようにした
79643285f HEAD@{10}: pull --rebase origin main (start): checkout 79643285fe6c4776488178d39eb022f9065525a1
85a590064 HEAD@{11}: commit: ローカル変数target_allowlistを導入、sendメソッドを
実行する前にチェックを入れるようにした
df820b6e1 HEAD@{12}: checkout: moving from feature/introduce-of-link-card to feature/Add-link-to-target-users-for-company-on-companies-page
bc84ed752 (feature/introduce-of-link-card) HEAD@{13}: reset: moving to HEAD
bc84ed752 (feature/introduce-of-link-card) HEAD@{14}: checkout: moving from feature/Add-link-to-target-users-for-company-on-companies-page to feature/introduce-of-link-card
df820b6e1 HEAD@{15}: pull --rebase origin main (finish): returning to refs/heads/feature/Add-link-to-target-users-for-company-on-companies-page
df820b6e1 HEAD@{16}: pull --rebase origin main (pick): if文が二重になっていたた
め削除
42b7b74d5 HEAD@{17}: pull --rebase origin main (pick): ヘルパーメソッドを再導入
し企業の各usersへのリンクの呼び出しをDRYに
2d14f4842 HEAD@{18}: pull --rebase origin main (pick): 企業の管理者ユーザー一覧
を表示できるようにした。同じく企業一覧ページからも飛べるようにした。
d1c0a5618 HEAD@{19}: pull --rebase origin main (pick): 企業ユーザーのカウント部
分にリンクを設定。ユーザーroleの絞り込みに管理者を追加。
13905a283 HEAD@{20}: pull --rebase origin main (pick): 余計な空白を削除
4a72ac083 HEAD@{21}: pull --rebase origin main (pick): 企業一覧ページから各企業
のアドバイザー・現役生のページへ飛べるようにした
258e32fb5 (main) HEAD@{22}: pull --rebase origin main (start): checkout 258e32fb51cb18056b0385fa68f27d7234749694
4c93c79a0 HEAD@{23}: checkout: moving from main to feature/Add-link-to-target-users-for-company-on-companies-page
258e32fb5 (main) HEAD@{24}: pull origin main: Fast-forward
2787cf4ad HEAD@{25}: checkout: moving from feature/introduce-of-link-card to main
よろしくお願いいたします🙇♂️