Q&AページのQuestion部分を非Vue化した #7439

junohm410 · 2024-02-26T09:18:30Z

Issue

Q&AページのQuestion部分を非Vue化したい #7356

概要

個別のQ&Aページ（https://bootcamp.fjord.jp/questions/{id}）のうち、答え部分を除いた質問本体部分を、Vueコンポーネント（app/javascript/components/question-page.vueの一部）による実装ではなく、Railsのviewテンプレートによる実装に変更しました。

これにより、質問の更新時にhttps://bootcamp.fjord.jp/questions/{id}/editへの画面遷移が発生するようになっています。

質問ページ下部の答え部分（AIの答え＋投稿された答え）は別Issueで非Vue化するとのことなので、こちらは既存の実装を残しております。
（app/javascript/components/question-page.vueの質問部分を省いたコードをリネームし、単体で動くように修正）

また、副次的に以下の点が変更されています。

質問作成・更新時のフラッシュメッセージの整理
質問作成時にも「プラクティス選択なし」を選べるようにする
「プラクティス選択なし」の状態かつ質問本文でユーザーがメンションをされると、メンション通知のタイトルで使われるプラクティス名部分は「プラクティス選択なし」になる

質問作成・更新時のフラッシュメッセージの整理

以下のように整理されています。
過去の状態・詳しい議論の経緯は以下のコメントをご参照ください。
#7356 (comment)

# 新規作成
新規作成（公開） -> 「質問を作成しました」
新規作成（WIP） -> 「質問をWIPとして保存しました」

# WIP -> 公開
「質問を更新しました」

# 公開 -> WIP
「質問をWIPとして保存しました」

# 公開 -> 公開（内容のみ変更）
「質問を更新しました」

# WIP -> WIP（内容のみ変更）
「質問をWIPとして保存しました」

質問作成時にも「プラクティス選択なし」を選べるようにする

質問更新時に選べるようになっていた「プラクティス選択なし」を、質問作成時でも選べるようにしました。
非Vue化で、作成・更新ともに同じ_formパーシャルを使うようになったためです。
過去の状態・詳しい議論の経緯は以下のコメントをご参照ください。
#7356 (comment)

「プラクティス選択なし」の状態かつ質問本文でユーザーがメンションをされると、メンション通知のタイトルで使われるプラクティス名部分は「プラクティス選択なし」になる

現在の本番環境では、上記の条件で質問が作成・更新されると、存在しない関連プラクティスのタイトルを通知タイトルに使おうとするため、エラーが起こります。
プラクティスが選択されていない質問でのメンション通知は、「プラクティス選択なし」をタイトルとして扱うように変更し、上記のバグを修正しています。
エラーの状態・詳しい議論の経緯は以下のコメントをご参照ください。
#7356 (comment)

作業内容のおおまかなイメージ

変更前のQ&AページのQuestion部分の構成

# Vue・質問＋答え全体

- QuestionPage（app/javascript/components/question-page.vue、大元の親コンポーネント）
    - QuestionEdit　（質問本文・編集画面、Vue）
        - UserIcon　（ユーザーアイコン、Vue）
        - WatchToggle　（Watchボタン、Vue）
        - BookmarkButton　（ブックマークボタン、Vue）
        - tags　（タグ機能、Vue）
        - reaction　（絵文字によるリアクション機能、Vue）
    - AIAnswer　（メンターのみのAI回答機能、Vue）
    - Answers　（投稿された回答一覧、Vue）
        - Answer　（個別の回答、Vue）

変更後のQ&AページのQuestion部分の構成

# Railsのview・質問部分

- questions/show
    - render 'question_header'
        - render 'users/icon'　（ユーザーアイコンは非同期処理の必要がないため、既存のパーシャルに差し替え）
        - WatchToggle　（同じVueコンポーネントをrailsのviewから読み込み）
        - BookmarkButton　（同じVueコンポーネントをrailsのviewから読み込み）
        - tags　（React版のタグ機能がすでに実装済みのため、そちらをrailsのviewから読み込み）
    - render 'question_body'
        - render 'reactions/reactions'　（VueではなくpureなJSで実装されたリアクション機能を持つ既存のパーシャルがあるため、そちらに差し替え）

# Question部分を切り取ったVue・答え部分

- QuestionAnswers（app/javascript/components/question-answers.vue、大元の親コンポーネントをリネームし、単独で動くよう修正）
    - AIAnswer
    - Answers
        - Answer

変更後の非同期の部品をRailsのパーシャルに差し替えるのか、Reactに差し替えるのかなどの選択については、非Vueでscaffoldライクに実装されているpagesやreportsと合わせることを基準としております。

変更確認方法

画面遷移を伴った更新（通常の更新、公開 -> WIP、WIP→公開）・フラッシュメッセージ・「プラクティス選択なし」の追加について、下記の方法でご確認いただけます。

