Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

提出物一覧のページの各タブの並びを昇順にした #4399

Merged
merged 18 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/products/passed_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class API::Products::PassedController < API::BaseController
def show
products = Product
.list
.order_for_not_wip_list
.ascending_by_date_of_publishing_and_id
@passed5 = products.count { |product| product.elapsed_days == 5 }
@passed6 = products.count { |product| product.elapsed_days == 6 }
@over7 = products.count { |product| product.elapsed_days >= 7 }
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/products/unassigned_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def index
.unchecked
.not_wip
.list
.order_for_not_wip_list
.ascending_by_date_of_publishing_and_id
@latest_product_submitted_just_5days = @products.find { |product| product.elapsed_days == 5 }
@latest_product_submitted_just_6days = @products.find { |product| product.elapsed_days == 6 }
@latest_product_submitted_over_7days = @products.find { |product| product.elapsed_days >= 7 }
Expand Down
3 changes: 1 addition & 2 deletions app/controllers/api/products/unchecked_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,13 @@ def index
Product.unchecked
.not_wip
.list
.order_for_not_wip_list
.ascending_by_date_of_publishing_and_id
.page(params[:page])
when 'unchecked_no_replied'
Product.unchecked_no_replied_products(current_user.id)
.unchecked
.not_wip
.list
.order_for_not_wip_list
.page(params[:page])
end
@latest_product_submitted_just_5days = @products.find { |product| product.elapsed_days == 5 }
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/products_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class API::ProductsController < API::BaseController
def index
@products = Product
.list
.order_for_list
.ascending_by_date_of_publishing_and_id
.page(params[:page])
end
end
8 changes: 4 additions & 4 deletions app/models/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ class Product < ApplicationRecord
{ checks: { user: { avatar_attachment: :blob } } })
}
scope :order_for_list, -> { order(created_at: :desc, id: :desc) }
Copy link
Contributor

@Saki-htr Saki-htr Apr 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created_atpublishe_at: :ascに変更していただけますと幸いです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちら、自分の勘違いでした。
order_for_listを提出物一覧ページで使っている部分はascending_by_date_of_publishing_and_idに置き換えているのでここのcreated_atは変更不要でした。

scope :order_for_not_wip_list, -> { order(published_at: :desc, id: :desc) }
scope :order_for_self_assigned_list, -> { order(commented_at: :desc, published_at: :desc) }
scope :ascending_by_date_of_publishing_and_id, -> { order(published_at: :asc, id: :asc) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idは昇順がいいのか、降順がいいのかわからなかったのですが、published_atに合わせて昇順にしています

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

並び替えはこのように、modelで定義してcontrollerで使っているんですね〜
とても勉強になりました!

idは昇順がいいのか、降順がいいのか

idは、昇順(上から見た並び順)に付与されたほうが、私も違和感がないと思いましたので、良いと思います〜

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@komagata
idは昇順が良いか、降順が良いかわからなかったのでアドバイスいただけると嬉しいです。
自分としては、published_at: :ascに合わせてidも昇順にしています。

scope :order_for_self_assigned_list, -> { order('commented_at asc nulls first, published_at asc') }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nulls firstを指定することでcomment_atnilのものが上に表示されます。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ order('commented_at asc nulls first, published_at asc') }が何を行っているかペアプロで教えていただきましたので、こちらに書いておきます〜

  • orderの引数には、並び替えの優先度の高いものを先に渡す。なので、published_at(提出日時)よりも先にcommented_at(コメント日時)を渡している。(published_atを先に書くと、提出日の昇順となってしまい、更に提出日が同じ提出物の中でcomment_atの昇順になってしまう。)
  • コメントが無いものを先に表示したいので、commented_at nulls firstと書いている。nulls firstと書くことで、commented_atカラムがnullのものを最初に持ってくることができる。
  • これにascを加えて、commented_at asc nulls firstと書くことで、comemnted_atnillでない提出物=コメントがされている提出物を、「コメント日時の昇順」に並び替える。
  • published_at asc'と書くことで、コメント無しの提出物を、「提出日の昇順」に並び替える


def self.add_latest_commented_at
Product.all.includes(:comments).find_each do |product|
Expand Down Expand Up @@ -122,13 +122,13 @@ def self.unchecked_no_replied_products_ids(current_user_id)
def self.self_assigned_no_replied_products(current_user_id)
no_replied_product_ids = self_assigned_no_replied_product_ids(current_user_id)
Product.where(id: no_replied_product_ids)
.order(commented_at: :desc, published_at: :desc)
.order(published_at: :asc, id: :asc)
end
Comment on lines 124 to 126
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「自分の担当」の未返信で使われているのですが、未返信の時点でcommented_atには何も入ってなく、意味を為していなかったので削除し、published_atid で並ぶようにしました。


def self.unchecked_no_replied_products(current_user_id)
no_replied_products_ids = unchecked_no_replied_products_ids(current_user_id)
Product.where(id: no_replied_products_ids)
.order(created_at: :desc)
.order(published_at: :asc, id: :asc)
end

