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.

Sign up for GitHub

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

Jump to bottom

タグ別のユーザーページにおいて「休会」と「退会」ユーザーが表示されない仕様に統一 #7089

Merged
merged 3 commits into from
Dec 31, 2023
Merged

タグ別のユーザーページにおいて「休会」と「退会」ユーザーが表示されない仕様に統一 #7089

merged 3 commits into from
Dec 31, 2023

Conversation

2525nicole
Copy link
Contributor

@2525nicole 2525nicole commented Nov 30, 2023 •
edited

Issue

概要

以下のタグ別ユーザーページにおいて「退会」と「休会」のユーザーが表示されない仕様に統一

変更確認方法

  1. bug/exclude-hibernated-and-retired-users-from-users-list-by-tagをローカルに取り込む
  2. foreman start -f Procfile.devを実行し、ローカル環境を立ち上げる
  3. localhost:3000にアクセス
  4. hajimeでログイン
  5. http://localhost:3000/users/tags/.NET_Frameworkにアクセスし、ページ右側「このタグを自分に追加」をクリック

スクリーンショット 2023-11-30 20 41 12

  1. 以下ページから休会または退会手続きを行う
  1. 任意のユーザーでログインする
  2. タグ別ユーザー一覧にアクセスし、以下二点を確認する
  • hajimeがユーザー数に含まれていないこと
  • hajimeのアイコンが表示されていないこと

スクリーンショット 2023-11-30 20 53 24

  1. タグ「.NET_Framework」のユーザーページにアクセスし、以下二点を確認する
  • hajimeがユーザー数に含まれていないこと
  • hajimeがユーザーとして表示されていないこと

スクリーンショット 2023-11-30 20 49 24

Screenshot

変更前

タグ別ユーザー一覧で「休会」または「退会」ユーザーもユーザー数に含まれ、アイコンが表示されている

スクリーンショット 2023-11-30 21 02 38

タグ「.NET_Framework」のユーザーページではユーザー数には含まれないが、ユーザー情報は表示されている

スクリーンショット 2023-11-30 21 05 51

変更後

タグ別ユーザー一覧で「休会」または「退会」ユーザーはユーザー数に含まず、アイコンも表示されない

スクリーンショット 2023-11-30 21 15 21

タグ「.NET_Framework」のユーザーページでもユーザー数には含まれず、ユーザー情報も表示されない

スクリーンショット 2023-11-30 21 09 12

@2525nicole 2525nicole self-assigned this Nov 30, 2023
@2525nicole 2525nicole changed the title Bug/exclude hibernated and retired users from users list by tag タグ別のユーザーページにおいて「休会」と「退会」ユーザーが表示されない仕様に統一 Nov 30, 2023
@2525nicole
Copy link
Contributor Author

タグ別のユーザーページにおいて「休会」と「退会」ユーザーが表示されない仕様に統一 by 2525nicole · Pull Request #7088 · fjordllc/bootcampのブランチ名を修正することにしたため、こちらはクローズいたします。

@2525nicole 2525nicole closed this Nov 30, 2023
@2525nicole 2525nicole deleted the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch November 30, 2023 12:49
@2525nicole 2525nicole restored the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch November 30, 2023 12:49
@2525nicole 2525nicole deleted the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch November 30, 2023 12:57
@2525nicole 2525nicole restored the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch November 30, 2023 12:57
@2525nicole 2525nicole reopened this Nov 30, 2023
@2525nicole 2525nicole marked this pull request as ready for review November 30, 2023 13:06
@2525nicole
Copy link
Contributor Author

@dowdiness
※ブランチ名の誤りがあり、再度のご連絡失礼いたします。
おつかれさまです！
先日はお話をさせていただきありがとうございました🙇‍♀️
（結果的に今回のIssueではVueファイルはほとんど変更を加えないかたちになりました＞＜）
急ぎではありませんので、もしご都合がよろしければレビューをお願いできますでしょうか🙏

@2525nicole 2525nicole removed the request for review from dowdiness December 23, 2023 00:34
@2525nicole
Copy link
Contributor Author

@dowdiness さんにご連絡し、別の方にレビュー依頼をお願いすることにいたしました🙇‍♀️
また機会がありましたらどうぞよろしくお願いいたします😊🙏

@2525nicole
Copy link
Contributor Author

@junohm410
おつかれさまです！nicole（ニコル）と申します🙇‍♀️
急ぎではありませんので、ご都合のよろしいタイミングでレビューをお願いできますでしょうか🙏
ご対応が難しい場合はご教示いただけますと幸いです💡
どうぞよろしくお願いいたします！

@junohm410
Copy link
Contributor

@2525nicole
お疲れ様です👍レビュー承知いたしました。
1週間以内にお返事できるようにいたします🙏よろしくお願いいたします！

@2525nicole
Copy link
Contributor Author

@junohm410
早速のご確認とお引き受けありがとうございます🙇‍♀️！！
お手数をおかけいたしますが、どうぞよろしくお願いいたします😊🌷

@junohm410
Copy link
Contributor

junohm410 commented Dec 25, 2023 •
edited

@2525nicole
お疲れ様です🤝レビューのためコードを拝読していたのですが、

本PRのapp/controllers/api/users_controller.rbへの変更が、↑のPRで修正された現在のorigin/mainをベースにしたものなっていないように見受けられるので、お手数ですがまずはご確認いただいてもよろしいでしょうか？🙏

