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

Conversation

Aseiide
Copy link
Contributor

@Aseiide Aseiide commented Mar 14, 2022

関連issue

概要・要件

提出物一覧のページにおいて、以下の要件に従って並び替える。

  • タブ「全て」
    • 作成日時の降順から提出日の昇順に変更
  • タブ「未完了」
    • 全て: 提出日の降順から提出日の昇順に変更
    • 未返信: 提出日の降順から提出日の昇順に変更
  • タブ「未アサイン」
    • 提出日の降順から提出日の昇順に変更
  • タブ「自分の担当」
    • 全て: コメント無・提出日の降順/ コメント有・コメント日時の降順 から コメント無・提出日の昇順/ コメント有・コメント日時の昇順に変更
    • 未返信: 提出日の降順から提出日の昇順に変更

変更前と変更後のスクリーンショットイメージ

画面 before after
タブ・全て 全て 全て
タブ未完了 未完了 未完了
タブ・未アサイン 未アサイン 未アサイン
タブ・自分の担当 自分の担当 自分の担当

@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 2ff9fa5 to 01829c6 Compare March 14, 2022 09:08
Comment on lines 52 to 53
scope :order_for_not_wip_list_descinding, -> { order(published_at: :desc, id: :desc) }
scope :order_for_not_wip_list_ascending, -> { 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.

descindingdescindingとわかるように、それに対応させてascendingascendingとわかるように命名しました。
idは昇順にするか降順にするか迷いましたが、メソッド名と対応させています。

@Aseiide Aseiide changed the title [WIP]未アサインの提出物一覧の並び順を逆にしたい 未アサインの提出物一覧の並び順を逆にしたい Mar 14, 2022
@Aseiide Aseiide requested a review from Saki-htr March 14, 2022 10:20
@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 14, 2022

@Saki-htr こちらレビューお願いいたします!

@Aseiide Aseiide changed the title 未アサインの提出物一覧の並び順を逆にしたい 未アサインの提出物一覧の並び順を逆にした Mar 16, 2022
@konaga-k
Copy link
Contributor

konaga-k commented Mar 16, 2022

横からすみません、気になったので質問させて下さい〜
これって他の提出物タブについて並び順を昇順にしたほうが自然かどうかは議論されてるんでしょうか?

未完了や自分の担当の提出物について、経過日数が多いもの(ほったらかしのもの)と少ないもの(新しく提出されたもの)のどちらを優先して見たいのかは意見が割れそうだと思いました
あと未アサインタブとそれ以外のタブで並び順が違うと、利用者視点で単純に疑問が湧くんじゃないかというのもあります
(既に議論済みだったらすみません🙇)

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 16, 2022

@konaga-k
コメントありがとうございます。
ご指摘の点については、議論していないませんでした。

確かに、未アサインのタブの並びだけ変わると混乱するかもしれないですね。
現在、提出物一覧ページにおいてタブ「全て」「未完了」「自分の担当」は降順になっていて、今回の変更でタブ「未アサイン」のみ昇順となりますね。

@machida @komagata
konagaさんのコメント含め、タブ「未アサイン」とその他のタブで並び順が変わってします点はいかがいたしましょう??

Copy link
Contributor

@Saki-htr Saki-htr left a comment

Choose a reason for hiding this comment

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

@Aseiide
お疲れさまです。
動作確認したところOKでした🙆‍♀️
コードも良いと思います! 提出物の並び順をテストする時はこのように書くのですね〜。とても勉強になりました🙏

一点気になったのですが、descriptionの変更前と変更後の画像の、黒字の「提出日時が古い」「提出日時が新しい」の部分が、実際の変更内容と逆になっているようですので、修正した方がいいかなと思いました。

他のタブと並び順が変わる点について確認中でいらっしゃるので、新たに仕様追加があるかもしれませんので、まだApproveはしないでおきます〜
よろしくお願いいたします🙇‍♀️

@komagata
Copy link
Member

@Aseiide (cc: @machida)

konagaさんのコメント含め、タブ「未アサイン」とその他のタブで並び順が変わってします点はいかがいたしましょう??

僕は他も全部今回の新しいのと同じ並びにするのがいいかな〜と思いました。
もしすごく大変な作業になるようでしたら言っていただければと思います〜。

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 21, 2022

@Saki-htr
レビューありがとうございました!要件に変更があるので、その修正をしたら再度レビュー依頼させてもらいますね。
スクショの指摘もありがとうございます。直します!

@komagata
承知しました。他のタブも同様に順番並び替えるようにします。おそらくそんなに大変ではないと思いますが、なにか有りましたら相談します。

@komagata
Copy link
Member

@Aseiide ありがとうございます!結構手間が増えそうなのでIssueのポイントを3に増やしておきました〜!

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 23, 2022


【未アサイン】

提出日時が古いもの
提出日時が新しいもの


【未完了】

提出日時が古いもの
提出日時が新しいもの


【全て】

提出日時が古いもの
提出日時が新しいもの

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 23, 2022

開発MTG質問メモ
自分の担当タブ

  • コメント無が上に来る
    • 提出日時(古)が上
    • 提出日時(新)が下
  • コメント有りは下に置く
    • コメントありのときは、
      • コメントの古いものが上
      • コメントが新しいものが下


コメント無・提出日時(古)
コメント無・提出日時(新)
コメント有・コメント日時(古)
コメント有・コメント日時(新)

@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 01829c6 to cf754e6 Compare March 23, 2022 13:35
@Aseiide Aseiide self-assigned this Mar 24, 2022
@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 15bae9b to ba94b38 Compare March 24, 2022 03:02
@komagata
Copy link
Member

komagata commented Mar 24, 2022

📝 システムでよく使う表現として昇順降順という言葉があるのでそれを使うと厳密に表現できるかもです。

@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from ba94b38 to 5e19f1a Compare March 25, 2022 08:32
Copy link
Contributor Author

@Aseiide Aseiide left a comment

Choose a reason for hiding this comment

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

タブ・未アサインだけでなく、他のタブも昇順にし、関連するテストの修正を行いました。

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) }
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'と書くことで、コメント無しの提出物を、「提出日の昇順」に並び替える

