プラクティス、コース、カテゴリーの編集権限を管理者からメンターに移行する #6344

mono-nobe · 2023-03-21T07:44:13Z

Issue

[プラクティス編集]プラクティス、コース、カテゴリーの編集権限を管理者からメンターに移行。 #6210

概要

プラクティス、コース、カテゴリーの編集権限を管理者からメンター(管理ページからメンターページ)に移行しました。
また、管理ページから上記項目を削除しました。

変更確認方法

feature/grant-edit-permission-to-mentor-for-categories-courses-practices をローカルに取り込む
mentormentaroでログインする
ヘッダーからメンターページに移動する

プラクティス、コース、カテゴリーのタブがあることを確認する
プラクティスページについて、以下の内容を確認する
- プラクティス一覧が表示されている
- 以下のリンクが正常に動作している
  - プラクティス
  - 所属カテゴリー
  - 提出物
  - 日報
  - Q&A
  - 編集
- プラクティスを編集できる
カテゴリーページについて、以下の内容を確認する
- カテゴリー一覧が表示されている
- 以下のリンクが正常に動作している
  - 名前
  - 操作
- カテゴリーを作成できる
- カテゴリーを編集できる
- カテゴリーを削除できる
コースページについて、以下の内容を確認する
- コース一覧が表示されている
- 以下のリンクが正常に動作している
  - コース名
  - 操作
    - 編集
    - 並び替え
- コースを作成できる
- コースを編集できる
- コースを並び替えられる
  - 具体的な操作については後述します
- プラクティスを並び替えられる
  - 具体的な操作については後述します
- プラクティスを作成できる
komagataでログインする
ヘッダーから管理ページに移動する

プラクティス、コース、カテゴリーのタブが存在しないことを確認する

コースを並び替える操作

メンターページへ移動する
- 上記の手順3まで同じ
コースタブをクリックする

操作列のハンバーガーメニューをクリックし、カテゴリー並び替え画面へ移動する

並び替えたいレコードの並び順カラムを上下にドラッグすることで、レコードを並び替える
- 保存ボタンはなく、上記操作で自動的に並び順が保存される

プラクティスを並び替える操作

メンターページのコースタブをクリックする
- コースを並び替える手順2まで同じ
適当なコースをクリックする

順番変更ボタンをクリックし、並び替えモードにする

並び替えたいレコードのハンバーガーメニューをドラッグすることで、レコードを並び替える

変更完了ボタンをクリックし、変更内容を保存する

Screenshot

変更前

メンターページ

プラクティス、コース、カテゴリーのタブが存在しない

管理ページ

プラクティス、コース、カテゴリーのタブが存在する

変更後

メンターページ

プラクティス、コース、カテゴリーのタブが存在する

管理ページ

プラクティス、コース、カテゴリーのタブが存在しない

mono-nobe · 2023-03-29T14:04:17Z

@wata00913
お疲れ様です！
もしご都合よろしければ、こちらレビューいただけないでしょうか？
差分がかなり多いため、PRの内容含め疑問点あればお気軽にご質問ください 🙇

wata00913 · 2023-03-29T22:49:03Z

@mono-nobe
お疲れ様です！
レビュー依頼ありがとうございます！
レビューに１週間ほど要しそうですが、大丈夫でしょうか？

mono-nobe · 2023-03-30T00:05:00Z

@wata00913
はい！大丈夫です〜
よろしくお願いします！ 🙏

wata00913 · 2023-04-05T05:34:57Z

app/controllers/api/mentor/base_controller.rb

+# frozen_string_literal: true
+
+class API::Mentor::BaseController < ApplicationController
+  skip_before_action :require_active_user_login, raise: false


require_active_user_loginをスキップする理由は何故でしょうか？

前提として、ApplicationController で before_action :require_active_user_login が定義されているため、継承している API::Mentor::BaseController においても before_action :require_active_user_login が実行されてしまいます。
しかし、この Controller は API 専用であるため画面でのログインができないため、 skip_before_action しています！

代わりに、同 Controller 内にある before_action :require_mentor_login_for_api で権限チェックをしています。

bootcamp/app/controllers/api/mentor/base_controller.rb

Line 5 in 8a9bb1e