chore/convert-question-section-from-vue-to-slimをローカルに取り込む
サーバーを立ち上げる。
komagataでログインし、http://localhost:3000/questions/newにアクセスする。
プラクティス入力部分が「プラクティス選択なし」であることを確認する。プラクティス・タイトル・本文・タグを入力（内容はなんでもOK）し、「登録する」ボタンを押す。
「質問を作成しました。」というフラッシュメッセージが出て、遷移先の質問ページで質問が作成されたことを確認し、そのまま「内容修正」ボタンを押す。
http://localhost:3000/questions/{id}/editに遷移したことと、4. で入力した内容がセットされていることを確認する。
プラクティス・タイトル・本文・タグを編集し（内容はなんでもOK）、「更新する」ボタンを押す。
「質問を更新しました。」というフラッシュメッセージが出て、遷移先の質問ページで編集内容が反映されたことを確認し、そのまま「内容修正」ボタンを押す。
遷移先で、「WIP」ボタンを押す。
「質問をWIPとして保存しました。」というフラッシュメッセージが出たことを確認する。
4.で作成した質問のページにアクセスする（1. 〜10. の操作の直後の場合、http://localhost:3000/questionsの一番上にリンクがあると思います）。
質問タイトルの左横に「WIP」マークがあることを確認し、「内容修正」ボタンを押す。
遷移先で、「質問を公開」ボタンを押す。
「質問を更新しました。」というフラッシュメッセージが出て、遷移先の質問ページで、質問タイトルの左横に「未解決」マークがあること（＝WIPでない）を確認する。

「プラクティス選択なし」かつメンションをした際にバグが起きないことは以下の方法でご確認いただけます。

kimuraでログインし、質問作成画面にアクセス。
「プラクティス選択なし」の状態のまま、タイトルを入力。本文では@hatsunoと入力する（hatsunoにメンションを飛ばす）。
「登録する」をクリックする。
質問が登録されたことを確認したら、hatsunoでログインする。
右上の通知ベルを開き、「kimuraさんのQ&A「プラクティス選択なし」でkimuraさんからメンションがきました。」という通知があること確認する。

Screenshot

変更前

質問更新時に画面遷移が起こらず、同じページ上で質問本文部分が編集UIに書き換わる。

変更後

質問更新時にhttps://bootcamp.fjord.jp/questions/{id}/editへの遷移が起こる（通常のRailsのscaffoldライクな動き）。

junohm410

変更部分の説明です。

junohm410 · 2024-02-27T01:33:07Z

app/controllers/api/questions_controller.rb

@@ -41,6 +40,6 @@ def update
  private

  def question_params
-    params.require(:question).permit(:title, :description, :practice_id, :tag_list, :wip)


非Vue化に伴い、APIを叩いて質問を更新するシナリオは「質問個別ページ上からのタグの編集」のみになりました。
なので、tag_list以外のパラメータは受け付けないようにしています。

またupdateアクションから、Newspaperによる通知の処理を削除しました。
この処理は更新によりwip状態が変更された場合に起こる処理でしたが、そもそもこの変更によりAPIからの更新でwipの状態が変わることがなくなったため、削除しています。

junohm410 · 2024-02-27T01:40:03Z

app/controllers/questions_controller.rb

-    questions =
-      case params[:target]
-      when 'solved'
-        Question.solved
-      when 'not_solved'
-        Question.not_solved.not_wip
-      else
-        Question.all
-      end
+    questions = Question.by_target(params[:target])


【行数オーバーのコントローラのリファクタリング】
ここだけではありませんが、showとupdateアクションをコントローラに追加したため、クラスの行数が100行を超えRubocopに指摘されるようになったため、既存の処理の一部をリファクタリングし、行数を減らしています。

※厳密にはこのIssueの趣旨に関連する変更ではありませんが、Rubocopに新たな除外項目を増やすのはNGという方針のため、このようなリファクタリングも当PRに含めています。

コントローラ上でcase whenの条件分岐のロジックまで担当するのは責務的に重いのと、solvedなどのtargetによる絞り込みは汎用性があると考えられるので、この部分はモデルのスコープに移しています。

junohm410 · 2024-02-27T01:42:22Z

app/controllers/questions_controller.rb

    if @question.save
      Newspaper.publish(:question_create, { question: @question })
-      redirect_to @question, notice: notice_message(@question)
+      redirect_to Redirection.determin_url(self, @question), notice: @question.generate_notice_message(:create)


Redirection.determin_urlは新規作成時・更新時のリダイレクト先を切り替えるクラスメソッドです。
下記のPRでIssueで実装されています。
#6741

junohm410 · 2024-02-27T01:43:29Z

app/controllers/questions_controller.rb

+  def update
+    set_wip
+    if @question.update(question_params)
+      Newspaper.publish(:question_update, { question: @question }) if @question.saved_change_to_wip?
+      redirect_to Redirection.determin_url(self, @question), notice: @question.generate_notice_message(:update)
+    else
+      render :edit
+    end
+  end
+


APIで行っていた更新処理を、通常のRailsのコントローラに移しています。
機能的にはAPIでやっていたものと変わりません。

junohm410 · 2024-02-27T01:45:38Z

app/controllers/questions_controller.rb

-  def questions_property
-    case params[:target]
-    when 'solved'
-      QuestionsProperty.new('解決済みのQ&A', '解決済みのQ&Aはありません。')
-    when 'not_solved'
-      QuestionsProperty.new('未解決のQ&A', '未解決のQ&Aはありません。')
-    else
-      QuestionsProperty.new('全てのQ&A', 'Q&Aはありません。')
-    end
-  end


【行数オーバーのコントローラのリファクタリング】
特定のQuestionモデルのインスタンスに紐づく処理はないけれど、質問という関心ごとには確実に関連している処理のため、Questionのインスタンスメソッドではなくクラスメソッドに移しました。

junohm410 · 2024-02-27T02:09:57Z

app/views/questions/show.html.slim

-      div(data-vue="QuestionPage" data-vue-current-user-id:number="#{current_user.id}" data-vue-question-id="#{@question.id}")
+      .question.page-content
+        = render 'question_header', question: @question
+        = render 'question_body', question: @question
+      div(data-vue="QuestionAnswers" data-vue-current-user-id:number="#{current_user.id}" data-vue-question-id="#{@question.id}")


