「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 #3624

AudioStakes · 2021-11-25T08:02:53Z

Ref: #3594

やったこと

メンター用の提出物一覧ページにおいて、「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装
上記の実装により落ちるようになったテストの修正（詳細は↓に別途記載）
- テストで自分の担当件数として「返信済みも含めたすべての担当件数」を取得するようにした
- テストのアサーションで「コメントすると担当件数が1件増えること」を確認できるようにするため、38f2828 をリバートした

画面の変更内容

※画像の右側の赤枠部分

変更前

変更後

※ 1件以上の場合は今まで通り表示

Issue の対応により落ちるようになったテストとその対応

Ref: #3649

調べた結果もともとテストが期待どおりに動いていなかったので、期待どおりに動くようにテストを修正した（逆に言うと、もともと期待どおりに動いていれば、この Issue の対応によってテストが落ちることはなかった）

落ちるようになったテスト

テストA

テストで確認したいこと
- メンターが担当者のいない提出物にコメントすると、その提出物の担当になること

テストコード

bootcamp/test/system/products_test.rb

Lines 297 to 316 in de2e6a8

    
           test 'be person on charge at comment on product of there are not person on charge' do 
        
             visit_with_auth products_not_responded_index_path, 'machida' 
        
             def assigned_product_count 
        
               find_link('自分の担当').find('.page-tabs__item-count').text.to_i 
        
             end 
        
             before_comment = assigned_product_count 
        
             [ 
        
               '担当者がいない提出物の場合、担当者になる', 
        
               '自分が担当者の場合、担当者のまま', 
        
               'タブ上の数字は未返信の数字のため、返信するとカウントされない' 
        
             ].each do |comment| 
        
               visit "/products/#{products(:product1).id}" 
        
               post_comment(comment) 
        
               visit products_not_responded_index_path 
        
               assert_equal before_comment, assigned_product_count 
        
             end 
        
           end

テストB

テストで確認したいこと
- メンターがすでに担当者のいる提出物にコメントしても、その提出物の担当にならないこと

テストコード

bootcamp/test/system/products_test.rb

Lines 318 to 338 in de2e6a8

    
           test 'be not person on charge at comment on product of there are person on charge' do 
        
             visit_with_auth products_not_responded_index_path, 'komagata' 
        
             product = find('.thread-list-item', match: :first) 
        
             product.click_button '担当する' 
        
             show_product_path = product.find_link(href: /products/)[:href] 
        
             logout 
        
             visit_with_auth products_not_responded_index_path, 'machida' 
        
             def assigned_product_count 
        
               find_link('自分の担当').find('.page-tabs__item-count').text.to_i 
        
             end 
        
             before_comment = assigned_product_count 
        
             visit show_product_path 
        
             post_comment('担当者がいる提出物の場合、担当者にならない') 
        
             visit products_not_responded_index_path 
        
             assert_equal before_comment, assigned_product_count 
        
           end

対応したこと

対応1: テストコードで自分の担当件数として「返信済みも含めたすべての担当件数」を取得するようにした

修正対象

テストAとテストBの両方

※ どちらのテストも、落ちるようになった原因は同じ

エラーメッセージ

テストA

Error:
ProductsTest#test_be_person_on_charge_at_comment_on_product_of_there_are_not_person_on_charge:
Capybara::ElementNotFound: Unable to find css ".page-tabs__item-count" within #<Capybara::Node::Element tag="a" path="/HTML/BODY[1]/DIV[1]/MAIN[1]/DIV[1]/DIV[1]/DIV[1]/UL[1]/LI[5]/A[1]">
    test/system/products_test.rb:300:in `assigned_product_count'
    test/system/products_test.rb:303:in `block in <class:ProductsTest>'

rails test test/system/products_test.rb:297

テストB

Error:
ProductsTest#test_be_not_person_on_charge_at_comment_on_product_of_there_are_person_on_charge:
Capybara::ElementNotFound: Unable to find css ".page-tabs__item-count" within #<Capybara::Node::Element tag="a" path="/HTML/BODY[1]/DIV[1]/MAIN[1]/DIV[1]/DIV[1]/DIV[1]/UL[1]/LI[5]/A[1]">
    test/system/products_test.rb:328:in `assigned_product_count'
    test/system/products_test.rb:331:in `block in <class:ProductsTest>'

rails test test/system/products_test.rb:318

落ちるようになった原因

テストコードにおいて担当件数が0件の時に「自分の担当」タブのバッチから担当件数が取得されており、今回の Issue の対応によりバッチが表示されなくなったことで、バッチが見つからないエラーが生じるようになったため
調査してわかったこと

テストが追加された Issue 担当者がいない提出物にメンターがコメントすると自動で担当者になる #2460 を調べてみると、テストコードで「本来取得したいはずの件数」と「実際に取得している件数」が異なっているということがわかった
- 本来取得したいはずの件数
  - 返信済みも含めた自分の担当の提出物すべての件数
