Userが複数のroleを持てるように変更

[naomichi-h]

issue: #3930

ユーザーにroleを複数もたせられるようにした場合の影響範囲の調査

調査方法：.roleで全ファイル検索を行い、ヒットしたファイルについてひとつずつ確認した。

結果：以下の機能、ページに関係する

• ユーザー詳細画面の区分欄
• ユーザーアイコンの縁の色
• 現役生のホーム画面下の未入力項目の表示
• 管理者向け機能
  • 自分以外が作成したQ&A、コメントの編集削除機能
  • プラクティスの編集機能

実施内容

User#roleの変更

bootcamp/app/decorators/user_decorator.rb

Line 16 in 54c4b6d

if student? && !graduated?

にて、複数のroleを持てるように変更。studentのみ、Userテーブルに単体でstudentだと判別できる属性がなかったため、User#student?を使用。User#student?のみだとgraduate_onがtrueなUserもstudent扱いになってしまうため、 if student? && !graduated?とした。

また、この変更により、変更前はUser.roleとすると、roleに応じたsymbolが返されていたが、変更後は、roleに応じた配列が返されるようになっている。

各影響範囲内の機能、ページの変更

現状、User.roleとしたときに配列を返すと不具合が生じる。そこで、修正前と同じ値が返るよう、User.roleを用いるページのほとんどをUser.role[0]とした。

スクリーンショット

ほとんどの画面は変更前と変わらないが、ユーザー詳細画面の区分欄のみ、複数のroleを表示するように変更した。

変更前

変更後

[naomichi-h]

@machida PRのdescriptionに載せたスクリーンショットの場所(複数roleを持つユーザーの、ユーザー詳細画面の区分欄)のみ見た目が変わっています。お手すきのときに確認お願いいたします🙇‍♂️

[machida]

@naomichi-h 複数入るようにマークアップを変更しましたー

[naomichi-h]

@machida 対応ありがとうございます！

@AudioStakes こちらのレビューをお願いできますでしょうか？
よろしくお願いいたします🙏

[AudioStakes]

@naomichi-h
レビュー依頼ありがとうございます😄
一つ気になったことがありますので、教えてくださいー🙏

複数の role を持つユーザーの class 属性について

現在の PR の変更においては、たとえば次のように user.role[0] が設定されています。

bootcamp/app/views/application/_active_users.html.slim

Line 6 in 38733b2

= image_tag user.avatar_url, class: "is-#{user.role[0]}"

これについて、user.role[0] 以外（つまり user.role[1], user.role[2], ...）を設定しなくて良いのかどうか、僕の方では判断がつきませんでした。

そのため、class 属性に設定する role は「どれか1つのみ」もしくは「複数の role すべて」のどちらなのか教えてもらいたいです。

また、それぞれの場合について、以下の質問があります。

「どれか1つのみ」の場合

その1つの role は常に一定に（常に同じ role が選ばれるように）する必要がありますか？
たとえば、「卒業生」と「アドバイザー」の role を持つユーザーに設定する class 属性は「常にアドバイザー」、「常に卒業生」もしくは「卒業生とアドバイザーのどちらでもよい」のどれになるでしょうか。

「複数の role すべて」の場合

お互いに干渉し合う role（たとえば CSS で指定されている色が異なる、など）の場合、どの role を優先するのでしょうか（そのような規則がすでにあるのでしょうか）？

[naomichi-h]

@AudioStakes 質問ありがとうございます🙇‍♂️

class 属性に設定する role は「どれか1つのみ」もしくは「複数の role すべて」のどちらなのか教えてもらいたいです。

どれか一つのみです❗️

本PRでは、今回の変更による影響を最小限に抑えるため、「ユーザー詳細の区分欄以外は全て、変更前と同じroleが出力される」ことを目指しています。

つまり、複数のroleをもつユーザーに設定するclass属性は、「変更前のUser#roleのコードで選ばれるroleと同じ」にしています。

変更前のUser#roleのコードはdetectを使用しているため、複数roleの条件に該当するuserであっても、「最初に条件に当てはまったroleのみ」持つようになっていました。

今回、userが複数roleを持てるように変更をしたのですが、user.role[0]は、「最初に条件に当てはまったrole」であり、変更前のUser#roleの結果と同値になります。

user.role[0]のみを使用し、user.role[1]やuser.role[2]を使用していないのはこのためです。

[AudioStakes]

@naomichi-h
なるほど、わかりやすく説明していただいてありがとうございます！
今の実装が期待される振る舞いになっているのだな、と理解できました。
基本的にOKだと思うものの、改善できそうなところを見つけたので、後ほどコメントします🙏