def completed?(user)
Expand Down
3 changes: 2 additions & 1 deletion db/fixtures/products.yml
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ product62:
practice: practice1
user: take8
body: テストの提出物62です。
published_at: <%= (now - 60 * 60 * 24 * 10).to_s(:db) %>
published_at: <%= 10.day.ago.to_s(:db) %>
created_at: <%= 10.day.ago.to_s(:db) %>

product63:
practice: practice6
Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/products.yml
Original file line number Diff line number Diff line change
Expand Up @@ -356,20 +356,29 @@ product62:
practice: practice45
user: kimura
body: 「自分の担当」かつ「未返信」の提出物
created_at: <%= 1.day.ago %>
updated_at: <%= 1.day.ago %>
published_at: <%= 1.day.ago %>
wip: false
checker_id: <%= ActiveRecord::FixtureSet.identify(:machida) %>

product63:
practice: practice46
user: kimura
body: 「自分の担当」かつ「返信済み」の提出物
created_at: <%= 2.day.ago %>
updated_at: <%= 2.day.ago %>
published_at: <%= 2.day.ago %>
wip: false
checker_id: <%= ActiveRecord::FixtureSet.identify(:machida) %>

product64:
practice: practice10
user: kimura
body: 担当者のいる提出物です。
created_at: <%= Time.current %>
updated_at: <%= Time.current %>
published_at: <%= Time.current %>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

日時の違う提出物が必要だったため、新たに定義

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ペアプロで、テスト実行時に必要だったため追記したとご説明いただきました:+1:
checker_id<%= ActiveRecord::FixtureSet.identify(:machida) %>と渡すと、その方の担当の提出物になるんですね〜! 知らなかったのでとても勉強になりました☺️

checker_id: "<%= ActiveRecord::FixtureSet.identify(:machida) %>"

product65:
Expand Down
12 changes: 12 additions & 0 deletions test/integration/api/products_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
require 'test_helper'

class API::ProductsTest < ActionDispatch::IntegrationTest
fixtures :products

test 'GET /api/products.json' do
get api_products_path(format: :json)
assert_response :unauthorized
Expand Down Expand Up @@ -42,4 +44,14 @@ class API::ProductsTest < ActionDispatch::IntegrationTest
headers: { 'Authorization' => "Bearer #{token}" }
assert_response :ok
end

test 'should return products in order ' do
token = create_token('machida', 'testtest')
get api_products_self_assigned_index_path(format: :json),
headers: { 'Authorization' => "Bearer #{token}" }

expected = products(:product15, :product63, :product62, :product64).map { |product| product.practice.title }
actual = response.parsed_body['products'].map { |product| product['practice']['title'] }
assert_equal expected, actual
Comment on lines +53 to +55
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected actual それぞれの変数名が適切かあんまりわかってない

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いでさんより、こちらのテストが何を検証しているかペアプロでご説明いただいたので書いておきます🙏

  • 並び順が published_atの昇順か をテストしたい。
  • product(:product15) とかくとfixturesからデータをひっぱってくることができるので、テストデータを利用している。
  • expectedには、アプリ画面上に見える提出物のタイトル名が入った配列を入れている。
  • actualはapiから返ってきたレスポンスの、タイトル名の入った配列を入れている。
  • expectedactualを比較してtrueになればテストが通る

end
end
24 changes: 6 additions & 18 deletions test/system/product/self_assigned_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,26 +42,14 @@ class Product::SelfAssignedTest < ApplicationSystemTestCase
end
end

test 'products order on self assigned tab' do
checker = users(:komagata)
# id順で並べたときの最初と最後の提出物を、コメントの日付順・提出日順で見たときに最新と最古になるように入れ替える
# 最古の提出物を画面上で判定するため、提出物を1ページ内に収める
Product.update_all(created_at: 3.days.ago, published_at: 3.days.ago, checker_id: checker.id) # rubocop:disable Rails/SkipsModelValidations
Product.unchecked.limit(Product.count - Product.default_per_page).delete_all
newest_product = Product.self_assigned_product(checker.id).unchecked.reorder(:id).first
newest_product.update(published_at: 1.day.ago)
oldest_product = Product.self_assigned_product(checker.id).unchecked.reorder(:id).last
oldest_product.update(commented_at: Time.current, published_at: Time.current)
test 'display products on self assigned tab' do
oldest_product = products(:product15)
newest_product = products(:product64)

visit_with_auth '/products/self_assigned', 'komagata'
visit_with_auth '/products/self_assigned', 'machida'

# 作成日の降順で並んでいることを検証する
titles = all('.thread-list-item-title__title').map { |t| t.text.gsub('★', '') }
names = all('.thread-list-item-meta .a-user-name').map(&:text)
assert_equal "#{newest_product.practice.title}の提出物", titles.first
assert_equal newest_product.user.login_name, names.first
assert_equal "#{oldest_product.practice.title}の提出物", titles.last
assert_equal oldest_product.user.login_name, names.last
assert_equal 'OS X Mountain Lionをクリーンインストールする', oldest_product.practice.title
assert_equal 'sshdをインストールする', newest_product.practice.title
end