- 実際に取得している件数
  - 返信済みを除いた自分の担当の提出物の件数（すなわち未返信の件数）
    - 補足: 「自分の担当」タブのバッチに表示されている件数が取得されている
対応したこと

自分の担当件数として「返信済みも含めたすべての担当件数」を取得するようにした。具体的には、「自分の担当」タブのテキストから件数を取得するようにした。
- 例: 「自分の担当（2）」というテキストの場合、担当件数は「2」
対応した結果
- テストA
  - エラーは起きなくなったけれど、テストが失敗するようになった
- テストB
  - エラーは起きなくなり、テストも成功するようになった

対応2: テストAで本来確認したいことを確認できるようにするため、 `38f2828` をリバートした

修正対象

テストAのみ

エラーメッセージ（対応1の修正後）

Failure:
ProductsTest#test_be_person_on_charge_at_comment_on_product_of_there_are_not_person_on_charge [/Users/satoudaisuke/src/github.com/fjordllc/bootcamp/test/system/products_test.rb:314]:
Expected: 0
  Actual: 1

rails test test/system/products_test.rb:297

失敗するようになった原因

テストAのアサーションでは「担当者のいない提出物にコメントしても担当件数が変わらないこと」を確認しているけれど、対応1の修正によりコメントすると担当件数が1件増えるようになったため
疑問に感じたこと

テストAで確認したいことは「コメントすると担当になる（件数が1件増える）こと」のはずなのに、なぜ「コメントしても担当にならない（件数が変わらない）こと」が確認されているのか？
調査してわかったこと

38f2828 でテストAのアサーションが変更されていた
- アサーションがどのように変更されたか
  
  「コメントしても担当件数が変わらないこと」を確認するように変更された
- なぜアサーションが修正されたか
  
  PR 担当提出物に未返信の絞り込み機能を実装 #3241 の実装過程において、テストAが失敗するようになったため
  - テストAが失敗するようになった実装
    
    「自分の担当」タブのバッチに表示する件数を未返信のみに絞るようにした実装
  - 起きたこと
    
    担当者のいない提出物にコメントすると、その提出物は「自分の担当」かつ「返信済み」となるため、バッチに表示される件数（未返信の件数）が増えなくなった → そのためテストAが失敗するようになった
- 考えたこと
  
  テストAで確認したいことは「コメントすると担当になること」なので、 38f2828 で「コメントしても担当にならないこと」を確認するようにアサーションを変えたことは、修正として不適切だったのではないのではないだろうか？
- どのように修正すると適切だったか
  
  テストAで確認したいことを考慮すると、アサーションは変更せずに、対応1のような修正をした方が適切だったように思います
対応したこと

テストAのアサーションで「コメントすると担当件数が1件増えること」を確認できるようにするため、 38f2828 をリバートした
対応した結果

テストAが成功するようになった

This reverts commit 38f2828.

AudioStakes · 2021-11-26T05:57:27Z

@haruguchi-yuma
ご都合の良い時に、こちらのレビューをお願いします!

Issue の対応により落ちるようになったテストについても追加で対応しています。テストの対応についての説明量が多くなり、レビューに時間がかかるかもしれません。

もし不明点がありましたらお気軽に質問してください。よろしくお願いします🙏

haruguchi-yuma

@AudioStakes
確認しました。PRがとても詳しく書かれていて、テスト変更の経緯など分かりやすかったです😄

確かに変更前はテストしたいこととアサーションがずれている気がするので、今回の変更で問題ないと思います！OKです👍

AudioStakes · 2021-11-26T07:30:00Z

@haruguchi-yuma
早速のレビュー、ありがとうございます！

@komagata
ご都合の良いときに、こちらのレビューをよろしくお願いします🙏

komagata

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

AudioStakes · 2021-12-01T08:39:33Z

「Issue の対応により落ちるようになったテストとその対応」の作業分のポイントについて、2021/12/01(水)15時~ のミーティングで machida さんに質問した結果、「新たに Issue を立ててそこで見積もる」と回答いただいたのでその Issue を新たに立てました。

新たに立てた Issue

#3649

ミーティングで質問したこと

落ちるようになったテストはもともと期待どおりに動いておらず、その対応作業は #3594 の見積もり対象の作業分とは別枠になると思うため、その分のポイントを加算するなどの調整をしてもらうことは可能なのだろうか？また可能な場合、どのようにそのポイントを見積もるのだろうか？

回答してもらったこと

今回の対応分はバグ修正にあたるので、ポイントを加算することになる。
ポイントの見積もりは、新たに Issue を立ててそこで行う。

AudioStakes · 2021-12-02T01:50:14Z

ステージングでこの機能の動作確認をしようとした結果、他のバグの影響で動作確認を完了できなかった。
以下、そのことについて Discord で報告・相談した文章の転載。

先ほどステージングで動作確認をしていて、僕が実装した機能に関連するバグを見つけました。バグの詳細は #3638 に記載しています。