@@ -49,8 +49,8 @@ class Product < ApplicationRecord
{ checks: { user: { avatar_attachment: :blob } } })
}
scope :order_for_list, -> { order(created_at: :desc, id: :desc) }
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も昇順にしています。

@Aseiide Aseiide changed the title 未アサインの提出物一覧の並び順を逆にした 提出物一覧のページの各タブの並びを昇順にした Mar 27, 2022
@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from d0105c9 to 49894ce Compare March 28, 2022 13:25
Comment on lines +53 to +55
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
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になればテストが通る

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) %>と渡すと、その方の担当の提出物になるんですね〜! 知らなかったのでとても勉強になりました☺️

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 29, 2022

@Saki-htr
再実装終わりましたのでレビューお願いします。
前回レビューいただいたときから要件が変わっていて、テキストのやり取りだと説明が難しいのでDiscordで繋ぎながらレビューさせてもらえると良いかもしれないです。

CIが落ちてるのは、 #4517 の修正でなおりそうです。

@Aseiide
Copy link
Contributor Author

Aseiide commented Mar 29, 2022

実装が複雑になったので、4/2 14時からペアプロでsakiさんにレビューしてもらう。

@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 49894ce to 7329867 Compare March 29, 2022 05:51
@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 7329867 to 1111b10 Compare April 2, 2022 14:08
@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from 1111b10 to e04d24d Compare April 3, 2022 04:10
Copy link
Contributor

@Saki-htr Saki-htr left a comment

Choose a reason for hiding this comment

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

@Aseiide
ペアプロで大変丁寧にご説明くださって、ありがとうございました。
動作確認はOKでした🙆‍♀️✨
気になった点をいくつかコメントさせていただきましたので、ご確認をお願いいたします〜🙏

@@ -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は変更不要でした。

Comment on lines +53 to +55
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
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になればテストが通る

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

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) %>と渡すと、その方の担当の提出物になるんですね〜! 知らなかったのでとても勉強になりました☺️

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) }
scope :order_for_self_assigned_list, -> { order('commented_at asc nulls first, published_at asc') }
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'と書くことで、コメント無しの提出物を、「提出日の昇順」に並び替える

assert_equal "#{oldest_product.practice.title}の提出物", titles.first
assert_equal oldest_product.user.login_name, names.first
assert_equal "#{newest_product.practice.title}の提出物", titles.last
assert_equal newest_product.user.login_name, names.last
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からデータをひっぱってくると簡潔で良いかなと思いましたので、ご変更いただけますと幸いです🙏