before_action :require_mentor_login_for_api

詳しく説明して頂きありがとうございます！require_active_user_loginをスキップする理由が理解できました〜

wata00913 · 2023-04-05T05:39:43Z

app/controllers/mentor/categories_controller.rb

@@ -17,7 +17,7 @@ def create
    @category = Category.new(category_params)

    if @category.save
-      redirect_to admin_categories_url, notice: 'カテゴリーを作成しました。'
+      redirect_to mentor_categories_url, notice: 'カテゴリーを作成しました。'
    else


以下のCoursesController#createアクションとMentor::CoursesController#createの処理が重複しているようです。CoursesController#createは削除しても良さそうです。
Mentor::CoursesController側の処理を残す理由は、名前空間のMentorがあるので責務がより明確になると思いました〜

bootcamp/app/controllers/courses_controller.rb

Line 14 in 6eef41e

def create

CoursesController#createはコース一覧画面/coursesの「コース作成」ボタンをクリック時に実行されるようです。
もし、Mentor::CoursesController#createに処理をまとめる場合は、「コース作成」ボタンの遷移先URLを変更する必要がありそうです。

確認したところ、同じ機能を持つ画面が複数存在しているようでした...
そのため、wataさんのおっしゃる通り mentor 用の画面( /mentor/courses/...)のみ残す方が良いかと思います！
しかし、そちらの対応は今回の issue のスコープ外である気もするため、一度駒形さんに確認させてください 🙇

📝
こちらで確認中
#6344 (comment)

こちら別issueで対応する方針となりましたので、ご確認をお願いします！
#6344 (comment)

こちらの確認、自分の方で早めに対応しておくべきでした。
もののべさんの負担をかけてすみませんでした🙇‍♂️

@wata00913
いえいえ！
wataさんのご指摘がなかったら今後対応されることはなかったと思うのでとても助かりました！🙌

wata00913 · 2023-04-05T05:51:42Z

app/controllers/mentor/practices_controller.rb

+
+class Mentor::PracticesController < MentorController
+  def index; end
+end


以下のアクションをMentor::PracticesContorllerに移動させた方が責務が明確になるのではないかと思いました。いかがでしょうか？
ただし、処理を移す作業が大変そうなので駒形さんに意見を聞いた方が良さそうです。

bootcamp/app/controllers/practices_controller.rb

Line 28 in 6ebdbcf

def create

bootcamp/app/controllers/practices_controller.rb

Line 38 in 6ebdbcf

def update

影響範囲の調査までありがとうございます！
こちらも駒形さんに確認します！

📝
こちらで確認中
#6344 (comment)

同様に別issueで対応する方針となりました。
#6344 (comment)

wata00913 · 2023-04-05T05:53:42Z

app/javascript/courses-practices.vue

@@ -14,9 +14,9 @@
              v-if='category.completed_all_practices === true')
              | 修了
        .categories-item__description
-          .categories-item__edit.is-only-mentor(v-if='isRole("admin")')
+          .categories-item__edit.is-only-mentor(v-if='isRole("mentor")')


adminの条件分岐を考慮をしなくても大丈夫でしょうか？

adminユーザーは必ずmentorユーザーでもあるため、mentorのみを考慮しています！

wata00913 · 2023-04-05T12:15:56Z

app/views/courses/index.html.slim

          = render @courses.order(:created_at)
-        - elsif !current_user&.admin? && @courses.where(published: true).present?
+        - elsif !current_user&.mentor? && @courses.where(published: true).present?


こちら（18行目、20行目）もadminの条件分岐を考慮をしなくても大丈夫でしょうか？

同様の理由でmentorのみ考慮しています。

wata00913 · 2023-04-05T12:32:48Z

app/views/courses/_course.html.slim

              .courses-item__title-icon.is-closed
                | 非表示
            span.courses-item__title-label
              = course.title
      .courses-item__description
        = simple_format(course.description)
-      - if admin_login?
+      - if mentor_login?


こちらも同様です。

同様の理由でmentorのみ考慮しています。

wata00913 · 2023-04-05T13:02:10Z

app/views/mentor/categories/show.html.slim