非Vue化で、質問本文の部分のうち、下の機能を各パーシャルに移しています。

ユーザーアイコン・Watchボタン・ブックマークボタン・タグ -> question_header

質問本文・編集ページへのリンク（「内容修正」）・絵文字リアクション -> question_body

残った答え部分はQuestionAnsewersというVueコンポーネントにリネームし、ここから繋げています。

junohm410 · 2024-02-27T02:11:42Z

test/integration/api/questions_test.rb

-    [
-      @question.user.login_name,
-      @non_editable_user_login_name,
-      User.find_by(admin: true).login_name
-    ].each do |name|
-      changed_title = "#{name} changed"
-      token = create_token(name, 'testtest')
-      patch @path,
-            params: { question: { title: changed_title } },
-            headers: { 'Authorization' => "Bearer #{token}" }
-
-      assert_response :ok
-      assert_equal changed_title, @question.reload.title
-    end
+    token = create_token('hajime', 'testtest')
+    patch @path,
+          params: { question: { tag_list: '新規タグ1,新規タグ2' } },
+          headers: { 'Authorization' => "Bearer #{token}" }
+    assert_response :ok
+    assert_equal %w[新規タグ1 新規タグ2], @question.reload.tag_list


質問編集APIのコントローラテストの部分です。
本PRにより、APIのupdateアクションにはタグを編集する機能だけが残るため、テストもそのように修正しています。

junohm410 · 2024-02-27T02:12:40Z

test/models/question_test.rb

+
+  test '.by_target' do
+    solved_question = questions(:question3)
+    not_solved_question = questions(:question1)
+    assert_includes Question.by_target('solved'), solved_question
+    assert_not_includes Question.by_target('solved'), not_solved_question
+
+    assert_includes Question.by_target('not_solved'), not_solved_question
+    assert_not_includes Question.by_target('not_solved'), solved_question
+
+    assert_includes Question.by_target(nil), solved_question
+    assert_includes Question.by_target(nil), not_solved_question
+  end


【行数オーバーのコントローラのリファクタリング】
コントローラからモデルに移したスコープ・メソッドのテストを追加しています。

junohm410 · 2024-02-27T02:14:55Z

test/system/notification/questions_test.rb

-    click_button '内容修正'
+    click_link '内容修正'


非Vue化により、質問を編集する「内容修正」ボタンでRailsのlink_toを使用するようになったため、タグがbuttonではなくaタグに変わっています。
（pagesやreportsのページでも「内容修正」ボタンはaタグで実装されており、それにあわせています）

この変更により、このボタンをクリックする多くのシステムテストが落ちるようになったため修正しています。

junohm410 · 2024-02-27T02:18:53Z

test/system/questions_test.rb

-
-  test 'using file uploading by file selection dialogue in textarea at editing question' do
-    question = questions(:question3)
-    visit_with_auth "/questions/#{question.id}", 'komagata'
-    click_button '内容修正'
-
-    element = first('.a-file-insert')
-    within element do
-      assert_selector 'input.js-question-file-input', visible: false
-    end
-    assert_equal '.js-question-file-input', find('textarea.a-text-input')['data-input']
-  end


このテストは、Vueでの質問編集画面上のテキストエリアでファイルアップロードが機能することを確かめる内容になっています。

Vueによる質問編集画面がなくなったこと

Railsのformパーシャル上のテキストエリアにおけるこの機能のテストは、一つ上の'using file uploading by file selection dialogue in textarea'テストで行われていること

この2点を考慮し、本テストは削除しました。

junohm410 · 2024-02-27T08:00:21Z

test/system/notification/questions_test.rb

-    assert_text '質問を更新しました'
+    assert_text '質問をWIPとして保存しました。'


フラッシュメッセージについて、JSではなくRailsのみで一貫して処理することになったので、その都合で一部のシナリオのメッセージの文言が変わりました。

フラッシュメッセージを使っているテストが落ちるようになったものは、このような形で修正しています。
文言変更の詳細については、Issueのやりとりをご確認ください。
#7356 (comment)

junohm410 · 2024-02-27T08:07:41Z

@SuzukaHori
お疲れ様です。こちらのレビューをお願いしたくご連絡させていただきました🙏
FileChangedの数が多くて申し訳ないのですが、そのぶん事前にコードの説明を多めにコメントさせていただきましたので、そちらを参照していただければ幸いです🙇‍♂️
Discordの通話でペアレビューとかも全然大丈夫なので、必要であればおっしゃっていただけるとありがたいです。
お手数をおかけしますが、可能であればよろしくおねがいいたします🙇‍♂️

SuzukaHori · 2024-02-27T10:52:12Z

@junohm410
こんにちは、レビュー承知しました👍
明日から取り組みますが、vueの実装をやったことがないのでかなりお時間いただくかもしれません🙇‍♀️
わからない際には質問させていただきます。よろしくお願いします！

SuzukaHori

お疲れさまです！
大変待たせいたしました🙇‍♀️

質問更新・フラッシュの動作を確認しました。バッチリでした👍
その他QA一覧・コメント・タグ・回答数の表示などで一連の操作を確認しましたが、正常に動作しているように見えました。
全体的な設計やコードについて
問題ないように見えました！
丁寧にコメントをつけていただき、大変助かりました。ありがとうございます🙏

以下は感想です：