test 'not display products in listing self-assigned if self-assigned products all checked' do
Expand Down
20 changes: 6 additions & 14 deletions test/system/product/unassigned_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,20 @@ class Product::UnassignedTest < ApplicationSystemTestCase
.unassigned
.unchecked
.not_wip
.order_for_not_wip_list
.ascending_by_date_of_publishing_and_id
.first
assert_text newest_product.body
end
end

test 'products order on unassigned tab' do
# id順で並べたときの最初と最後の提出物を、提出日順で見たときに最新と最古になるように入れ替える
Product.update_all(created_at: 1.day.ago, published_at: 1.day.ago) # rubocop:disable Rails/SkipsModelValidations
newest_product = Product.unassigned.unchecked.not_wip.reorder(:id).first
newest_product.update(published_at: Time.current)
oldest_product = Product.unassigned.unchecked.not_wip.reorder(:id).last
oldest_product.update(published_at: 2.days.ago)
oldest_product = products(:product14)
newest_product = products(:product26)

visit_with_auth '/products/unassigned', 'komagata'

# 提出日の降順で並んでいることを検証する
titles = all('.thread-list-item-title__title').map { |t| t.text.gsub('★', '') }
names = all('.thread-list-item-meta .a-user-name').map(&:text)
assert_equal "#{newest_product.practice.title}の提出物", titles.first
assert_equal newest_product.user.login_name, names.first
assert_equal "#{oldest_product.practice.title}の提出物", titles.last
assert_equal oldest_product.user.login_name, names.last
# 提出日の昇順で並んでいることを検証する
assert_equal 'OS X Mountain Lionをクリーンインストールする', oldest_product.practice.title
assert_equal 'sshdでパスワード認証を禁止にする', newest_product.practice.title
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/system/product/self_assigned_test.rbで書いてくださったのと同様に、fixturesからデータをひっぱってくると簡潔で良いかなと思いましたので、ご変更いただけますと幸いです🙏

33 changes: 8 additions & 25 deletions test/system/product/unchecked_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,31 +27,22 @@ class Product::UncheckedTest < ApplicationSystemTestCase
newest_product = Product
.unchecked
.not_wip
.order_for_not_wip_list
.ascending_by_date_of_publishing_and_id
.first
assert_text newest_product.body
end
end

test 'products order on unchecked tab' do
# id順で並べたときの最初と最後の提出物を、提出日順で見たときに最新と最古になるように入れ替える
Product.update_all(created_at: 1.day.ago, published_at: 1.day.ago) # rubocop:disable Rails/SkipsModelValidations
# 最古の提出物を画面上で判定するため、提出物を1ページ内に収める
Product.unchecked.not_wip.limit(Product.count - Product.default_per_page).delete_all
newest_product = Product.unchecked.not_wip.reorder(:id).first
newest_product.update(published_at: Time.current)
oldest_product = Product.unchecked.not_wip.reorder(:id).last
oldest_product.update(published_at: 2.days.ago)
oldest_product = products(:product14)
newest_product = products(:product26)

# 提出日の昇順で並んでいることを検証する
visit_with_auth '/products/unchecked', 'komagata'
assert_equal 'OS X Mountain Lionをクリーンインストールする', oldest_product.practice.title

# 提出日の降順で並んでいることを検証する
titles = all('.thread-list-item-title__title').map { |t| t.text.gsub('★', '') }
names = all('.thread-list-item-meta .a-user-name').map(&:text)
assert_equal "#{newest_product.practice.title}の提出物", titles.first
assert_equal newest_product.user.login_name, names.first
assert_equal "#{oldest_product.practice.title}の提出物", titles.last
assert_equal oldest_product.user.login_name, names.last
visit_with_auth '/products/unchecked?page=2', 'komagata'
assert_equal 'sshdでパスワード認証を禁止にする', newest_product.practice.title
end

test 'not display products in listing unchecked if unchecked products all checked' do
Expand Down Expand Up @@ -89,15 +80,7 @@ class Product::UncheckedTest < ApplicationSystemTestCase
end

test 'display no-replied products if click on no-replied-button' do
checker = users(:komagata)
practice = practices(:practice47)
user = users(:kimura)
product = Product.create!(
body: 'test',
user: user,
practice: practice,
checker_id: checker.id
)
product = products(:product8)
visit_with_auth "/products/#{product.id}", 'kimura'
fill_in('new_comment[description]', with: 'test')
click_button 'コメントする'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いでさんとペアプロしていただいた際にこちらのテストの意図を教えていただきましたので、メモしておきます〜

  • 「提出物を提出したユーザー本人(kimura)が、自分の提出物にコメントしても、「未完了」タブの「未返信」に、提出物が表示されたまま であることをテストしている。

Expand Down