ユーザアイコンにroleを表す色枠が出ていないバグの修正

[hirano-vm4]

Issue

• roleを表す色枠が出ていないユーザアイコンがある #7516

概要

日報一覧ページ/reportsのミニアイコンなど、一部のユーザアイコンでroleを表す色枠が表示されていないバグを修正するIsuseです。

下記の該当ページroleが表示されているかの確認をお願いします！

変更確認方法

1. bug/fix-missing-role-border-on-iconsをローカルに取り込む
2. 管理者・アドバイザー・メンターいずれかでログイン
3. http://localhost:3000/reports?page=4 でコメントユーザーのroleを表す色枠が表示されているかを確認
4. http://localhost:3000/products でコメントユーザーのroleを表す色枠が表示されているかを確認
5. http://localhost:3000/external_entries で投稿者のroleを表す色枠が表示されているかを確認

※一般ユーザーは枠線はありません
※同じユーザーが複数回コメントをしてもアイコンの表示は1回のみ

Screenshot

変更前

---

/reportsのミニアイコン

---

productsのミニアイコン

---

/external_entriesの普通のアイコン

---

変更後

---

/reportsのミニアイコン

---

productsのミニアイコン

---

/external_entriesの普通のアイコン

---

[hirano-vm4]

@dowdiness

レビュー依頼

お疲れ様です🙏
お忙しい中、申し訳ありませんがレビューをお願いすることは可能でしょうか🙆？

急ぎではありませんので、無理ないタイミングで全く問題ありません！
ご検討のほど、よろしくお願いいたします🙇‍♂️

[hirano-vm4]

@dowdiness

お疲れ様です🙏
念の為の確認になります〜！

お忙しい中、申し訳ありませんがレビューをお願いすることは可能でしょうか🙆？

急ぎではありませんので、無理ないタイミングで全く問題ありません！
ご検討のほど、よろしくお願いいたします🙇‍♂️

追記

お忙しそうなので今回は別の方に依頼したいと思います！またお願いいたします〜

[hirano-vm4]

@kyokucho1989

お疲れ様です🙏
お忙しい中、申し訳ありませんがレビューをお願いすることは可能でしょうか🙆？

急ぎではありませんので、無理ないタイミングで全く問題ありません！(1~2週間後とかでもOKです)

ご検討のほど、よろしくお願いいたします🙇‍♂️

[kyokucho1989]

@hirano-vm4
了解しました！
一週間くらいかかるかと思いますー

ひとまずコードをざっくり読んで、週末に不明点など質問するかと思います。

[kyokucho1989]

@hirano-vm4
お疲れ様です。まだコードの確認中ですが、コミットメッセージについて、修正が必要かもしれません。

「コメントユーザーアイコンの重複を削除」
699553c

これは、「本Issue以前からアイコンが重複していた」というわけではありませんよね？
今回の試行錯誤の結果、アイコンが重複するようになったので修正した、ということですよね。

あとから見返すと分かりづらくなってしまいそうなので、コミットをまとめるとよいかと思います。

[kyokucho1989]

1f7e5e8

このコミットから最新のコミットまでをひとまとめにするとよいかと思いますー

[hirano-vm4]

@kyokucho1989

ありがとうございます！おっしゃる通り、このIssueで作業の過程で発生した確認の必要のない修正まで細かくコミットしてしまっていたのでコミットをまとめました🙏

確認しにくい状態になっていて、お手数おかけしました！引き続きよろしくお願いします🙇‍♂️

[kyokucho1989]

@hirano-vm4
おはようございます。レビューしました。
1点確認をお願いします！

変更後の影響が大きいようなら、現状のままでも構いません。

[kyokucho1989]

comments.commented_users のままで user.primary_roleを利用できた方がいいかもしれません。

（app/views/api/courses/practices/index.json.jbuilder のように）

comments.commented_usersだとuser.primary_role　が使えません。おそらくCommentモデルのモデルメソッドを呼び出しています。
report.commented_users　なら使えるのはReportモデルで Commentable moduleをincludeしているためだと思います。Commentモデルにも同様にできないか試してみてください。

