入って1週間のユーザーにはメンターにしか表示されないマークを表示する #8349

harada-webdev · 2025-02-18T19:08:18Z

Issue

入って1週間のユーザーにはメンターにしか表示されないマークを表示する。 #8269

概要

ログインしているユーザーがメンターの場合に、入会して1週間以内の現役生と研修生のアイコンのclassにis-new-userを追加しました。ユーザーアイコンの周りを装飾するclassは、view側でis-#{user.primary_role}と定義されているので、primary_roleからnew-userを戻り値にして、is-new-userを生成しています。

is-#{user.primary_role}のclassを使ってユーザーアイコンの縁を色で囲むことができます。例えば、現在の実装では、is-graduateの場合には緑色で囲まれていたり、is-traineeの場合は黄色で囲まれています。そのため、is-new-userの場合にも、同様に何かしらの色で囲むことができます。

また、プロフィールページでは、is-graduateのclassを持つユーザーは卒業生、is-traineeを持つユーザーは研修生と表示されるので、is-new-userのクラスを持つユーザーは、プロフィールページに「新入生」と表示させるようにしました。

変更確認方法

feature/add-mark-to-new-userをローカルに取り込む
i. git fetch origin feature/add-mark-to-new-user
ii. git checkout feature/add-mark-to-new-user
foreman start -f Procfile.devで開発環境を立ち上げる
ユーザー名komagata、パスワードtesttestでログインする
入会して1週間以内のユーザーのプロフィールページに移動する
ユーザーアイコンの横に「新入生」が表示されていることを確認する

Screenshot

変更前

変更後(デザインを入れる前)

harada-webdev · 2025-02-18T19:55:22Z

@machida
お疲れ様です
ユーザーのアイコンにマークを付けるためのclassであるis-new-userを実装しましたので、デザイン付けをお願いいたします🙏

JunichiIto · 2025-02-24T02:40:16Z

@machida @harada-webdev
ちょっとすいません、今 @unikounio さんのコードレビュー模擬試験の練習をしているのですが、こちらのPRがコードレビューの練習の題材として良さそうなので、このPRのレビューを @unikounio さんにやってもらってもいいでしょうか？
よろしくお願いしますー。

harada-webdev · 2025-02-24T15:29:03Z

@JunichiIto
デザイン付けがまだ行われていないのですが、自分は問題ないです！

unikounio · 2025-02-25T12:30:22Z

@harada-webdev さん
ありがとうございます！
明日には完了させる予定ですので、今しばらくお待ちください🙏

@JunichiIto さん
ご調整ありがとうございます🙏

unikounio

@harada-webdev さん
何点かコメントさせていただきました～
お手すきの際にご確認をお願いします🙏

unikounio · 2025-02-26T02:20:08Z

app/components/products/product_component.rb