-            = link_to new_admin_category_path, class: 'a-button is-md is-secondary is-block' do
-              i.fa-regular.fa-plus
-              | カテゴリー作成
+        - if current_user.admin_or_mentor?


MentorControllerにてログインユーザーの判定をしているので削除して良いかなと思いました。

bootcamp/app/controllers/mentor_controller.rb

Line 4 in 39cd8ea

before_action :require_admin_or_mentor_login

おっしゃる通り、mentor専用のページでもあるため判定は不要ですね 👀
削除しました！

wata00913 · 2023-04-05T13:03:48Z

app/views/mentor/courses/index.html.slim

+    .page-header__inner
+      h2.page-header__title = title
+      .page-header-actions
+        - if current_user.admin_or_mentor?


こちらの条件処理もMentorControllerにてログインユーザーの判定をしているので削除して良いかなと思いました。

こちらも同様の理由で削除しました！

wata00913 · 2023-04-05T13:05:56Z

test/system/admin/home_test.rb

@@ -6,5 +6,12 @@ class Admin::HomeTest < ApplicationSystemTestCase
  test 'GET /admin' do
    visit_with_auth '/admin', 'komagata'
    assert_equal '管理ページ | FBC', title
+    assert_selector '.page-tabs__item-link', text: '管理ページ', visible: true


visible: trueが必要なのは何故でしょうか？

Capybara の仕様を正しく理解していませんでした...
オプション不要なため削除しました！

wata00913 · 2023-04-05T13:16:17Z

test/system/admin/home_test.rb

+    assert_selector '.page-tabs__item-link', text: 'ユーザー', visible: true
+    assert_selector '.page-tabs__item-link', text: '企業', visible: true
+    assert_selector '.page-tabs__item-link', text: 'お試し延長', visible: true
+    assert_no_selector '.page-tabs__item-link', text: 'プラクティス', visible: true


タブがないことを保証するテストを書いてて良いですね！👍
もう少し抽象化出来ると良さそうです！例えば、.page-tabs__item-linkをもつselectorの個数が4つ(つまり表示されているタブの個数)であることを保証するテストコードに置き換えるのはどうでしょうか？

タブがないことを保証するテストを書いてて良いですね！👍

ありがとうございます！😊

例えば、.page-tabs__item-linkをもつselectorの個数が4つ(つまり表示されているタブの個数)であることを保証するテストコードに置き換えるのはどうでしょうか？

確かに、現状だとタブが増えてもテストが通ってしまいますね... 🤔
何のタブが表示されているかのテストは残したいので、selector の個数も確認するよう追加しました！

wata00913 · 2023-04-05T13:21:23Z

test/system/mentor/home_test.rb

+    assert_selector '.page-tabs__item-link', text: 'コース', visible: true
+    assert_no_selector '.page-tabs__item-link', text: 'ユーザー', visible: true
+    assert_no_selector '.page-tabs__item-link', text: '企業', visible: true
+    assert_no_selector '.page-tabs__item-link', text: 'お試し延長', visible: true


こちらのコメントと同じです。

同様にassertionを追加しました！

wata00913

@mono-nobe
mononobeさん、お待たせしました🙏

動作確認について

動作確認はOKでした〜

変更確認方法について確認したいこと、指摘内容があります。

確認したいこと
1. 手順7.「コースページについて、以下の内容を確認する」の「コースを並び替えられる」での動作確認対象のページは、 /courses/{course_id}/categories で合っているでしょうか？
2. 手順7.「コースページについて、以下の内容を確認する」の「プラクティスを並び替えられる」での動作確認対象のページは、 /courses/{course_id}/practices/sort で合っているでしょうか？
指摘内容
- 動作確認「コースを並び替えられる」と「プラクティスを並び替えられる」は具体的な操作方法が無いため、確認に戸惑ってしまいました。この2点のみでも、具体的な操作手順を書いた方が、初見の人でも対応しやすいのかなと思いました〜
- 手順7.「コースページについて、以下の内容を確認する」で添付された以下の図が、どの操作に対してのものか不明でした。図についての説明があると分かりやすいと思いました。
- 手順7.「コースページについて、以下の内容を確認する」で、「コースを作成できる」の記述漏れがありました。動作確認はOKでした！