ただ、他の箇所で弊害が起こる可能性もあるため、問題がでてしまったら今回のままでやったほうがよさそうです。

[hirano-vm4]

@kyokucho1989

お忙しい中、確認いただきありがとうございます🙏

comments.commented_users のままで user.primary_roleを利用できた方がいいかもしれません。（app/views/api/courses/practices/index.json.jbuilder のように）

これは私も同意で当初その方向で進めていましたが、以下の原因から実現できませんでした🧐

またCommentモデルでCommentable moduleをincludeしても状況は変わらずといった状態でした⚡️

原因については下記になります。

primary_roleについて

まず、primary_role自体はUserDecorator(app/decorators/user_decorator/role.rb)というdecorator内に定義されています。

このdecoratorは、ActiveDecoratorというgemで管理されています。

ActiveDecoratorは、ViewでModelが関わるロジック等を扱いたいとき（表示を整形したいとか）にHelperの代替として用いられるもので、明示的にdecorateやincludeしなくても利用できるようになる便利なgemとのことです。

_user_icons.json.jbuilderにreportを渡すとprimary_roleを取得できてcommentsだと取得できない理由

ActiveDecoratorの仕様を確認すると、Controllerで宣言されたインスタンス変数にdecorateメソッドを自動で適用するコードになっていることが確認できました。

自動でデコレーションされる条件

この自動デコレーションは、特定の条件下でのみ行われます。

コントローラーからビューへ渡されるインスタンス変数や、ビュー等でレンダリングされる部分テンプレート（_user_icons.json.jbuilder）に直接渡されるオブジェクトがデコレートの対象となるようです。

report自体を部分テンプレートに渡す場合

この場合はインスタンス変数からオブジェクトを直接渡しているので、部分テンプレート先でreportから辿ってprimary_roleが取得できます。

関連オブジェクトcommentsを部分テンプレートに渡す場合

一方で、関連オブジェクトを渡した場合、そのオブジェクト自体は自動デコレートの条件から外れるため、commentsからprimary_roleが取得できなくなります。

以上が原因のようです！

対策

現状のコードで実現するか、もしくはコード量を減らし、修正点を最小限にする場合は、以下のように_user_icons.json.jbuilderのuser情報を取得する際に明示的にUserDecoratorを適用させる方法もありました。

app/views/api/comments/_user_icons.json.jbuilder

  json.comments do
    json.array! comments.commented_users do |user|
      user.extend(UserDecorator::Role)

      json.user_icon user.avatar_url
      json.user_id user.id
      json.primary_role user.primary_role
    end
  end

どちらの方法が一般的か、また他の方法があるのか含めて駒形さんのレビューでも確認したいと思っています！

長くなってしまい申し訳ありませんが、改めて確認をお願いいたします🙏

[kyokucho1989]

了解しました！

[kyokucho1989]

https://github.com/fjordllc/bootcamp/pull/7559/files#r1547030503

@hirano-vm4
了解しました。Approveしますー。今後の方針は @komagata さんに確認をお願いします。

[hirano-vm4]

@kyokucho1989

ありがとうございました🙏分かりにくい部分を確認いただき、ありがとうございました！引き続きよろしくお願いします！

@komagata

レビュー依頼

チームメンバーによるレビューが終わりました！レビューをお願いします🙏

コードで確認したい点

現状、バグの修正は終了していますが、 app/views/api/reports/index.json.jbuilderから_user_icons.json.jbuilderにreportを渡すとActiveDecoratorが適用され、report.commentsを渡すとprimary_roleを取得できない部分がありました。

これはActiveDecoratorによるもので、関連モデルを別ファイルに渡すと自動デコレートされないことが原因です。

今回はデコレートされたreportを渡すことで対応しましたが、それにともなってこのコミットにあるように複数の.jbuilderも同様に修正しました。

しかし、明示的に以下のようにdecorateすると以下のファイルの一箇所だけの修正だけで実現できそうです。