question-edit.vueの置き換えについて
question/show内の構造について、日報ページなどと一致してDRYになっており、良いと思いました。
question/editのフラッシュメッセージについても、適切に条件分岐されているとように見えました。
Answer部分を単独で動かすための修正について
vueに詳しくないので自信がないのですが、3回読んでも特に問題はなさそうに見えました。
ファットコントローラー解消のためのリファクタリングについて
コントローラー→モデルへの移行がいい感じで行われており、テストも追加されていて良いと思いました。

とても細かい部分で、2点ほどコメントしておりますのでご確認をお願いします！（うち一点は単純な疑問です🙇‍♀️）

SuzukaHori · 2024-02-29T03:32:36Z

app/views/questions/_question_header.html.slim

+
+    .page-content-header__row
+      .page-content-header__tags
+        = react_component('Tags/Tags', { tagsInitialValue: question.tag_list.join(',').to_s, tagsParamName: 'question[tag_list]', tagsInputId: 'question_tag_list', tagsType: 'Question', tagsTypeId: question.id.to_s })


1つ目のto_sが不要に思えたのですが、いかがでしょうか？？
joinの時点で文字列になっているように見えます👀

irb(main):018:0> Question.last.tag_list.join(',') => "ruby,rail,Linux" irb(main):018:0> Question.last.tag_list.join(',').class => String

2つ目のto_sについても、idを文字列に変換しているのを不思議に感じたので、理由を伺いたいです🙇‍♀️

ありがとうございます！こちらの引数の渡し方についてはpagesの実装を真似ていたところだったので、細かく見られていないところでした🙇‍♂️お恥ずかしい限りです。

Reactコンポーネントの部分を含めて、あらためて確認しました。

1つ目のto_sが不要に思えたのですが、いかがでしょうか？？
joinの時点で文字列になっているように見えます👀

こちらはおっしゃる通りだと思います。
tagsInitialValueはその先のコンポーネントでも文字列であることを期待されていますが、Rubyのメソッドチェーンのjoinの時点で確実に文字列が返ってくるので、このto_sは不要です。

2つ目のto_sについても、idを文字列に変換しているのを不思議に感じたので、理由を伺いたいです🙇‍♀️

結論的には、こちらのto_sはあってもいいと考えました。

bootcamp/app/javascript/components/Tags/Tags.jsx

Line 68 in 6dcc837