@@ -48,10 +48,10 @@ class Product::UncheckedTest < ApplicationSystemTestCase
# 提出日の降順で並んでいることを検証する
titles = all('.thread-list-item-title__title').map { |t| t.text.gsub('★', '') }
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらも、fixturesからデータを持ってくると、より簡潔なコードになって良いと思います〜🙏

@@ -48,10 +48,10 @@ class Product::UncheckedTest < ApplicationSystemTestCase
# 提出日の降順で並んでいることを検証する
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらのコメントも「昇順」にご変更をお願いいたします〜🙏

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)が、自分の提出物にコメントしても、「未完了」タブの「未返信」に、提出物が表示されたまま であることをテストしている。

@@ -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(commented_at: :desc, published_at: :asc)
Copy link
Contributor

Choose a reason for hiding this comment

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

自分の担当タブの「未返信」には、全てコメントが付いていない提出物が表示されるので、commented_at: :descは不要ではないかと思いました。
また、こちらにもid: :ascを追加した方が良いと思います〜

@@ -49,8 +49,8 @@ class Product < ApplicationRecord
{ checks: { user: { avatar_attachment: :blob } } })
}
scope :order_for_list, -> { order(created_at: :desc, id: :desc) }
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

Choose a reason for hiding this comment

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

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

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

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

@Aseiide Aseiide force-pushed the feature/reverse-list-of-unassigned-products branch from e676b29 to 46cbf32 Compare April 3, 2022 14:24
Copy link
Contributor Author

@Aseiide Aseiide left a comment

Choose a reason for hiding this comment

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

@Saki-htr
テストの修正とcommented_atが不要だった件に対応したのでレビューお願いいたします。

@@ -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 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は変更不要でした。

Comment on lines 124 to 126
Product.where(id: no_replied_product_ids)
.order(commented_at: :desc, published_at: :desc)
.order(published_at: :asc, id: :asc)
end
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 で並ぶようにしました。

@Saki-htr
Copy link
Contributor

Saki-htr commented Apr 6, 2022

@Aseiide
ご修正ありがとうございます!
おまたせしていてすみません、明日にはお返しできるようにいたします🙏

@Aseiide
Copy link
Contributor Author

Aseiide commented Apr 6, 2022

@Saki-htr 全然だいじょうぶです!

@Saki-htr
Copy link
Contributor

Saki-htr commented Apr 7, 2022

@Aseiide
テストの修正と、commented_atの削除を確認しました🙆‍♀️
komagataさんに仕様について質問したことや、実装について細かくPRにメモされていて、他の方から見ても分かりやすくなっていて素晴らしいと思いました👏 descriptionの、変更前と変更後のスクリーンショットイメージも、一度目よりも一層見やすくなっていて良いと思いました!

ペアプロした際に、"「自分の担当」タブの「全て」欄の、コメント有りと無しの境目がどこか分かりづらい" 点についてお話したかと思うのですが、

  • 今回のissue内容は提出物の並び順を変更することなので、このPRとは別に新しいissueを作成して解決することではないか
  • リリースされてメンターの方々に使っていただいてから検討していくことではないか

と個人的に考えましたので私の方ではApproveさせていただきます☺️

image

よろしくお願いいたします。

@Aseiide
Copy link
Contributor Author

Aseiide commented Apr 7, 2022

@Saki-htr
レビューありがとうございました!

コメント有りと無しの境目がどこか分かりづらい

こちらはリリース後にissueつくってメンターさんの反応を見るのが良さそうです。

@komagata
チームメンバーのレビューが通りましたのでレビューお願いします。

test/integration/api/products_test.rb Outdated Show resolved Hide resolved
Co-authored-by: Masaki Komagata <komagata@gmail.com>
@Aseiide
Copy link
Contributor Author

Aseiide commented Apr 9, 2022

@komagata レビューありがとうございます。修正したのでレビューお願いいたします。

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

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

@komagata komagata merged commit ee09261 into main Apr 13, 2022
@komagata komagata deleted the feature/reverse-list-of-unassigned-products branch April 13, 2022 11:58
@github-actions github-actions bot mentioned this pull request Apr 13, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants