プラクティス > Docs一覧をVue.js化する #4293

aim2bpg · 2022-02-24T08:10:26Z

Issue: #4048

概要

プラクティス > Docs一覧をVue.js化して、非同期に読み込むようにしました。

参考PR

Refs: #3852 他、#3963,#3181,#2537,#2450,#2286,#2280,#2260 を参照しました。

変更点

差分が大量となるため、レビュー効率向上と自分の備忘録も兼ねて、概要を以下に、詳細を日報にまとめました。

Routing
- routes/api.rbにpractices/pages#indexアクションを追記。
Controller
- APIのコントローラapp/controllers/api/practices/pages_controller.rbを追加（既存のapp/controllers/practices/pages_controller.rbを流用し、メソッド名の先頭にAPI::を付加、継承元もAPI::BaseControllerに変更）
- 既存のapp/controllers/practices/pages_controller.rbより不要となった@pagesの代入部分を削除。
View
- RoutingとControllerで追加したAPIのindexアクションに対応させるため、app/views/api/practices/pages/index.json.jbuilderを追加（app/views/api/pages/index.json.jbuilderを完全流用）
- app/views/practices/pages/index.html.slimのVue.jsに置き換えたDocs一覧表示部分を削除し、Vue.jsのtemplateを埋め込むためのdiv要素を追記。
- Vue.jsに置き換えられて不要となったHTMLのpageパーシャルapp/views/pages/_page.html.slimを削除。
vue読込用JS
- app/javascript/packs/application.jsにコンパイル対象のJSファイルpractices-pages.jsを追記。
- vueファイルを読み込むためのJSファイルapp/javascript/practices-pages.jsを追加（同階層のpages.jsを流用）
- 上記ファイルに、既存のMenu > Docs一覧と表示処理を共通化させるために、practicesを親とするURLパスの一部を引数としてvueファイルに渡す記述を追記。
vue
- 読込用JSからpracticesを親とするURLパスの一部を受け取って、Menuからでもプラクティスからでも同じvueファイルを使って表示できるように共通化するための記述をapp/javascript/pages.vueに追記。
テスト
- APIのコントローラ用の機能テストtest/integration/api/practices_pages.test.rbを追加
以上の詳細情報は「日報」を参照ください。
- 【🐹 165日目｜システム開発】チーム開発(19) | FJORD BOOT CAMP（フィヨルドブートキャンプ）

2022-03-11 追記

本PRを参考とされる方はご注意ください。
本PRでは、すでにVue.js化されたトップページ > Docs一覧とコードをどう共通化させるかが課題でした。他のVue.js化にそのまま当てはまらないかもしれません。ただ、ファイルごとにどのような修正が入るのかイメージするのには参考となるかもしれません。
また、上記修正後のレビューで「APIを増やさずにDocsのAPIにpracticeでの絞り込み機能を追加した方が良い」との助言があり、作ったAPIを削る修正をしていますので、Files changedの最終形をいきなり作ったわけではなく、そういう修正の経緯があったことをご承知の上で参考とされてください。

動作確認

aim2bpg · 2022-02-25T07:35:57Z

@garammasala29 さん、おつかれさまです。
類似Issueに着手されるようですので、変更点のインプットも兼ねてレビューをお願いいたします🙏
なお、テストで1つエラーとなるところがあるのですが、別Issue #4298 を立ててレビュー依頼中です。
マージされると、解消する見込みです。ご承知おきください🙇🏻‍♂️

garammasala29

@aim2bpg さん
お疲れ様です。レビュー依頼ありがとうございました！大変遅くなりすみません🙇‍♂️
手元でもVue化されることを確認できました。

default.mov

issueを作成された箇所以外のテストが通ることも確認できています。
日報で実装の意図などを丁寧に書かれていたので、内容をよく理解しながらレビューすることができました。LGTMです！

1点だけ気になる点があり、app/controllers/pages_controller.rb#indexでVue化に伴い不要なコードが見受けられ、どうしたらいいかなと考えていました。
@aim2bpg さんの今回の変更差分と関係ない部分ではありますが、少し気になったので記しておきます。

aim2bpg · 2022-03-01T23:52:48Z