fetch(`/api/${tagsType.toLowerCase()}s/${tagsTypeId}`, {

ここのtagsTypeIdpropsは、Tags/Tagsコンポーネントの中で、APIのエンドポイントを指定するのに使われています。
ここでtagsTypeIdが数値型だった場合、暗黙の型変換による文字列結合が起こります。

数値型でもエラーにはならないですが、文字列のエンドポイントを生成する用途と決まっているのであれば、propsで渡すタイミングで確実にto_sで文字列にしておいてもいいのかなと考えました👀

なので、tagsInitialValueの方のto_sのみ外す、という修正がいいかなと考えていますが、いかがでしょうか？
その他の修正の確認含め、ご確認いただけるとありがたいです🙏

ご説明ありがとうございます！

tagsTypeIdに関して、コンポーネント内で文字列型以外で使う可能性がないのなら、こちらで変換を行なって良いと思いました👍
tagsInitialValueの方のto_sのみ外す方向で、修正をお願いします🙏

ご確認ありがとうございます🙏tagsInitialValueの方のto_sのみ外す修正を行いました。

SuzukaHori · 2024-02-29T06:38:24Z

test/system/questions_test.rb

    within 'form[name=question]' do
      fill_in 'question[title]', with: 'テストの質問（修正）'
      fill_in 'question[description]', with: 'テストの質問です。（修正）'
      find('.choices__inner').click
-      find('#choices--js-choices-single-select-item-choice-12', text: 'sshdでパスワード認証を禁止にする').click
+      find('#choices--js-choices-single-select-item-choice-11', text: 'sshdでパスワード認証を禁止にする').click


元の「12」だとテストが失敗＆「11」にすると成功するのは確認したのですが、なぜこうなるのかわからなかったため、教えていただきたいです🙇‍♀️

test/fixtures/practices.ymlで、「sshdでパスワード認証を禁止にする」は12番目のデータに見えるので、不思議に思いました。（見る場所が違っていたらすみません🙇‍♀️）

こちらの説明が足りていませんでした、すみませんでした🙇‍♂️

もともと11に修正していたのは、更新時のフォームから「プラクティス選択なし」という選択肢がなくなったからでした。
（既存の_formにその選択肢がなく、Vue上の編集画面にのみ独自でその選択肢が実装されていた）

先頭の要素'#choices--js-choices-single-select-item-choice-1'がこの「プラクティス選択なし」で、それが消えたので、12から11に順番が変わった、というイメージです。

ただ、そもそも「更新画面から『プラクティス選択なし』を消す」のではなく、「新規画面にも同じ選択肢を追加する」という方向がいいような気がしたので、machidaさんに提案の上、実装の方針を変更しました。
#7356 (comment)
PRの概要欄にも説明を足しております。

よってこの部分はもとの「12」に戻しています🙇‍♂️
代わりに他のテストがまた影響を受けているのですが、その部分は別途コメントを残しておきます。

test/fixtures/practices.ymlで、「sshdでパスワード認証を禁止にする」は12番目のデータに見えるので、不思議に思いました。（見る場所が違っていたらすみません🙇‍♀️）

こちらですが、プラクティスの並びは必ずしもtest/fixtures/practices.ymlの並びとは一致しない感じのようですね👀
kimuraの所属するコース「Railsプログラマー」には「Mac OS X」などのカテゴリがあり、コースの中のカテゴリの順番がまず決まっていて、その中で個別のプラクティスが並んでいく、のような構造になっています。

たとえばtest/fixtures/practices.ymlの7・8番目にある「vi」に関するプラクティスですが、これは「Vim」というカテゴリに所属し、このカテゴリは「Railsプログラマー」の中ではそこそこ後ろの方のため、「sshdでパスワード認証を禁止にする」よりも後に並びます。

なので、fixturesの上から12番目にあった「sshdでパスワード認証を禁止にする」がプラクティス並びでも12番目にあったのは、上記のようなことが積み重なった上でのたまたまということになりそうです😅

テストについて

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

kimuraの所属するコース「Railsプログラマー」には「Mac OS X」などのカテゴリがあり、コースの中のカテゴリの順番がまず決まっていて、その中で個別のプラクティスが並んでいく、のような構造になっています。

なるほど、並び順はカテゴリの順番の影響を受けていて、practices.ymlとは一致しないのですね😳
ご丁寧にありがとうございます、大変勉強になりました！！

「プラクティス選択なし」の追加について

修正ありがとうございます！
新規作成時・更新時ともに「プラクティス選択なし」を選べることを確認しました👍

Docsの「関連プラクティスなし」にはデザインが入っているように見えるので、一応町田さんに確認すると良いかもと思いました👀

細かいところまでチェックいただきありがとうございます！

デザインの入り方の違いですが、使用するセレクトボックス拡張のライブラリのデフォルト状態をそのまま使うかどうか、という問題な気がしたので、そのような方向でmachidaさんに相談しました👍
#7439 (comment)

結論としては現状のままでOK（Docsの方で使うライブラリをQ&Aに合わせる）ということになりましたので、こちらへの修正は特に加えていません🙏

junohm410

1度目のレビュー後の修正部分の補足コメントです。

junohm410 · 2024-03-02T06:18:56Z

app/controllers/questions_controller.rb

+    @questions = Question
+                 .by_target(params[:target])
+                 .by_practice_id(params[:practice_id])
+                 .by_tag(params[:tag])
                 .with_avatar
                 .includes(:practice, :answers, :tags, :correct_answer)
-                 .order(updated_at: :desc, id: :desc)
+                 .recent


【QuestionsControllerのリファクタリング】
by_target以外にも、paramsから受け取るpractice_id・タグ名に基づく条件分岐を伴った検索や、直近の質問を並べるクエリメソッドをモデルのスコープに移しました。

また、既存のコードは@questionsをセットする処理の途中でタグの取得などを行なっていたりしましたが、このスコープへの切り出しにより、@questionsのセットを一つのメソッドチェーンにまとめられるようになりました。

移したモデルのスコープのテストも追加しております。

junohm410 · 2024-03-02T06:20:58Z

app/models/concerns/mentioner.rb

    when Question
-      "#{user.login_name}さんのQ&A「#{practice[:title]}」"
+      practice_title = practice ? practice[:title] : 'プラクティス選択なし'
+      "#{user.login_name}さんのQ&A「#{practice_title}」"
    end


「プラクティスを選択しない状態かつ、他のユーザーにメンションをした状態でQ&Aを新規作成・更新するとエラーになる」現象への対応です。
#7356 (comment)

junohm410 · 2024-03-02T06:29:06Z

app/views/questions/_form.html.slim

-                = f.select(:practice_id, practice_options_within_course, { selected: params[:practice_id] }, { id: 'js-choices-single-select' })
+                - if question.new_record?
+                  = f.select(:practice_id, practice_options_within_course, { selected: params[:practice_id], include_blank: 'プラクティス選択なし' }, { id: 'js-choices-single-select' })
+                - else
+                  = f.select(:practice_id, practice_options_within_course, { include_blank: 'プラクティス選択なし' }, { id: 'js-choices-single-select' })


question.new_record?で分岐しているのは、「プラクティスページの『質問する』ボタンから/questions/new?practice_id=...に飛んだ時に、自動でプラクティスが選択されている」という機能を維持するためです。

当該機能のテストは以下です。

bootcamp/test/system/questions_test.rb

Lines 201 to 205 in 6dcc837

test 'select practice title when push question button on practice page' do

visit_with_auth "/practices/#{practices(:practice23).id}", 'hatsuno'

click_on '質問する'

assert_text 'rubyをインストールする'

end

フォームに紐づくモデルオブジェクトが新規のものである場合は、pracrtice_idクエリパラメータがあればそれをもとにプラクティスがセットされ、そうでなければ「プラクティス選択なし」がセットされるようになっています。

更新時は上記のことをケアすることがないため、「プラクティス選択なし」が選択肢の中に用意されることのみ設定しています。
#7356 (comment)

junohm410 · 2024-03-02T06:31:47Z

test/system/questions_test.rb

-    find('#choices--js-choices-single-select-item-choice-6', text: 'Linuxのファイル操作の基礎を覚える').click
+    find('#choices--js-choices-single-select-item-choice-7', text: 'Linuxのファイル操作の基礎を覚える').click


新規作成フォームのプラクティス選択部分の先頭に「プラクティス選択なし」を追加したことにより、既存のプラクティスの並びが一つ後ろにずれました。そのことによる修正です。

junohm410 · 2024-03-03T04:43:37Z

@SuzukaHori
すみません、昨日先に色々とコメントしましたが、思ったよりCIがなかなか通らない状況です。
通り次第改めてご連絡しますので、それまでは見ていただかなくて大丈夫です（いくつか追加対応した部分を概要に書いていますので、そこだけ見てもらっているとあとでコードを見ていただく時にスムーズかもですが）🙏
レビューをセルフアサインいただいていたので、念のためご連絡です〜🙇‍♂️

SuzukaHori · 2024-03-03T11:13:42Z

@junohm410
ご連絡ありがとうございます🙏CIの件承知しました！
まだきちんとコードを見れていない状態なので、全く問題ありません😅（Fjord Choiceにレビュー状態がうまく反映されていなかったので、self-requestedしました）

ご連絡お待ちしております！

junohm410 · 2024-03-03T13:44:10Z

@SuzukaHori
CIが通りましたので、また明日以降にお手隙のタイミングでご確認いただけると幸いです🙏
ご不明点があればお気軽に聞いていただければと思います🙏よろしくお願いいたします🙇‍♂️

SuzukaHori

修正お疲れさまです、お待たせしました！

再度コードと動作を確認し、「プラクティス選択なし」が選択できること、「プラクティスを選択しない状態かつメンション時」にバグが発生しないことを確認しました👍

3箇所コメントを追加しましたので、お手隙の際にご確認ください🙇‍♀️
今回も丁寧にコメントをつけていただき、とても助かりました😭🙏

SuzukaHori · 2024-03-05T01:56:48Z

app/controllers/questions_controller.rb

@@ -31,7 +31,7 @@ def show
                          .where(practice: @question.practice)
                          .where.not(id: @question.id)
                          .includes(:correct_answer)
-                          .order(updated_at: :desc, id: :desc)
+                          .recent


recentの名前を見た時、このサイトのような「最新○件のデータを持ってくる」メソッドを想像しました。

リファクタリングに影響がないのならコントローラーにそのまま書くか、メソッド名を具体的に修正するといいかなと思ったのですが、いかがでしょうか？？

おっしゃる通りで、recentだと検索&取得のスコープと勘違いしますね😅勉強になります。
取得ではなく並び替えのスコープであることと、更新順であることの2点を具体化させるべきだと思いましたので、latest_update_orderに変えました。
（indexとshowで全く同じ並び替えをしているので、DRY原則に則ってメソッドに切り出す方針はそのままにしました）

SuzukaHori · 2024-03-05T06:22:53Z

app/views/questions/_question_header.html.slim

+
+    .page-content-header__row
+      .page-content-header__tags
+        = react_component('Tags/Tags', { tagsInitialValue: question.tag_list.join(',').to_s, tagsParamName: 'question[tag_list]', tagsInputId: 'question_tag_list', tagsType: 'Question', tagsTypeId: question.id.to_s })


ご説明ありがとうございます！

tagsTypeIdに関して、コンポーネント内で文字列型以外で使う可能性がないのなら、こちらで変換を行なって良いと思いました👍
tagsInitialValueの方のto_sのみ外す方向で、修正をお願いします🙏

SuzukaHori · 2024-03-05T06:39:28Z

test/system/questions_test.rb

    within 'form[name=question]' do
      fill_in 'question[title]', with: 'テストの質問（修正）'
      fill_in 'question[description]', with: 'テストの質問です。（修正）'
      find('.choices__inner').click
-      find('#choices--js-choices-single-select-item-choice-12', text: 'sshdでパスワード認証を禁止にする').click
+      find('#choices--js-choices-single-select-item-choice-11', text: 'sshdでパスワード認証を禁止にする').click


テストについて

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

kimuraの所属するコース「Railsプログラマー」には「Mac OS X」などのカテゴリがあり、コースの中のカテゴリの順番がまず決まっていて、その中で個別のプラクティスが並んでいく、のような構造になっています。

なるほど、並び順はカテゴリの順番の影響を受けていて、practices.ymlとは一致しないのですね😳
ご丁寧にありがとうございます、大変勉強になりました！！

「プラクティス選択なし」の追加について

修正ありがとうございます！
新規作成時・更新時ともに「プラクティス選択なし」を選べることを確認しました👍

Docsの「関連プラクティスなし」にはデザインが入っているように見えるので、一応町田さんに確認すると良いかもと思いました👀

junohm410 · 2024-03-05T07:56:34Z

@machida
お疲れ様です。
質問作成において「プラクティス選択なし」を選択できるようにする件で、一点ご相談させてください。

レビューでご指摘いただいた問題

「選択なし」の選択肢のスタイルが、DocsとQ&Aで異なる。
#7439 (comment)

詳細

現在同様の機能を持っているDocsでは、「関連プラクティスを指定しない」の選択肢にスタイルが当たっています。

テキストカラーが他の選択肢と同様の黒
現在選ばれている場合、別の選択肢を選んでいる間も灰色でハイライトされている

今回実装したQ&Aでの「プラクティス選択なし」では、以下のようなスタイルになっています。

テキストカラーはうっすら灰色
現在選ばれている場合でも、別の選択肢に写ろうとするとハイライトされなくなる（なおこれは「プラクティス選択なし」以外の他の選択肢でも同じで、現在の本番環境でもそのような挙動を取る）

DocsとQ&Aで、「選択なし」状態を実装するロジックは同じです。
Railsのformヘルパーで、include_blankというオプションを設定することで、無選択の場合の選択肢とそのラベルを用意しています。

# Docs
= f.select(:practice_id, practice_options(categories), { include_blank: '関連プラクティスを指定しない' }, { class: 'js-select2' })

# Q&A
= f.select(:practice_id, practice_options_within_course, { include_blank: 'プラクティス選択なし' }, { id: 'js-choices-single-select' })

原因

DocsとQ&Aで異なるのは、セレクトボックスを拡張するライブラリの違いです。
Docsはselect2、Q&AはChoices-jsを使用しています。

Q&Aはもともとselect2で実装されていたましたが、下記のPRでChoices-jsに置き換えられたようです。

試しにQ＆Aの方でもselect2で実装したところ、スタイルの当たり方はDocと全く同じなりました。
Docsのようにスタイルがあたるのは、select2とChoices-jsでデフォルトで用意されるクラスにあたるスタイルの差だと思われます。
（Choices-jsで無選択の場合の選択肢を用意するのが今回初めてなので、まだスタイルが用意されてない？）

対応について、私としての意見

対応ですが、私個人として今の状態でもわかりやすいかな？とは思っています。
（またDocsに合わせるべきというよりは、Docsだけが古いライブラリを使っていることがそもそもの問題なような気もします）

個別にスタイルを当てることも可能だとは思いますので、まずは上記をご確認いただき、もし「プラクティス選択なし」にスタイルを当てる必要があれば、そのご対応をお願いしたく思います🙏

ご確認のほど、何卒よろしくお願いいたします🙏

machida · 2024-03-05T18:15:05Z

@junohm410 なるほどー、今回はchoices.jsの見た目のままでお願いします🙏select2をchoices.jsに移行するというのも別Issueで対応したいと思います。こちらのIssue作成をお願いしてもいいでしょうか？

…プに移す

…モデルに移動

junohm410 · 2024-03-06T01:32:40Z

@machida
早々にご確認いただき、ありがとうございました🙇‍♂️
それでは、本PRではChoices.jsのデフォルトのままでいきたいと思います。

select2をchoices.jsに移行するというのも別Issueで対応したいと思います。こちらのIssue作成をお願いしてもいいでしょうか？

[Choices.js] Docs作成フォームのプラクティス選択をChoices.jsに置き換える。 #7498

Issueを作成しておきました👍

junohm410 · 2024-03-06T10:05:03Z

@SuzukaHori
お疲れ様です。レビューいただきありがとうございました🙏
修正・コメント返信させていただきましたので、改めてご確認のほど、よろしくお願いいたします🙏

SuzukaHori

お疲れさまです！

修正内容確認しました😊 Issueの作成含め、ありがとうございました🙏
問題ないと思いますので、私からはApproveさせていただきます✨

レビュー中色々と教えていただき、ありがとうございました！shodanさんの調査方法や、コミュニケーションの取り方など、大変勉強になりました🙇‍♀️🙇‍♀️

junohm410 · 2024-03-06T11:47:37Z

@SuzukaHori
ご確認いただき、ありがとうございました🙏
こちらこそ、細かいところまでチェックいただき大変勉強になりました🙇‍♂️感謝です。

@komagata
チームメンバーからApproveいただきましたので、レビューのほどよろしくお願いいたします🙏

komagata

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

junohm410 mentioned this pull request Feb 26, 2024

bookmark-button.vueをreact対応にする #5114

Closed

junohm410 commented Feb 27, 2024

View reviewed changes

junohm410 marked this pull request as ready for review February 27, 2024 08:02

junohm410 requested a review from SuzukaHori February 27, 2024 08:03

junohm410 self-assigned this Feb 28, 2024

SuzukaHori reviewed Mar 1, 2024

View reviewed changes

junohm410 force-pushed the chore/convert-question-section-from-vue-to-slim branch from 3310ec0 to 9b8fbe4 Compare March 2, 2024 04:30

junohm410 commented Mar 2, 2024

View reviewed changes

junohm410 force-pushed the chore/convert-question-section-from-vue-to-slim branch from 9b8fbe4 to fe9cf3e Compare March 2, 2024 07:51

SuzukaHori self-requested a review March 3, 2024 02:06

SuzukaHori reviewed Mar 5, 2024

View reviewed changes

junohm410 added 11 commits March 6, 2024 09:58

questionパーシャルビューテンプレートを追加し、questions/showから読み込むようにする

9ea657c

質問の編集ページ用のviewテンプレートを追加

b038e94

questions_controller.rbに、APIで処理していたedit/updateアクションを追加し、ルーティングを修正

db9e4ef

QuestionPageコンポーネントからAI・投稿された答え部分を分離し、リネームして独立したコンポーネントに変更

f195d61

質問部分を分離したcomponents/answers.vueが、単体でベストアンサー/回答数の更新・ローディング画面の表示が行えるように修正

372b124

質問の作成・更新・WIP化後のフラッシュメッセージについて、コンテキストに合わせて選択する処理をメソッド化

fc38ecf

質問をWIPで保存した際はeditページにリダイレクトするように修正

f8c5e4b

質問編集ページのsubmitボタン上の表記を、Vueで編集機能を実装していた際の表記と同じになるように修正

a73099b

Questionページの実装の変更（Vue -> Railsのview）で通らなくなったシステムテストの修正

9768392

保存時のページ遷移先を決める処理で、独自のメソッドを使わずRedirectionクラスのメソッドを使用するように修正

aaa4f37

QuestionsControllerリファクタリング: モデルの関連を使って行数を減らす

c1eb503

junohm410 added 15 commits March 6, 2024 09:58

QuestionsControllerリファクタリング: クエリパラメータtargetによる条件分岐・オブジェクト検索処理をモデルのスコー…

6ae2395

…プに移す

QuestionsControllerリファクタリング: フラッシュメッセージとビューでのタイトル・0件時のメッセージを生成するメソッドを…

e8305ea

…モデルに移動

質問の作成・更新時のwipカラムを変更する処理を一つにまとめる

0b219e3

質問更新のAPIの処理から、編集機能をVueで実装しなくなったことで不要になった処理を省いた

43f3b8f

questions/showで表示する要素の中で、ヘッダーにあたる部分をパーシャルに切り出す

fcb91fd

個別の質問ページの中で読み込まれる質問本体のパーシャルのファイル名を、他のリソースを参考に_question_bodyへ変更

14473a8

VueのQuestionEditコンポーネントを削除

4b15ec9

Flakyになった質問ページのタグ編集に関するテストの修正

b4f817f

子コンポーネントとの名前の衝突を避けるため、質問ページの答え部分の親のVueファイルをリネーム

0c6ca8b

質問編集画面遷移時へのに、現在のプラクティスが自動でセットされるように修正

e39e67a

新規質問作成時においても、「プラクティス選択なし」を選べるように修正

0e032c8

質問の作成・更新時にプラクティスを選択しない状態で本文でメンションをすると発生するバグを修正

36273e1

QuestionsControllerリファクタリング: index/showで呼んでいたクエリメソッドの一部をモデルのscopeに移す

7d8f268

タグのReactコンポーネントにpropsを渡す際の不要な文字列変換メソッドの使用を止める

80a0a5c

最新の更新順に質問を並びかえるスコープの名前を具体的なものに修正

ff86b95

junohm410 force-pushed the chore/convert-question-section-from-vue-to-slim branch from fe9cf3e to ff86b95 Compare March 6, 2024 00:59

junohm410 mentioned this pull request Mar 6, 2024

[Choices.js] Docs作成フォームのプラクティス選択をChoices.jsに置き換える。 #7498

Closed

junohm410 requested a review from SuzukaHori March 6, 2024 10:05

SuzukaHori approved these changes Mar 6, 2024

View reviewed changes

junohm410 requested a review from komagata March 6, 2024 11:48

komagata approved these changes Mar 9, 2024

View reviewed changes

komagata merged commit ac2b8a2 into main Mar 9, 2024
8 checks passed

komagata deleted the chore/convert-question-section-from-vue-to-slim branch March 9, 2024 17:06

github-actions bot mentioned this pull request Mar 9, 2024

Release 2024-03-15 10:38:54 #7507

Merged

29 tasks

reckyy mentioned this pull request Aug 25, 2024

answer.vue, answers.vueを非vue化する #5116

Open

		assert_text '質問を更新しました'
		assert_text '質問をWIPとして保存しました。'

	test 'select practice title when push question button on practice page' do
	visit_with_auth "/practices/#{practices(:practice23).id}", 'hatsuno'
	click_on '質問する'
	assert_text 'rubyをインストールする'
	end

		find('#choices--js-choices-single-select-item-choice-6', text: 'Linuxのファイル操作の基礎を覚える').click
		find('#choices--js-choices-single-select-item-choice-7', text: 'Linuxのファイル操作の基礎を覚える').click

Q&AページのQuestion部分を非Vue化した #7439

Q&AページのQuestion部分を非Vue化した #7439

Conversation

junohm410 commented Feb 26, 2024 • edited Loading

Issue

概要

質問作成・更新時のフラッシュメッセージの整理

質問作成時にも「プラクティス選択なし」を選べるようにする

「プラクティス選択なし」の状態かつ質問本文でユーザーがメンションをされると、メンション通知のタイトルで使われるプラクティス名部分は「プラクティス選択なし」になる

作業内容のおおまかなイメージ

変更前のQ&AページのQuestion部分の構成

変更後のQ&AページのQuestion部分の構成

変更確認方法

Screenshot

変更前

変更後

junohm410 left a 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

junohm410 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junohm410 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junohm410 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

junohm410 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

junohm410 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

junohm410 commented Feb 27, 2024

SuzukaHori commented Feb 27, 2024

SuzukaHori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junohm410 Mar 2, 2024 • 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

junohm410 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

テストについて

「プラクティス選択なし」の追加について

Choose a reason for hiding this comment

junohm410 left a comment

Choose a reason for hiding this comment

junohm410 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junohm410 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junohm410 commented Mar 3, 2024 • edited Loading

SuzukaHori commented Mar 3, 2024 • edited Loading

junohm410 commented Mar 3, 2024

SuzukaHori left a 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

テストについて

「プラクティス選択なし」の追加について

junohm410 commented Mar 5, 2024 • edited Loading

レビューでご指摘いただいた問題

詳細

原因

対応について、私としての意見

machida commented Mar 5, 2024

junohm410 commented Mar 6, 2024

junohm410 commented Mar 6, 2024

SuzukaHori left a comment

Choose a reason for hiding this comment

junohm410 commented Mar 6, 2024

junohm410 commented Feb 26, 2024 •

edited

Loading

junohm410 Feb 27, 2024 •

edited

Loading

junohm410 Feb 27, 2024 •

edited

Loading

junohm410 Feb 27, 2024 •

edited

Loading

junohm410 Feb 27, 2024 •

edited

Loading

junohm410 Feb 27, 2024 •

edited

Loading

junohm410 Mar 2, 2024 •

edited

Loading

junohm410 Mar 2, 2024 •

edited

Loading

junohm410 Mar 2, 2024 •

edited

Loading

junohm410 Mar 2, 2024 •

edited

Loading

junohm410 commented Mar 3, 2024 •

edited

Loading

SuzukaHori commented Mar 3, 2024 •

edited

Loading

junohm410 commented Mar 5, 2024 •

edited

Loading