[AudioStakes]

修正内容について確認しました！
期待どおりの振る舞いになっていますので、基本的には OK です👍

改善できそうなところはコメントしましたので、ご確認お願いしますー

修正する場合は修正後に改めてレビュー依頼をお願いします🙏

[AudioStakes]

※ UserDecorator#role にコメントしたつもりだったのですが、位置がずれてしまいました🙇‍♂️

2つほど改善できそうなところがあったのでコメントしますー
どちらも「必ず修正してほしい」という意図はありません。もし改善されていると実感できるようであれば、修正してみてもらえたらと思いますー🙏

改善案1

UserDecorator#role について、修正前後の返り値が「最初に条件に当てはまった role 」から「条件に当てはまる role すべて」に変わっていますね。
この変更に伴って、修正が必要となる View 関連のファイルが多数生じている（すなわち影響範囲が大きい）ため、 UserDecorator#role の返り値は変えない方がよいかもしれません。

代替案として、UserDecorator#role はそのままにして、「条件に当てはまる role すべて」を返す UserDecorator#roles を追加するという案があります。
UserDecorator#roles の処理の中身は、今の修正後の UserDecorator#role そのままです。
次のようなイメージです。

module UserDecorator

~省略~

  def role
    roles = [
      { role: :admin, value: admin },
      { role: :mentor, value: mentor },
      { role: :adviser, value: adviser },
      { role: :trainee, value: trainee },
      { role: :graduate, value: graduated_on },
      { role: :student, value: true }
    ]
    roles.detect { |v| v[:value] }[:role]
  end

  def roles
    roles = [
      { role: :admin, value: admin },
      { role: :mentor, value: mentor },
      { role: :adviser, value: adviser },
      { role: :trainee, value: trainee },
      { role: :graduate, value: graduated_on }
    ]
    if student? && !graduated?
      [:student]
    else
      roles.select { |v| v[:value] }.map { |h| h[:role] }
    end
  end

~省略~

end

UserDecorator#roles を追加した後に、次の app/views/users/_metas.html.slim の user.role.each を user.roles.each に変更することで、ユーザー詳細画面の区分欄には今の修正後のとおり複数の role を表示できます。

bootcamp/app/views/users/_metas.html.slim

Lines 68 to 70 in 38733b2

	- user.role.each do |role|
	li
	= t "target.#{role}"

これにより、それ以外（ユーザー詳細画面の区分欄以外）は View まわりのコードを修正しなくて済むようになるので、このような実装にしてもよさそうですー

改善案2

初めて UserDecorator#role の処理を見たとき、どんな値が返ってくるか少し考える必要がありました。
※ 同様に、UserDecorator#role に基づいて実装された UserDecorator#roles も少し考えました。

そのため、UserDecorator#role （と UserDecorator#roles） はもう少しわかりやすくできるかもしれません。

僕の改善案としては、以下のようにすると返り値が少しわかりやすくなるかもしれないなと思いました。

  def role
    return :admin if admin?
    return :mentor if mentor?
    return :adviser if adviser?
    return :trainee if trainee?
    return :graduate if graduated_on?

    :student
  end

  def roles
    roles = []

    roles << :admin if admin?
    roles << :mentor if mentor?
    roles << :adviser if adviser?
    roles << :trainee if trainee?
    roles << :graduate if graduated_on?
    roles << :student if roles.empty?

    roles
  end

※ 個人的にはもっとわかりやすくできそうな気がしています😅

もしより良い案がありましたら、そちらを選んでもらえたらと思いますー👍

---

追記

UserDecorator#role は以下のようにした方が簡潔で良いかもしれないな、と思いましたー

  def role
    roles.first
  end

※ UserDecorator#roles は既存の UserDecorator#roleと優先順位が同じになるようにして配列を返しているつもりです（何か見落としているかもしれません）

[AudioStakes]

改善できそうなところをもう1つ見つけたので、お伝えしますー🙏

改善案3

複数の role を持つユーザーを例に考えると、既存の UserDecorator#role が返す値は「複数の role の中で最も優先順位の高い role 」と捉えることができると思います。
それを表すために、メソッド名をUserDecorator#primary_role に変更すると良いかもしれません。

この変更により UserDecorator#role を使っている箇所のメソッド名を変更する必要が生じるものの、 role の用途に応じて user.primary_role と user.roles を使い分けしやすくなり、コードの読み手にもその意図が伝わりやすくなると思うので、このような実装にしても良さそうですー