@garammasala29 さん、ご確認ありがとうございます🙇🏻‍♂️

1点だけ気になる点があり、app/controllers/pages_controller.rb#indexでVue化に伴い不要なコードが見受けられ、どうしたらいいかなと考えていました。

@practice = Practice.find(params[:practice_id])のことでしょうか？
こちらは、プラクティスのタイトルに関わる記述でしたので、削除するとエラーとなってしまいますので必要なコードという認識です。こちらのコードでない場合は、具体的に記載していただけますと幸いです😄

garammasala29 · 2022-03-02T00:42:53Z

@aim2bpg さん
お疲れ様です。
app/controllers/pages_controller.rbの@pages = Page.with_avatar~~に関してのお話でした、practicesから生えていないものになります
(変更したファイルではないので、対象のコードに直接コメントができませんでした。分かりづらくすみません🙏)

aim2bpg · 2022-03-02T02:15:33Z

@garammasala29 さん、ありがとうございます。
自分がファイルパスをよく見ておらずに誤解してました🙏

app/controllers/pages_controller.rbの@pages = Page.with_avatar~~

上記の記述は、本Issueとは別ページの部品と思われますのでスコープ外とさせて下さい。

手元で上記の記述を削除してみると、たしかにDocs一覧ページも表示できましたし、テストもPassしました。
一見すると不要と思われるのですが、影響範囲を調べずに安易に削除してしまうと、意図せず表示が壊れたりする恐れもあるので、そのままでもよいかと考えました。

aim2bpg · 2022-03-02T02:18:46Z

@komagata さん、お手すきの際にレビューをお願いいたします🙏
なお、テストで1つエラーとなるところがあるのですが、別Issue #4298 を立ててレビュー依頼中です。
マージされると解消する見込みです。別Issue #4298 と合わせてご確認をお願いいたします🙇🏻‍♂️

garammasala29 · 2022-03-02T03:09:58Z

@aim2bpg さん

上記の記述は、本Issueとは別ページの部品と思われますのでスコープ外とさせて下さい。

全然問題ありません、大丈夫です。気になる部分でしたので、共有させていただいてよかったです。
ご確認いただきありがとうございました🙏

aim2bpg · 2022-03-05T05:23:57Z

@machida さん
mainを取り込んだ際に、Vue.js化前後でテストでFailする点がもう1箇所あって、どちらの記述があるべき姿がわかりませんのでご教示いただきますでしょうかー

machida · 2022-03-06T14:49:34Z

@aim2bpg 返信遅れてすいません。meta が付く方でお願いします。

ちなみに、付かない方のclassは

こっちの大きい方ようのclassですね。

それが間違えて使われているっぽいです。

aim2bpg · 2022-03-06T21:53:37Z

@machida さん、metaの方で承知しましたー🙇🏻‍♂️
多分そうかなと思ったのですが、念の為にご確認させていただきました🙏

aim2bpg · 2022-03-06T22:44:18Z

@komagata さん、あらためましてレビュー依頼させていただきます〜🙏
Vue.js化後にテストが通らなくなった件 #4298 も解消しました。

komagata · 2022-03-07T14:01:00Z

app/controllers/api/practices/pages_controller.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class API::Practices::PagesController < API::BaseController


APIを増やすのではなく、DocsのAPIにpracticeでの絞り込み機能を追加することで対応できればと思います〜

https://github.com/fjordllc/bootcamp/blob/main/app/controllers/api/pages_controller.rb#L3

/api/pages?practice_id=1234のようなURLになるイメージ。

ご提案ありがとうございます。おかげさまで、コードがスッキリしました〜😄

aim2bpg · 2022-03-09T12:25:28Z

@komagata さん、#4293 (comment) の修正をしましたので、ご確認をお願いいたします〜🙏
あと、DocsのAPIテストが入れてなかったので、今回追加しておきました。

cafedomancer · 2022-03-09T12:42:13Z

app/javascript/pages.vue

@@ -27,6 +27,7 @@ export default {
    page: page
  },
  props: {
+    practiceId: { type: String, required: true },


通常の docs 一覧ページのほうの js のコードでは practiceId を props として渡していないと思うので、required: true にしちゃうとそっちで warning になっちゃう気がしました。このへんです。

bootcamp/app/javascript/pages.js

Line 12 in aa00f49

props: { selectedTag: selectedTag }

エラーが出ないからと、リファクタしたつもりが裏目に出てしまいました。
下記の元の記述に戻すことで、ワーニングが出ないことを確認しました。
practiceId: { type: String, default: '', required: false },

cafedomancer · 2022-03-09T12:46:55Z

app/controllers/api/pages_controller.rb

+    if params[:practice_id]
+      practice = Practice.find(params[:practice_id])
+      @pages = practice.pages.with_avatar
+                       .includes(:comments,
+                                 { last_updated_user: { avatar_attachment: :blob } })
+                       .order(updated_at: :desc)
+                       .page(params[:page])
+    else
+      @pages = Page.with_avatar
+                   .includes(:comments, :practice, :tags,
+                             { last_updated_user: { avatar_attachment: :blob } })
+                   .order(updated_at: :desc)
+                   .page(params[:page])
+      @pages = @pages.tagged_with(params[:tag]) if params[:tag]
+    end


params[:practice_id] があるときもないときもベースになるクエリはほとんど変わらないと思うので、プラクティスによる絞り込みだけ conditional にやるのが良いのではと思いました 💡 Kaminari の page による絞り込みは先にあって良いのかちょっとわからなかったので、そのへんは調査してみてもらえると〜 🙏

Suggested change

if params[:practice_id]

practice = Practice.find(params[:practice_id])

@pages = practice.pages.with_avatar

.includes(:comments,

{ last_updated_user: { avatar_attachment: :blob } })

.order(updated_at: :desc)

.page(params[:page])

else

@pages = Page.with_avatar

.includes(:comments, :practice, :tags,

{ last_updated_user: { avatar_attachment: :blob } })

.order(updated_at: :desc)

.page(params[:page])

@pages = @pages.tagged_with(params[:tag]) if params[:tag]

end

@pages = Page.with_avatar

.includes(:comments, :practice, :tags,

{ last_updated_user: { avatar_attachment: :blob } })

.order(updated_at: :desc)

.page(params[:page])

@pages = @pages.where(practice_id: params[:practice_id]) if params[:practice_id]

@pages = @pages.tagged_with(params[:tag]) if params[:tag]

end

こちらもご提案ありがとうございます🙏コードがスッキリしました😄
page絞り込みが先にあっても違和感なく動作することを確認しました。

cafedomancer · 2022-03-09T12:48:37Z

test/integration/api/pages_test.rb

+  test 'GET /api/pages.json' do
+    get api_pages_path(format: :json)
+    assert_response :unauthorized
+
+    token = create_token('kimura', 'testtest')
+    get api_pages_path(format: :json),
+        headers: { 'Authorization' => "Bearer #{token}" }
+    assert_response :ok
+  end


practice_id による絞り込みや tag による絞り込みのテストもあると better だと思うけど omakase です〜。

たしかに、practice_id による絞り込みは実装してますのでテスト追加しました。
tagの方は、Vue.js化されていませんので対象外としています。

今回の PR の範囲で言うとそうだと思うんですが、API 単体の仕様としてはタグでの絞り込みができる、かつここは API に対するテストを書く場所なので、仕様があるならそれを確認するテストもここにあるべきだと思いました。そしてこの機能は、通常の Docs の一覧のほうで使われていそう (タグ一覧からタグで絞り込める) です。ただ、この PR の作業範囲としてどこまでのテストの追加を含めるかというのはまた別の話なので、その点は引き続きおまかせです！

aim2bpg · 2022-03-09T13:55:18Z

@cafedomancer さん、ご提案ありがとうございました。
修正完了しましたので、ご確認いただけますと幸いです🙏

cafedomancer · 2022-03-10T09:08:41Z

test/integration/api/pages_test.rb

+  test 'GET /api/pages.json' do
+    get api_pages_path(format: :json)
+    assert_response :unauthorized
+
+    token = create_token('kimura', 'testtest')
+    get api_pages_path(format: :json),
+        headers: { 'Authorization' => "Bearer #{token}" }
+    assert_response :ok
+  end


今回の PR の範囲で言うとそうだと思うんですが、API 単体の仕様としてはタグでの絞り込みができる、かつここは API に対するテストを書く場所なので、仕様があるならそれを確認するテストもここにあるべきだと思いました。そしてこの機能は、通常の Docs の一覧のほうで使われていそう (タグ一覧からタグで絞り込める) です。ただ、この PR の作業範囲としてどこまでのテストの追加を含めるかというのはまた別の話なので、その点は引き続きおまかせです！

cafedomancer · 2022-03-10T09:12:16Z

test/integration/api/pages_test.rb

+  test 'GET /api/practices/315059988/pages.json' do
+    practices = practices(:practice1)
+    get api_pages_path(practices.id, format: :json)
+    assert_response :unauthorized
+
+    token = create_token('kimura', 'testtest')
+    get api_pages_path(practices.id, format: :json),
+        headers: { 'Authorization' => "Bearer #{token}" }
+    assert_response :ok
+  end


あれ、なんか test の説明がおかしそう...？ practice_id は query string に入るはず。

たしかに〜😇　/api/pages.json?practice_id=315059988に修正しました🙏

cafedomancer · 2022-03-10T09:13:57Z

app/javascript/pages.vue

@@ -27,6 +27,7 @@ export default {
    page: page
  },
  props: {
+    practiceId: { type: String, default: '', required: false },


default が空文字になっているのは、あんまり意味を成していないような気がしました。値が入っていないことを表現したいのであれば null か undefined を使う、もしくは props を渡さないときは practiceId がそのように設定されるという挙動に任せておく、のどちらかが良いと思います。

reports.vueでnullが使われていたので、pagesでもnullを使用しました。

aim2bpg · 2022-03-10T11:32:15Z

@cafedomancer さん、ご指摘ありがとうございました🙇🏻‍♂️
APIのDocs一覧でタグによる絞り込みについても、テスト追加しております。
お手すきの際に、ご確認をお願いいたします🙏

aim2bpg · 2022-03-10T12:04:07Z

@cafedomancer さん、ご確認ありがとうございました🙇🏻‍♂️ 初めてレビューしていただいて勉強になりました〜

@komagata さん、#4293 (comment) につきまして、修正しましたのでご確認をお願いいたします〜🙏

komagata

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

aim2bpg force-pushed the feature/convert-practices-docs-lists-to-vuejs branch from c794d70 to 64f77df Compare February 24, 2022 08:47

aim2bpg changed the title ~~プラクティス > Docs一覧をVue.js化した~~ プラクティス > Docs一覧をVue.js化する Feb 24, 2022

aim2bpg requested a review from garammasala29 February 25, 2022 07:32

aim2bpg marked this pull request as ready for review February 25, 2022 07:35

garammasala29 approved these changes Mar 1, 2022

View reviewed changes

aim2bpg requested a review from komagata March 2, 2022 02:16

garammasala29 mentioned this pull request Mar 4, 2022

ユーザー個別ページ > 質問一覧の vue.js化 #4343

Merged

aim2bpg force-pushed the feature/convert-practices-docs-lists-to-vuejs branch 2 times, most recently from 64accee to d294b83 Compare March 4, 2022 12:16

aim2bpg force-pushed the feature/convert-practices-docs-lists-to-vuejs branch from d294b83 to 7ad8732 Compare March 6, 2022 22:24

komagata requested changes Mar 7, 2022

View reviewed changes

aim2bpg added 6 commits March 9, 2022 21:05

プラクティス > Docs一覧をVue.js化した

f4af4db

Vue.js化で不要となったHTMLのpageパーシャルを削除

8f5203e

Vue.js化プラクティス > Docs一覧のVue.js化後にテストでエラーとなるバグを修正

b25c454

テスト不通によりMenu > Docs一覧側のVue.js化時のバグをもう1箇所修正

57232ff

Lintで指摘を受けたprettierの自動修正を実行

16e86a1

APIを増やすのではなく、DocsのAPIにpracticeでの絞り込み機能を追加する

cf46d07

aim2bpg force-pushed the feature/convert-practices-docs-lists-to-vuejs branch from c4947e8 to cf46d07 Compare March 9, 2022 12:06

cafedomancer reviewed Mar 9, 2022

View reviewed changes

コントローラのリファクタリング、vueワーニングの修正、APIのプラクティスID絞り込みテスト追加

63a1e09

cafedomancer reviewed Mar 10, 2022

View reviewed changes

vueファイルのpracticeIdのdefault値をnullに変更、APIのタグ絞り込みテストを追加

2e89106

cafedomancer approved these changes Mar 10, 2022

View reviewed changes

komagata approved these changes Mar 10, 2022

View reviewed changes

komagata merged commit 31249a3 into main Mar 10, 2022

komagata deleted the feature/convert-practices-docs-lists-to-vuejs branch March 10, 2022 18:16

github-actions bot mentioned this pull request Mar 10, 2022

Release 2022-03-18 01:41:12 #4380

Merged

32 tasks

eatplaynap mentioned this pull request Mar 18, 2022

ユーザー個別ページ > 回答一覧をVue.js化した #4319

Merged

tomonariha mentioned this pull request Jul 23, 2022

プラクティス個別 > Q&Aの一覧を Vue.js化した #5255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

プラクティス > Docs一覧をVue.js化する #4293

プラクティス > Docs一覧をVue.js化する #4293

aim2bpg commented Feb 24, 2022 •

edited

Loading

aim2bpg commented Feb 25, 2022 •

edited

Loading

garammasala29 left a comment

aim2bpg commented Mar 1, 2022

garammasala29 commented Mar 2, 2022

aim2bpg commented Mar 2, 2022

aim2bpg commented Mar 2, 2022

garammasala29 commented Mar 2, 2022

aim2bpg commented Mar 5, 2022

machida commented Mar 6, 2022

aim2bpg commented Mar 6, 2022

aim2bpg commented Mar 6, 2022

komagata Mar 7, 2022

aim2bpg Mar 9, 2022

aim2bpg commented Mar 9, 2022

cafedomancer Mar 9, 2022

aim2bpg Mar 9, 2022

cafedomancer Mar 9, 2022

aim2bpg Mar 9, 2022

cafedomancer Mar 9, 2022

aim2bpg Mar 9, 2022

cafedomancer Mar 10, 2022

aim2bpg commented Mar 9, 2022

cafedomancer Mar 10, 2022

cafedomancer Mar 10, 2022

aim2bpg Mar 10, 2022

cafedomancer Mar 10, 2022

aim2bpg Mar 10, 2022

aim2bpg commented Mar 10, 2022

aim2bpg commented Mar 10, 2022 •

edited

Loading

komagata left a comment •

edited

Loading

		@@ -0,0 +1,14 @@
		# frozen_string_literal: true

		class API::Practices::PagesController < API::BaseController

プラクティス > Docs一覧をVue.js化する #4293

プラクティス > Docs一覧をVue.js化する #4293

Conversation

aim2bpg commented Feb 24, 2022 • edited Loading

概要

参考PR

変更点

2022-03-11 追記

動作確認

aim2bpg commented Feb 25, 2022 • edited Loading

garammasala29 left a comment

Choose a reason for hiding this comment

aim2bpg commented Mar 1, 2022

garammasala29 commented Mar 2, 2022

aim2bpg commented Mar 2, 2022

aim2bpg commented Mar 2, 2022

garammasala29 commented Mar 2, 2022

aim2bpg commented Mar 5, 2022

machida commented Mar 6, 2022

aim2bpg commented Mar 6, 2022

aim2bpg commented Mar 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aim2bpg commented Mar 9, 2022

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

Choose a reason for hiding this comment

aim2bpg commented Mar 9, 2022

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

aim2bpg commented Mar 10, 2022

aim2bpg commented Mar 10, 2022 • edited Loading

komagata left a comment • edited Loading

Choose a reason for hiding this comment

aim2bpg commented Feb 24, 2022 •

edited

Loading

aim2bpg commented Feb 25, 2022 •

edited

Loading

aim2bpg commented Mar 10, 2022 •

edited

Loading

komagata left a comment •

edited

Loading