提出物一覧ページでの提出物の並び順を修正 #3228

konaga-k · 2021-09-05T11:03:01Z

概要

提出物一覧ページの提出物の並び順を以下のように修正

現在の提出物の並び順

画面	提出済みの提出物の並び順	WIPの提出物を含むか	WIPの提出物の並び順
全ての提出物	提出日順	含む	提出物の先頭
未完了の提出物	作成日順	含まない	-
未返信の提出物	提出日順	含まない	-
未アサインの提出物	作成日順	含まない	-
自分の担当の提出物	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
ログインユーザーの提出物（/current_user/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
ユーザーごとの提出物（/users/:user_id/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
企業所属ユーザーの提出物（companies/:company_id/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）

本PRでの提出物の並び順

画面	提出済みの提出物の並び順	WIPの提出物を含むか	WIPの提出物の並び順
全ての提出物	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
未完了の提出物	提出日順	含まない	-
未返信の提出物	提出日順	含まない	-
未アサインの提出物	提出日順	含まない	-
自分の担当の提出物	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
ログインユーザーの提出物（/current_user/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
ユーザーごとの提出物（/users/:user_id/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）
企業所属ユーザーの提出物（companies/:company_id/products）	作成日順	含む	作成日順（提出済み/WIP関係なく並べられる）

変更意図は以下の通り

未アサインタブだけ並び順を修正しても他のタブと不揃いになるため、まとめて整理する
WIPの提出物を含まない画面の場合、提出日順で並べる
WIPの提出物を含む画面の場合、直近のWIPの提出物も確認したいだろうから（特にログインユーザーの提出物画面）、作成日順で並べる。提出日順で並べるのは、WIPのものだけが先頭に来てしまうので止める

他に並び替えが必要であれば別issueで対応

UI

画面	before	after
全ての提出物
未完了の提出物
未アサインの提出物

konaga-k

@udaikue
お手すきでレビューお願いします🙏

konaga-k · 2021-09-05T11:58:54Z

app/models/product.rb

  scope :list, lambda {
    with_avatar
-      .preload([:practice, :comments, { checks: { user: { avatar_attachment: :blob } } }])
-      .order(created_at: :desc)
+      .preload(:practice,
+               :comments,
+               { user: :company },
+               { checks: { user: { avatar_attachment: :blob } } })
  }
-  scope :reorder_for_not_responded_products, -> { reorder(published_at: :desc, id: :desc) }
+  scope :order_for_list, -> { order(created_at: :desc, id: :desc) }
+  scope :order_for_not_wip_list, -> { order(published_at: :desc, id: :desc) }


https://github.com/fjordllc/bootcamp/pull/2280/files#r585987542
で、orderとorder以外のスコープは分離したいという意図を感じたので、以下のように変更しました

Product.list、Product.not_responded_productsからorderは削除

orderは別個のスコープorder_for_listとorder_for_not_wip_listとして切り出し

Product.list、Product.not_responded_productsを使っていた既存のコードで、並び替えが必要そうな箇所では order_for_listかorder_for_not_wip_listを呼び出すようにする

一通りコード検索した限り、 Product.list 、 Product.not_responded_products を使ってorderし忘れている箇所は無いと思います
一箇所だけorderを追加しなかった箇所はありますが、countしてるだけなので並び替えは不要かと思いました

bootcamp/app/models/cache.rb

Lines 25 to 29 in ae9fb5b

def not_responded_product_count

Rails.cache.fetch 'not_responded_product_count' do

Product.not_responded_products.count

end

end

今見返すとスコープが微妙な感じですね…
何か改善案があればお願いします🙇

list というスコープ名が適切ではない(preloadしてるだけなので)。でも代案が思いつかない

list したあとに order_for_list もしくは order_for_not_wip_list の呼び出しが推奨される、という暗黙のルールが存在してしまっている。とはいえ list の中にorderを入れると、作成日順か提出日順かを自由に選べなくなる

@konaga-k

list というスコープ名が適切ではない(preloadしてるだけなので)。でも代案が思いつかない

うーん、個人的にはlistがわかりやすいと思いますけど...🤔