@@ -11,8 +11,8 @@ def initialize(product:, is_mentor:, is_admin:, current_user_id:, reply_deadline
    @display_user_icon = display_user_icon
  end

-  def role_class
-    "is-#{@product.user.primary_role}"
+  def role_class(user: helpers.current_user)


普通の引数でよいかもしれません。
呼び出し側をみる限り、キーワード引数じゃなくてもuserを指定していることは伝わりそうに見えました👀
同様の箇所がいくつかありそうなので、もし修正される場合はそちらも合わせて対応していただけると良さそうです。
【参考】Docs: なんでもかんでもキーワード引数にしない | FBC

デフォルトで引数を指定する際、キーワード引数を使わないといけないと勘違いしていました😭
下記の例のようにして、関連する箇所すべてキーワード引数を使わないように修正しました。

def role_class(user= helpers.current_user)

commit: キーワード引数ではなくデフォルト引数を使うようにした

unikounio · 2025-02-26T02:25:16Z

app/decorators/user_decorator/role.rb

@@ -15,12 +15,13 @@ def roles
      roles = role_list.find_all { |v| v[:value] }
                       .map { |v| v[:role] }
      roles << :student if roles.empty?
+      roles.unshift('new-user') if user&.mentor? && (roles & %i[student trainee]).any? && elapsed_days <= 7


呼び出し側を確認すると、引数として渡されるuserには「ログインしているユーザー」と「アイコンを表示するユーザー」の２パターンがあるように見えました👬
後者の場合、「アイコンを表示するユーザー」がメンターかどうかを確認することになりそうです。
プロフィールページの「新入生」は正しく動いていそうでしたが、アイコンの装飾をしたときにharadaさんの意図しない動きをするかもしれないなと思いました。

条件式のネストが少し深いので、変数に切り出すと読みやすくなりそうです。

7がマジックナンバーになっているので、定数化すると意図が明確になるかもです。

呼び出し側を確認すると、引数として渡されるuserには「ログインしているユーザー」と「アイコンを表示するユーザー」の２パターンがあるように見えました👬

確かに2パターンありました！ユーザーアイコン(新入生のアイコン)を表示するために、roleやprimary_roleを使う場合には、それらに渡す引数をcurrent_userのみに限定するようにしました。

また、テストコードで上記のメソッドを使う場合は、新入生アイコンの表示とテストの目的が関係ないので、引数をレシーバとするように設定しています。(テスト側でcurrent_userが定義されていないことがあり、その際にrolesなどのデフォルト引数のcurrent_userを参照するとエラーが起きるので、それを回避する目的もあります)

commit: アイコンを表示する際に、current_userを引数とした

条件式のネストが少し深いので、変数に切り出すと読みやすくなりそうです。

new_student_or_trainee = (roles & %i[student trainee]).any? && elapsed_days <= FIRSTWEEK roles.unshift('new-user') if new_student_or_trainee && user&.mentor?

上記のように、新入生かどうかの真偽値を返す箇所を変数に切り出しました

キーワード引数ではなくデフォルト引数を使うようにした

7がマジックナンバーになっているので、定数化すると意図が明確になるかもです。

7という数字は最初の1週間という意味なので、7を格納した定数FIRSTWEEKを使用することにしました

commit: 入って最初の1週間を表す7を定数に格納した

unikounio · 2025-02-26T08:59:32Z

test/system/generations_test.rb

Descriptionで

ログインしているユーザーがメンターの場合に、入会して1週間以内の現役生と研修生のアイコンのclassにis-new-userを追加しました。

とご説明いただいているのに対して、テストケースが少し少なめに見えました👀

ログインしているユーザーが

メンターの場合に

入会して１週間以内の

現役生と研修生

というように結構限定的な条件になっているので、もう少しテストを追加するとより安心できるかもです😌

@unikounio
こちら、時間の都合上、入会して1週間以内の現役生の場合のテストのみしか作れませんでした。
入会して1週間以内の研修生の場合のテストは後程作りますので、まずは、そのテストのみの確認をお願いいたします。申し訳ございません🙏

commit: 戻り値がnew-userになるかのテストを追加した

unikounio

Commentのみの意図だったのですが誤ってRequest changesにしてしまいました！
申し訳ないです🙏

harada-webdev · 2025-02-26T14:20:26Z

@unikounio
レビューありがとうございます！
明日(27日)までに修正しますのでよろしくお願いいたします🙏

harada-webdev · 2025-02-27T14:52:44Z

@unikounio
レビューありがとうございます！
修正を行いましたので、ご確認のほどよろしくお願いいたします🙏

unikounio

ご対応・ご説明ありがとうございます🙏

いくつかコメントさせていただきました。
どれも「こうするとさらに良くなるかも」という内容ですので、ご参考までにご確認いただければと思います😊
機能としては大きな問題はないと思いましたので、Approveさせていただきます！

ただ、テストの事情を本番の実装で対応している点は技術負債になる可能性もありそうなので、その点については引き続きご検討いただけると嬉しいです💡

unikounio · 2025-02-28T06:15:29Z

test/decorators/user_decorator/role_test.rb

+      controller.stub(:current_user, users(:komagata)) do
+        new_student = decorate(users(:otameshi))
+
+        assert_equal 'new-user', new_student.primary_role


アサーションを追加するなどして、new_studentが「入会から１週間以内の現役生」であることがわかるようにすると、意図がより伝わりやすくなるかもしれません🔰

unikounio · 2025-02-28T06:41:42Z

app/decorators/user_decorator/role.rb

@@ -16,11 +18,14 @@ def roles
                       .map { |v| v[:role] }
      roles << :student if roles.empty?

+      new_student_or_trainee = (roles & %i[student trainee]).any? && elapsed_days <= FIRSTWEEK


&&の前後も変数化するとさらに読みやすくなるかもしれません！
（以下、あくまで一例ですがご参考までに）

is_student_or_trainee = (roles & %i[student trainee]).any? is_within_first_week = elapsed_days <= FIRSTWEEK new_student_or_trainee = is_student_or_trainee && is_within_first_week

unikounio · 2025-02-28T07:30:43Z

app/components/products/product_component.html.slim

@@ -3,7 +3,8 @@
    - if @display_user_icon
      .card-list-item__user
        = link_to user_url(@product.user), class: "card-list-item__user-link" do
-          span class=["a-user-role", role_class]
+          - target_user = defined?(current_user) ? current_user : @product.user
+          span class=["a-user-role", role_class(target_user)]


テスト側でcurrent_userが定義されていないケースがある、という事情だったんですね！
ただ、テスト側の事情を汲むために、本番の実装に特別な処理が入っている状態は、後々理解が難しくなってしまいそうなので、少し気になりました💦

可能であれば、テスト側でcurrent_userを適切にセットする形にして、本番の実装は「ログインユーザーがメンターかどうか」を素直に見る形にした方が、設計としてはシンプルに保てるかなと思いました！
もしuserが常にcurrent_userになるようなら、関連するメソッドの引数も不要になりそうです。

JunichiIto · 2025-02-28T10:37:23Z

app/decorators/user_decorator/role.rb

-    def roles
+    FIRSTWEEK = 7
+
+    def roles(user = current_user)


@harada-webdev
質問なんですが、

Suggested change

def roles(user = current_user)

def roles

とした上で、

roles.unshift('new-user') if new_student_or_trainee && current_user&.mentor?

にすると都合が悪くなるケースってありますか？

unikounio self-requested a review February 25, 2025 22:14

unikounio requested changes Feb 26, 2025

View reviewed changes

unikounio reviewed Feb 26, 2025

View reviewed changes

harada-webdev added 3 commits February 27, 2025 22:19

特定の条件でユーザーのアイコンのclassにnew-userを追加

bb59f97

アイコンのclassにnew-userがついているユーザーを新入生と表示した

c40b886

new-user classの追加で影響するテストを修正

08e2209

harada-webdev force-pushed the feature/add-mark-to-new-user branch from c4baa1e to d34c909 Compare February 27, 2025 13:21

harada-webdev added 5 commits February 27, 2025 23:20

new-userをrolesに追加するかの条件式の一部を変数に切り出した

719f56c

入って最初の1週間を表す7を定数に格納した

a0f6f11

キーワード引数ではなくデフォルト引数を使うようにした

58eac4d

アイコンを表示する際に、current_userを引数とした

f0b004c

戻り値がnew-userになるかのテストを追加した

e8d6de8

harada-webdev force-pushed the feature/add-mark-to-new-user branch from d34c909 to e8d6de8 Compare February 27, 2025 14:28

unikounio approved these changes Feb 28, 2025

View reviewed changes

JunichiIto reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

入って1週間のユーザーにはメンターにしか表示されないマークを表示する #8349

入って1週間のユーザーにはメンターにしか表示されないマークを表示する #8349

harada-webdev commented Feb 18, 2025 •

edited

Loading

harada-webdev commented Feb 18, 2025

JunichiIto commented Feb 24, 2025

harada-webdev commented Feb 24, 2025

unikounio commented Feb 25, 2025

unikounio left a comment

unikounio Feb 26, 2025

harada-webdev Feb 27, 2025 •

edited

Loading

unikounio Feb 26, 2025

harada-webdev Feb 27, 2025 •

edited

Loading

harada-webdev Feb 27, 2025

harada-webdev Feb 27, 2025 •

edited

Loading

unikounio Feb 26, 2025

harada-webdev Feb 27, 2025

unikounio left a comment

harada-webdev commented Feb 26, 2025

harada-webdev commented Feb 27, 2025

unikounio left a comment

unikounio Feb 28, 2025

unikounio Feb 28, 2025

unikounio Feb 28, 2025 •

edited

Loading

JunichiIto Feb 28, 2025

入って1週間のユーザーにはメンターにしか表示されないマークを表示する #8349

Are you sure you want to change the base?

入って1週間のユーザーにはメンターにしか表示されないマークを表示する #8349

Conversation

harada-webdev commented Feb 18, 2025 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後(デザインを入れる前)

harada-webdev commented Feb 18, 2025

JunichiIto commented Feb 24, 2025

harada-webdev commented Feb 24, 2025

unikounio commented Feb 25, 2025

unikounio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harada-webdev Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harada-webdev Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harada-webdev Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unikounio left a comment

Choose a reason for hiding this comment

harada-webdev commented Feb 26, 2025

harada-webdev commented Feb 27, 2025

unikounio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unikounio Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harada-webdev commented Feb 18, 2025 •

edited

Loading

harada-webdev Feb 27, 2025 •

edited

Loading

harada-webdev Feb 27, 2025 •

edited

Loading

harada-webdev Feb 27, 2025 •

edited

Loading

unikounio Feb 28, 2025 •

edited

Loading