@2525nicole 2525nicole force-pushed the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch from 780d5ea to 07f4333 Compare December 25, 2023 13:18
@2525nicole
Copy link
Contributor Author

@junohm410
おつかれさまです🤝
お忙しいところ早速のご確認ありがとうございます🙇‍♀️！！

今回レビューをご依頼する前に改めてmainをrebaseするべきでした...申し訳ありません！
また丁寧にご教示いただきありがとうございます🙇‍♀️
ご指摘のとおりでしたのでmainの変更を取り込みましたので、
再度ご確認をお願いできますでしょうか🙏
お手数をおかけしますがよろしくお願いいたします🍀

junohm410
junohm410 reviewed Dec 27, 2023
View reviewed changes
Copy link
Contributor

@junohm410 junohm410 left a comment

Choose a reason for hiding this comment

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

お疲れ様です。
記載いただいた変更確認方法で、動作確認できました🤝
一点コメントさせていただきましたので、ご確認いただけますと幸いです🙏

test/system/user/tags_test.rb
visit_with_auth users_tags_path, 'kensyu'
displayed_users_number = find('span.user-group__title-label', text: 'ギター').sibling('span.user-group__count').text.scan(/\d+/).first
assert_equal '2', displayed_users_number
assert_selector ".a-user-icons__items img[title='#{user.login_name} (#{user.name})']"
Copy link
Contributor

@junohm410 junohm410 Dec 27, 2023 •
edited

Choose a reason for hiding this comment

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

users/tagsページ上のユーザーアイコンの検索・アサーションでは、「ターゲットとなるユーザーが、このページ上の他のタグにも属している」という可能性を考えると、withinなどで「ギター」タグのユーザー一覧の中にスコープを絞ってアイコン検索するようにした方がいいかなと考えましたが、いかがでしょうか？👀

Copy link
Contributor Author

@2525nicole 2525nicole Dec 28, 2023 •
edited

Choose a reason for hiding this comment

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

@junohm410
※お先にコメントのご返信のみ失礼いたします＞＜！
CircleCIが通りましたら別途「ご確認をお願いいたします」とご連絡しますので、
その後確認をいただけますと幸いです🙏

ご提案ありがとうございます🙇‍♀️
HTMLの構造上、特定のタグ名とタグに属するアイコンに共通するセレクタを指定するのが難しそうに感じたのでアプローチを変更いたしました🙇‍♀️

ser.tags.where.not(name: tag_name).destroy_allによりアプリ内で「ギター」タグのみを残すことで、
「ギター」タグのみに対して表示を確認できるようにしております🙏

ここでテストしたいのはタグ別ページにおいて「休会と退会ユーザーが表示されなくなること」なので、
ターゲットとなる「ギター」以外のタグをページに残した状態でテストを行う必要がない（=kensyuが登録している「ギター」タグのみ表示し、テストできれば良い）のではないかと判断いたしました🙏

参考にさせていただいた記事：
【minitest / RSpec】繰り返し表示されている要素の中から「これ」という1件を選んで検証する方法あれこれ #Rails - Qiita

Copy link
Contributor

Choose a reason for hiding this comment

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

ご修正ありがとうございます！
ご説明やURLを拝読しあらためて実際のHTML要素もみたところ、withinでは解決しにくい問題でしたね🙇‍♂️こちらはすみませんでした🙏

ギタータグ以外のタグ登録を消去して、今回検証したい「ギター」に対し、kensyuが属するか否かを確実に確認するという方針について、把握しました！
大変勉強になりました🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junohm410
ご確認ありがとうございます🙇‍♀️
とんでもないです！！HTML要素まで見ていただき恐れ入ります🙏
「対象を絞る」という視点が不足していたので、大事な点をご教示いただきこちらこそ大変勉強になりました！
ありがとうございます🙇‍♀️

@2525nicole 2525nicole force-pushed the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch from 07f4333 to c8f7838 Compare December 28, 2023 13:29
@2525nicole
Copy link
Contributor Author

@junohm410
お忙しいところ早速のご確認ありがとうございます🙇‍♀️
ご提案いただいた内容をもとにコードを変更いたしましたので、
ご確認をお願いできますでしょうか🙏
たびたびお手数をおかけいたしますが、どうぞよろしくお願いいたします🍀

@junohm410
Copy link
Contributor

@2525nicole
お疲れ様です。ご修正ありがとうございました（大変勉強になりました🙇‍♂️）！
私の方からApproveとさせていただきます🙏

@2525nicole
Copy link
Contributor Author

@junohm410
おつかれさまです☀️
お忙しいところ早速ご対応をいただきありがとうございました🙇‍♀️🙇‍♀️🙇‍♀️
自分だけでは不足していた視点についてご教示いただき大変勉強になりました🙌
ありがとうございます🙏✨

引き続きどうぞよろしくお願いいたします🍀

@komagata
おつかれさまです！
チームメンバーの方にApproveをいただきましたので、レビューをお願いできますでしょうか🙏

komagata
komagata approved these changes Dec 31, 2023
View reviewed changes
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 cbe4137 into main Dec 31, 2023
6 checks passed
@komagata komagata deleted the bug/exclude-hibernated-and-retired-users-from-users-list-by-tag branch December 31, 2023 07:58
This was referenced Dec 31, 2023
Merged
Merged
@github-actions github-actions bot mentioned this pull request Jan 13, 2024
Merged
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@komagata komagata komagata approved these changes

@junohm410 junohm410 junohm410 approved these changes

Assignees

@2525nicole 2525nicole

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@2525nicole @junohm410 @komagata