このバグがある状態では僕が実装した機能をうまく動作確認できなさそうなので、このバグを修正するまでリリースを待った方が良いかもしれないと考えています。リリースを待った方が良いかどうか、何か対応した方が良さそうなことがあるか、などについてアドバイスをいただけますでしょうか？

なお、開発時にこのバグに気づかなかった理由は、このバグにはキャッシュが関わっており、開発環境でキャッシュが有効にされていない状態で実装を進めていたためです。

AudioStakes self-assigned this Nov 25, 2021

AudioStakes changed the title ~~「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように修正~~ 「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 Nov 26, 2021

AudioStakes added 3 commits November 26, 2021 14:15

「自分の担当」タブのバッチに表示されている件数が０件の場合はバッチを表示しないように実装

1904cb9

提出物にコメントすると担当になる機能のテストにおいて担当する提出物すべての件数を取得するように修正

03e7f51

Revert "自分の担当タブの数字を変更したことに伴う、システムテストの修正"

9a327d0

This reverts commit 38f2828.

AudioStakes force-pushed the feature/hide_a_badge_on_self_assigned_tab_if_the_number_displayed_on_the_badge_is_zero branch from 762491b to 9a327d0 Compare November 26, 2021 05:21

AudioStakes marked this pull request as ready for review November 26, 2021 05:49

AudioStakes requested a review from haruguchi-yuma November 26, 2021 06:33

haruguchi-yuma reviewed Nov 26, 2021

View reviewed changes

AudioStakes requested a review from komagata November 26, 2021 07:30

komagata approved these changes Nov 29, 2021

View reviewed changes

komagata merged commit 2cd3b53 into main Nov 29, 2021

komagata deleted the feature/hide_a_badge_on_self_assigned_tab_if_the_number_displayed_on_the_badge_is_zero branch November 29, 2021 23:23

github-actions bot mentioned this pull request Nov 29, 2021

Release 2021-11-30 08:31:11 #3627

Merged

8 tasks

AudioStakes mentioned this pull request Dec 1, 2021

「担当者のいない(or いる）提出物にコメントするとその提出物の担当になる(or ならない)」ことを確認するテストが期待どおりに動いていない #3649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 #3624

「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 #3624

AudioStakes commented Nov 25, 2021 •

edited

Loading

AudioStakes commented Nov 26, 2021

haruguchi-yuma left a comment

AudioStakes commented Nov 26, 2021

komagata left a comment

AudioStakes commented Dec 1, 2021

AudioStakes commented Dec 2, 2021 •

edited

Loading

	test 'be person on charge at comment on product of there are not person on charge' do
	visit_with_auth products_not_responded_index_path, 'machida'
	def assigned_product_count
	find_link('自分の担当').find('.page-tabs__item-count').text.to_i
	end

	before_comment = assigned_product_count

	[
	'担当者がいない提出物の場合、担当者になる',
	'自分が担当者の場合、担当者のまま',
	'タブ上の数字は未返信の数字のため、返信するとカウントされない'
	].each do \|comment\|
	visit "/products/#{products(:product1).id}"
	post_comment(comment)

	visit products_not_responded_index_path
	assert_equal before_comment, assigned_product_count
	end
	end

	test 'be not person on charge at comment on product of there are person on charge' do
	visit_with_auth products_not_responded_index_path, 'komagata'
	product = find('.thread-list-item', match: :first)
	product.click_button '担当する'
	show_product_path = product.find_link(href: /products/)[:href]
	logout

	visit_with_auth products_not_responded_index_path, 'machida'

	def assigned_product_count
	find_link('自分の担当').find('.page-tabs__item-count').text.to_i
	end

	before_comment = assigned_product_count

	visit show_product_path
	post_comment('担当者がいる提出物の場合、担当者にならない')

	visit products_not_responded_index_path
	assert_equal before_comment, assigned_product_count
	end

「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 #3624

「自分の担当」タブのバッチに表示される件数が０件の場合はバッチを表示しないように実装 #3624

Conversation

AudioStakes commented Nov 25, 2021 • edited Loading

やったこと

画面の変更内容

変更前

変更後

Issue の対応により落ちるようになったテストとその対応

落ちるようになったテスト

テストA

テストB

対応したこと

対応1: テストコードで自分の担当件数として「返信済みも含めたすべての担当件数」を取得するようにした

対応2: テストAで本来確認したいことを確認できるようにするため、 38f2828 をリバートした

AudioStakes commented Nov 26, 2021

haruguchi-yuma left a comment

Choose a reason for hiding this comment

AudioStakes commented Nov 26, 2021

komagata left a comment

Choose a reason for hiding this comment

AudioStakes commented Dec 1, 2021

新たに立てた Issue

ミーティングで質問したこと

回答してもらったこと

AudioStakes commented Dec 2, 2021 • edited Loading

AudioStakes commented Nov 25, 2021 •

edited

Loading

対応2: テストAで本来確認したいことを確認できるようにするため、 `38f2828` をリバートした

AudioStakes commented Dec 2, 2021 •

edited

Loading