[naomichi-h]

@AudioStakes たくさんの改善案ありがとうございます😊

たしかに！と思う部分が多数あったため、全面的に取り入れ、以下のように修正しました。

• UserDecorator#rolesを追加
• UserDecorator#rolesを提案されたものに変更（実は僕も最初、UserDecorator#roleの処理がわかりづらいなと感じました。)
• UserDecorator#roleをUserDecorator#primary_roleとし、中身をroles.firstに変更

お手数ですが、確認をお願いいたします🙏

[AudioStakes]

@naomichi-h
ありがとうございます、修正内容を確認しました！
関数名について気づいたことをコメントしましたので、ご確認お願いしますー🙏
それ以外はOKです👍

---

追記

この PR の Assignees に、 naomichi さんがアサインされていないようですー

[AudioStakes]

関数 roleAdmin と roleAdviser はどちらも Boolean 型の値( true or false )を返していますね。
そのような関数には isHoge や hasFuga のように命名することが多いです。

Suggested change

	roleAdmin: function () {
	return this.currentUser.roles.includes('admin')
	},
	roleAdviser: function () {
	return this.currentUser.roles.includes('adviser')
	}
	isAdmin: function () {
	return this.currentUser.roles.includes('admin')
	},
	isAdviser: function () {
	return this.currentUser.roles.includes('adviser')
	}

JavaScript の命名規則の参考資料を見つけられなかったため、代わりに bootcamp アプリ内のコードを例に示しますと以下のような関数があります

bootcamp/app/javascript/reports.vue

Lines 75 to 77 in 400969f

	isUncheckedReportsPage() {
	return location.pathname.includes('unchecked')
	},

bootcamp/app/javascript/answers.vue

Lines 97 to 99 in 400969f

	hasCorrectAnswer: function () {
	return this.answers.some((answer) => answer.type === 'CorrectAnswer')
	},

それぞれの関数名を一括で置換して対応できると思いますので、修正をお願いできますでしょうか🙏

[naomichi-h]

@AudioStakes

関数 roleAdmin と roleAdviser はどちらも Boolean 型の値( true or false )を返していますね。
そのような関数には isHoge や hasFuga のように命名することが多いです。

おお〜、そうなのですね！ありがとうございます😊いや本当に勉強になります✨
提案いただいた内容で修正しましたので、確認をよろしくお願いいたします🙏

[AudioStakes]

@naomichi-h
確認しました、OKですー👍
修正対象のファイル数が多くて修正に手間がかかったかもしれませんが、改善案を取り入れていただきありがとうございます😄

[naomichi-h]

@AudioStakes こちらこそ、多くの修正をみていただいてありがとうございました 😊 @AudioStakes さんにお願いしてよかったです✨

@komagata こちらチームメンバーのレビュー通過しましたので、お手隙のときに確認をよろしくお願いいたします🙇‍♂️

[komagata]

共通の処理はmixinにすると良いかもです〜

[naomichi-h]

二つの処理をisRole()としてまとめました！

[komagata]

@naomichi-h mixinはVue.jsの仕組みとして存在するmixinのことです〜

[naomichi-h]

完全に勘違いしていました...😓
mixin使ってisRoleメソッドをファイルに分離しました！

[naomichi-h]

@komagata 修正を行いましたので、ご確認お願いいたします🙏

[naomichi-h]

rubocopにて

Lint/IneffectiveAccessModifier: private (on line 265) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.

と警告が出ていたため、self.product_updateをprivate外に出しました。
(警告通りにprivate_class_methodなどを使ってself.product_updateをprivate化してしまうと、NoMethodErrorが発生し、products_test:23でエラーになってしまうためです。)

[naomichi-h]

#4103
の内容をpullしました！(修正方法は同様)

[naomichi-h]

@komagata 修正とコメントをしましたので、確認のほどよろしくお願いいたします🙏

[komagata]

mixin名がis-roleだとこのメソッド一つ限定になっちゃうのでroleとか広い意味の方がいいかもです〜

[naomichi-h]

なるほど、たしかに今後role関係のメソッドを切り出したりすることを考えると、広い意味の方が良いですね😄
mixin名をroleに変更します！

[naomichi-h]

@komagata 修正とコメントをしましたので、確認のほどよろしくお願いいたします🙏

[komagata]

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

[naomichi-h]

@komagata これがチーム開発最後のマージとなりました✨ ありがとうございました！！

[komagata]

@naomichi-h うお〜、お疲れ様です〜！開発ありがとうございました〜！！