app/views/api/comments/_user_icons.json.jbuilder

  json.comments do
    json.array! comments.commented_users do |user|
      user.extend(UserDecorator::Role)

      json.user_icon user.avatar_url
      json.user_id user.id
      json.primary_role user.primary_role
    end
  end

上記のアプローチと、現状の変更のどちらがFBCには適しているのか、判断ができなかったのでご意見いただければと思います。

個人的には上記のように明示的にdecorateすると変更するコードの記述量が少ないので良いと考えていますが、他のファイルを見る限り同じような記述はないので統一した方が良いという考えもあるような気がしています。

この点についていかがでしょうか？

[komagata]

@hirano-vm4

上記のアプローチと、現状の変更のどちらがFBCには適しているのか、判断ができなかったのでご意見いただければと思います。

extendが大丈夫なのかちょっと怖い感じがしました。

https://www.satoryu.com/blog/2021/08/01/how-to-use-active_decorator-for-ruby-object.html

partial使ったときにdecorateされるのでjbuilder内でpartialを使うのはどうでしょうか？

[hirano-vm4]

@komagata

extendが大丈夫なのかちょっと怖い感じがしました。

https://www.satoryu.com/blog/2021/08/01/how-to-use-active_decorator-for-ruby-object.html

partial使ったときにdecorateされるのでjbuilder内でpartialを使うのはどうでしょうか？

ありがとうございます！extend を使用すると、オブジェクトの振る舞いがその場その場で変更されるためにコードを追うのも難しくなるのと、意図しない挙動に繋がる恐れがあるので避けたいと思います🙏

参考URLもありがとうざいます！

現在の実装のままでいきたいと思いますがいかがでしょうか？

app/views/api/reports/index.json.jbuilder

• api/comments/user_iconsに渡した先でuser_roleを使える現在の実装

json.reports @reports do |report|
  json.partial! "api/reports/report", report: report
  json.partial! "api/reports/checks", checks: report.checks
  json.partial! "api/comments/user_icons", report: report # コントローラーのインスタンス変数の時点でデコレートされた@reportの要素自身を渡す
  json.user do
    json.partial! "api/users/user", user: report.user
  end
end

このようにするとapi/comments/user_iconsで、デコレートされたreportからcommented_usersと辿ってuser_roleを利用できる。

コントローラー（ActionController）で（暗黙的なものも含めて）呼ばれるrender

• デコレートされない実装

json.reports @reports do |report|
  json.partial! "api/reports/report", report: report
  json.partial! "api/reports/checks", checks: report.checks
  json.partial! "api/comments/user_icons", comments: report.comments # reportから関連オブジェクト`comments`を渡す
  json.user do
    json.partial! "api/users/user", user: report.user
  end
end

上記のように、report.commentsという具体的な関連オブジェクトを直接partialに渡すと、この場合、ActiveDecoratorは自動的にデコレーターを適用しますが。こちらの記述にあるように、render partialの時点でcommentsを渡すと、渡したクラスの文脈からCommentsDecoratorを探してしまうため、user_roleは使えない。

• 他の方法

json.partial! "api/comments/user_icons", comments: report.comments, commented_users: report.commented_users.uniq

このように渡す時点でuserオブジェクトを指定すれば上記のようにUserDecoratoeを自動で適用してくれるので、問題なくuser_roleは使用できました。

これであれば、シンプルにインスタンス変数の時点でデコレートされたものを渡す現在の実装でも良いかな？と個人的には感じましたがいかがでしょうか？

[komagata]

@hirano-vm4 良いと思います～！

[hirano-vm4]

@komagata

ありがとうございます！現在のコードで大丈夫そうであればマージしていただき、ステージング環境にデプロイされたことを確認次第、動作確認に入ります🙆

[hirano-vm4]

@komagata
お忙しいところ、恐れ入ります！こちらいかがでしょうか？

お手隙の際に確認をお願いいたします！

[komagata]

確認させていただきました。OKです～👌