preloadした結果をどういう名前で表せばいいのかよく分からなくなってしまったんですよね…
一旦これで行こうと思います🙇

konaga-k · 2021-09-05T12:00:01Z

app/models/product.rb

-      .order(created_at: :desc)
+      .preload(:practice,
+               :comments,
+               { user: :company },


bullet（SQLのN+1を検知するgem）が警告を吐いていたので { user: :company } を追加しました

konaga-k · 2021-09-05T12:02:54Z

app/controllers/api/products_controller.rb

-                .includes(
-                  :practice,
-                  { comments: { user: :avatar_attachment } },
-                  { user: [{ avatar_attachment: :blob }, :company] },
-                  { checks: { user: [:company] } }
-                )
-                .order(published_at: :desc)
+                .list
+                .order_for_list


全ての提出物タブでの提出物表示用に色々includesしていた箇所ですが、全部必要には見えなかったので、bullet.logを見つつ list スコープで置換しました
新しくN+1の警告が増えている箇所はありません

この状態で全ての提出物タブ（ http://localhost:3000/products ）にアクセスすると、bullet.logにincludes追加の警告が一つだけ出ます
listスコープでの置換前のincludesでも同じ警告が出ている + productとreportに直接のrelationが無いので、どう修正したものか分からずに困ってます
パフォーマンス問題なので、露骨に影響が出なければ一旦放置でいいかなと思っています

2021-09-05 16:27:01[WARN] user: konaga GET /api/reports/recents.json USE eager loading detected Report => [:checks] Add to your query: .includes([:checks]) Call stack /home/konaga/environment/fjord/bootcamp/app/views/api/reports/recents/index.json.jbuilder:6:in `block in _app_views_api_reports_recents_index_json_jbuilder___3742612758457132164_46982601130400' /home/konaga/environment/fjord/bootcamp/app/views/api/reports/recents/index.json.jbuilder:1:in `_app_views_api_reports_recents_index_json_jbuilder___3742612758457132164_46982601130400'

まずはscopeを使わずにN+1がでないような状態を目指してやってみてください〜

わかりづらくてすみません、scopeは使う・使わないどっちでも良いので、もうちょっとN+1がなくなるように頑張ってみてください〜

910fc38
右サイドメニューの直近の日報一覧でN+1が生じていたみたいです
提出物とは直接関係ありませんが、ついでなので潰しました

@konaga-k ありがとうございます！！

konaga-k · 2021-09-05T12:04:27Z

test/system/product/unassigned_test.rb

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


提出物の未アサインタブはSystem Test自体が存在していなかったので、他のタブのSystem Testを参考にして新規作成しました

konaga-k · 2021-09-05T12:06:44Z

test/system/product/unassigned_test.rb

+    # id順で並べたときの最初と最後の提出物を、提出日順で見たときに最新と最古になるように入れ替える
+    Product.update_all(created_at: 1.day.ago, published_at: 1.day.ago) # rubocop:disable Rails/SkipsModelValidations
+    # 最古の提出物を画面上で判定するため、提出物を1ページ内に収める
+    Product.unassigned.unchecked.not_wip.limit(Product.count - PAGINATES_PER).delete_all


提出物一覧の先頭と最後尾を確認することで並び順を検証するテストですが、
提出物に2ページ名以降が発生すると、画面1ページ目の最後尾と、未アサイン提出物全体の最後尾が一致しなくなるため、
1ページに収まるようにレコードを削除しています

@konaga-k こんな技があるんですね〜👏

konaga-k · 2021-09-05T12:11:14Z

app/controllers/api/products/passed_controller.rb

    products = Product
               .not_responded_products
               .list
+               .order_for_not_wip_list


Discordのmentorチャンネル投稿用のAPIでもlistスコープが使われています
WIPを含まない一覧は提出日の降順で並べるという方針を一貫させたかったため、画面同様に提出日の降順に並び替えました
（そもそも並び替えしなくてもいいかもしれない…）

to: 駒形さん
このAPIでのproductsの並び順を作成日降順 -> 提出日降順に変えています
問題あれば教えてください🙇

@konaga-k 問題ございません〜

udaikue · 2021-09-06T00:25:33Z

レビュー、ちょっとお時間いただきます🙏

udaikue

完璧にはチェックできていないかもしれませんが、良いと思います！丁寧に考えられていて参考になります〜

konaga-k

@udaikue
レビューありがとうございます🙏

konaga-k · 2021-09-11T08:32:45Z

app/models/product.rb

  scope :list, lambda {
    with_avatar
-      .preload([:practice, :comments, { checks: { user: { avatar_attachment: :blob } } }])
-      .order(created_at: :desc)
+      .preload(:practice,
+               :comments,
+               { user: :company },
+               { checks: { user: { avatar_attachment: :blob } } })
  }
-  scope :reorder_for_not_responded_products, -> { reorder(published_at: :desc, id: :desc) }
+  scope :order_for_list, -> { order(created_at: :desc, id: :desc) }
+  scope :order_for_not_wip_list, -> { order(published_at: :desc, id: :desc) }


preloadした結果をどういう名前で表せばいいのかよく分からなくなってしまったんですよね…
一旦これで行こうと思います🙇

konaga-k

@komagata
レビューお願いします🙏

komagata · 2021-09-13T17:57:49Z

app/controllers/api/products_controller.rb

-                .includes(
-                  :practice,
-                  { comments: { user: :avatar_attachment } },
-                  { user: [{ avatar_attachment: :blob }, :company] },
-                  { checks: { user: [:company] } }
-                )
-                .order(published_at: :desc)
+                .list
+                .order_for_list


まずはscopeを使わずにN+1がでないような状態を目指してやってみてください〜

komagata · 2021-09-13T17:59:32Z

test/system/product/self_assigned_test.rb

@@ -2,6 +2,8 @@

 require 'application_system_test_case'

+PAGINATES_PER = 50


できるだけsocpeが狭い方がいいのでProductsTestの中に入れちゃっていいと思います〜

5a1bebf
すみません、定数を中に入れ忘れてました🙇

konaga-k

@komagata
修正しました〜

konaga-k · 2021-09-16T11:16:51Z

app/controllers/api/products_controller.rb

-                .includes(
-                  :practice,
-                  { comments: { user: :avatar_attachment } },
-                  { user: [{ avatar_attachment: :blob }, :company] },
-                  { checks: { user: [:company] } }
-                )
-                .order(published_at: :desc)
+                .list
+                .order_for_list


910fc38
右サイドメニューの直近の日報一覧でN+1が生じていたみたいです
提出物とは直接関係ありませんが、ついでなので潰しました

komagata · 2021-09-21T05:51:50Z

#3228 (comment)

ありがとうございます！

komagata

conflictの解消お願いします〜

konaga-k · 2021-09-21T14:15:35Z

@komagata
コンフリクト解消しました🙏
テストが落ちてますが #3300 で解消されると思います

komagata · 2021-09-25T13:32:43Z

@konaga-k マージコミットがいくつかありますが、これは避けられないものですかね？（bootcampではrebaseで取り込むのを基本にしているため）

konaga-k · 2021-09-26T08:13:01Z

@komagata
rebaseに失敗したときに切り戻しができなくなると思ってマージコミットを積んでいました
rebaseするようにしたので確認お願いします🙇
https://github.com/fjordllc/bootcamp/pull/3228/commits

komagata

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

konaga-k self-assigned this Sep 5, 2021

konaga-k commented Sep 5, 2021

View reviewed changes

konaga-k requested a review from udaikue September 5, 2021 13:01

udaikue approved these changes Sep 11, 2021

View reviewed changes

konaga-k commented Sep 11, 2021

View reviewed changes

komagata requested changes Sep 13, 2021

View reviewed changes

konaga-k force-pushed the feature/reorder-products branch from dd1cf1b to 910fc38 Compare September 16, 2021 11:13

konaga-k commented Sep 16, 2021

View reviewed changes

komagata requested changes Sep 21, 2021

View reviewed changes

konaga-k added 12 commits September 26, 2021 13:25

提出物の並び順を変更

9495fc2

Product.listからorderを分離、長すぎるメソッドチェインを改行

353cce3

自分の担当の提出物の並び順を作成日か提出日かで間違えたときに、テストが失敗するように修正

bbc34dc

テストの期待結果の説明を修正

768cb96

提出物の並び順のテストを追加

bbb6f11

product.rbのスコープでorderとwhereを分離、reorderは不要のためorderに変更

6e79535

Rubocop修正

9e1760b

テスト名の重複解消

b6004e4

typo修正

d85a50f

failureになるテストを修正

3a675e9

右サイドメニューの日報一覧で生じるN+1を解消

5c23522

定数のスコープを狭めるため宣言をTestクラスの中に移動

9cd2dfd

konaga-k force-pushed the feature/reorder-products branch from abb9dc0 to 9cd2dfd Compare September 26, 2021 04:27

komagata approved these changes Sep 28, 2021

View reviewed changes

komagata merged commit 9f93461 into main Sep 28, 2021

komagata deleted the feature/reorder-products branch September 28, 2021 17:42

github-actions bot mentioned this pull request Sep 28, 2021

Release 2021-10-01 14:11:20 #3313

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

提出物一覧ページでの提出物の並び順を修正 #3228

提出物一覧ページでの提出物の並び順を修正 #3228

konaga-k commented Sep 5, 2021 •

edited

Loading

konaga-k left a comment

konaga-k Sep 5, 2021 •

edited

Loading

konaga-k Sep 6, 2021 •

edited

Loading

udaikue Sep 11, 2021

konaga-k Sep 11, 2021

konaga-k Sep 5, 2021

konaga-k Sep 5, 2021 •

edited

Loading

komagata Sep 13, 2021

komagata Sep 14, 2021

konaga-k Sep 16, 2021

komagata Sep 28, 2021

konaga-k Sep 5, 2021

konaga-k Sep 5, 2021

udaikue Sep 11, 2021

konaga-k Sep 5, 2021 •

edited

Loading

komagata Sep 6, 2021

udaikue commented Sep 6, 2021

udaikue left a comment

konaga-k left a comment

konaga-k Sep 11, 2021

konaga-k left a comment

komagata Sep 13, 2021

komagata Sep 13, 2021

konaga-k Sep 16, 2021 •

edited

Loading

konaga-k left a comment

konaga-k Sep 16, 2021

komagata commented Sep 21, 2021

komagata left a comment

konaga-k commented Sep 21, 2021

komagata commented Sep 25, 2021

konaga-k commented Sep 26, 2021

komagata left a comment

	def not_responded_product_count
	Rails.cache.fetch 'not_responded_product_count' do
	Product.not_responded_products.count
	end
	end

		@@ -2,6 +2,8 @@

		require 'application_system_test_case'

		PAGINATES_PER = 50

提出物一覧ページでの提出物の並び順を修正 #3228

提出物一覧ページでの提出物の並び順を修正 #3228

Conversation

konaga-k commented Sep 5, 2021 • edited Loading

概要

現在の提出物の並び順

本PRでの提出物の並び順

UI

konaga-k left a comment

Choose a reason for hiding this comment

konaga-k Sep 5, 2021 • edited Loading

Choose a reason for hiding this comment

konaga-k Sep 6, 2021 • 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

konaga-k Sep 5, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konaga-k Sep 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udaikue commented Sep 6, 2021

udaikue left a comment

Choose a reason for hiding this comment

konaga-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konaga-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konaga-k Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

konaga-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komagata commented Sep 21, 2021

komagata left a comment

Choose a reason for hiding this comment

konaga-k commented Sep 21, 2021

komagata commented Sep 25, 2021

konaga-k commented Sep 26, 2021

komagata left a comment

Choose a reason for hiding this comment

konaga-k commented Sep 5, 2021 •

edited

Loading

konaga-k Sep 5, 2021 •

edited

Loading

konaga-k Sep 6, 2021 •

edited

Loading

konaga-k Sep 5, 2021 •

edited

Loading

konaga-k Sep 5, 2021 •

edited

Loading

konaga-k Sep 16, 2021 •

edited

Loading