コードについて

細かいコメントはコード毎に記載しております。自分の観点でコメントしましたが、気になることや間違いがあれば是非ご指摘をよろしくお願いします🙏

あと、１点確認したいことがあります。
管理者はメンターの役割も必ず持つのでしょうか？
今回のIssueではプラクティス、カテゴリー、コースの編集権限を管理者だけでなく、メンターも追加することだと思いますが、メンターのみを考慮したコードが幾つかありました。
メンターでない管理者がいるケースだと、メンターのみを考慮したコードの修正が必要だと思います。

mono-nobe · 2023-04-18T14:53:46Z

@wata00913
レビューありがとうございます！
対応までお時間いただくかと思いますが、引き続きよろしくお願いします... 🙇

mono-nobe · 2023-05-19T08:50:55Z

@komagata
リファクタリングが必要そうな箇所があるのですが、本 issue で対応するべきかをご相談させてください。
現状、adminのみ実行できるControllerメソッドが、admin用のControllerがあるにも関わらず、一般？のController内で定義されております。
理想は、adminのみ実行できるメソッドはadmin用のControllerのみに定義するべきだと考えています。
しかし、これらを改修するとなると controller 以外も改修する必要があるため、対応範囲が大きくなってしまいます。
本 issue はあくまで admin 権限を mentor に移行することであるため、リファクタリングは別 issue での対応としたいのですが、いかがでしょうか？ 👀

#6344 (comment)
#6344 (comment)

mono-nobe · 2023-05-19T12:55:31Z

リファクタリングが必要そうな箇所があるのですが、本 issue で対応するべきかをご相談させてください。

こちら、リファクタリングは別Issueにしたいと思いますー
Issueを作ってもらえると助かります🙏

こちら @machida さんよりご回答いただき、別 issue(#6536) で実施することになった

https://discord.com/channels/715806612824260640/809595476847493192/1109043419441016862

mono-nobe · 2023-05-19T14:21:56Z

@wata00913
大変お待たせしました。。 🙇
コードだけではなく、PRの内容や仕様周りも丁寧にレビューいただきとても助かります！
コメントいただいた内容について対応しましたので、ご確認をお願いします。
また、ご質問(指摘)に対する回答を以下に記載しておりますので、併せてご確認をお願いします。

確認したいこと

はい！どちらとも合っております！ 🙆‍♂️

指摘内容

失礼しました。
見返すと確かに説明不足ですね... 😓
いただいた内容を追記しましたので、過不足ないか再度ご確認いただけると幸いです。

細かいコメントはコード毎に記載しております。自分の観点でコメントしましたが、気になることや間違いがあれば是非ご指摘をよろしくお願いします🙏

ありがとうございます！😊
本issueで対応するか迷った箇所については町田さんに確認し回答しております！

管理者はメンターの役割も必ず持つのでしょうか？

こちら確認したところ、管理者はメンターの役割を必ず持つとのことでした！

これからは時間が取りやすくなるため、今回のように時間がかかることはないかと思います。。
引き続きよろしくお願いします。

mono-nobe · 2023-05-19T15:35:13Z

@wata00913
現在テストが落ちているため、通り次第再度ご連絡します 🙇

wata00913 · 2023-05-23T05:31:44Z

@mono-nobe

現在テストが落ちているため、通り次第再度ご連絡します 🙇

了解しました、動作確認やコードの確認はテストが通った後確認しようと思います！

それと、現在コンフリクトが起きているみたいなので、対応の方を後程よろしくお願いします！🙏

- 各種作成 - カテゴリー削除 - プラクティスの順番変更

mono-nobe · 2023-07-02T11:29:31Z

@komagata
いただいた内容を修正しましたので、ご確認をお願いします！

komagata

確認させて頂きました。OKです〜🙆‍♂️

mono-nobe self-assigned this Mar 21, 2023

mono-nobe force-pushed the feature/grant-edit-permission-to-mentor-for-categories-courses-practices branch 2 times, most recently from cb05234 to 67d5207 Compare March 21, 2023 16:19

mono-nobe marked this pull request as ready for review March 29, 2023 13:56

mono-nobe requested a review from wata00913 March 29, 2023 14:00

wata00913 assigned komagata and unassigned komagata Mar 30, 2023

wata00913 reviewed Apr 5, 2023

View reviewed changes

mono-nobe force-pushed the feature/grant-edit-permission-to-mentor-for-categories-courses-practices branch from 6eef41e to 8a9bb1e Compare May 19, 2023 07:00

mono-nobe force-pushed the feature/grant-edit-permission-to-mentor-for-categories-courses-practices branch from a3ea8b7 to 2ff8a75 Compare May 24, 2023 11:16

mono-nobe added 23 commits July 2, 2023 18:41

categoriesのmentorの権限を絞る

eec1120

coursesの不要なadmin用コンポーネントを削除

a81a6b4

practicesのadminの権限を絞る

f60b669

管理ページの不要なタブを削除する

3293173

不要なデバッグ用ログを削除

f61e377

coursesの編集画面へのpathをadminからmentorに変更

245ae2b

courses-practicesのcategory編集ボタンをメンターにも表示する

bb01be4

courses一覧画面の条件式を修正

efbc952

以下の権限を管理者からメンターに移行

870fafc

- 各種作成 - カテゴリー削除 - プラクティスの順番変更

lint修正

d6f1548

失敗しているシステムテストを修正

e1725e1

管理者・メンター用ページに表示されるタブのテストを追加

cd8cadf

メンターがプラクティスを並べ替えられることを確認するテストを追加

8d90501

不要なテストケースを削除

a836ff2

各ページのシステムテストを移行

0416e8e

不要な判定式の削除

e0d667f

表示されているタブの個数をテスト

0ec63cd

カテゴリー作成時のバリデーションエラー表示機能の移行

6df7ccd

テスト修正

06ce89f

権限チェックをメンターのみに変更

0cc6a68

システムテストの修正

4b5e909

不要なボッチ演算子を修正

c9335af

タイトルに合っていないかつ壊れやすいテストを削除

e21f240

mono-nobe force-pushed the feature/grant-edit-permission-to-mentor-for-categories-courses-practices branch from 0d4cb90 to e21f240 Compare July 2, 2023 09:41

komagata approved these changes Jul 11, 2023

View reviewed changes

komagata merged commit ca94879 into main Jul 11, 2023
2 checks passed

komagata deleted the feature/grant-edit-permission-to-mentor-for-categories-courses-practices branch July 11, 2023 12:42

github-actions bot mentioned this pull request Jul 11, 2023

Release 2023-07-13 09:14:13 #6714

Merged

6 tasks

mono-nobe mentioned this pull request Jul 31, 2023

[プラクティス編集]プラクティス、コース、カテゴリーの編集権限を管理者からメンターに移行。 #6210

Closed

プラクティス、コース、カテゴリーの編集権限を管理者からメンターに移行する #6344

プラクティス、コース、カテゴリーの編集権限を管理者からメンターに移行する #6344

Conversation

mono-nobe commented Mar 21, 2023 • edited Loading

Issue

概要

変更確認方法

コースを並び替える操作

プラクティスを並び替える操作

Screenshot

変更前

メンターページ

管理ページ

変更後

メンターページ

管理ページ

mono-nobe commented Mar 29, 2023

wata00913 commented Mar 29, 2023

mono-nobe commented Mar 30, 2023

Choose a reason for hiding this comment

mono-nobe May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mono-nobe May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mono-nobe May 19, 2023 • edited Loading

Choose a reason for hiding this comment

wata00913 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 left a comment • edited Loading

Choose a reason for hiding this comment

動作確認について

コードについて

mono-nobe commented Apr 18, 2023

mono-nobe commented May 19, 2023

mono-nobe commented May 19, 2023

mono-nobe commented May 19, 2023

mono-nobe commented May 19, 2023

wata00913 commented May 23, 2023

mono-nobe commented Jul 2, 2023

komagata left a comment

Choose a reason for hiding this comment

mono-nobe commented Mar 21, 2023 •

edited

Loading

mono-nobe May 19, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

mono-nobe May 19, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

mono-nobe May 19, 2023 •

edited

Loading

wata00913 Apr 5, 2023 •

edited

Loading

wata00913 left a comment •

edited

Loading