都道府県別ユーザー一覧の非React化 #8034

Shrimprin · 2024-08-25T13:25:38Z

Issue

都道府県別ユーザー一覧を非React化したい。 #7873

概要

[都道府県別ユーザー一覧(http://localhost:3000/users/areas)をReactからRails標準(html.slim)に変更しました。

変更確認方法

feature/rewrite-users-by-area-from-react-to-htmlをローカルに取り込む
bin/setupを実行
foreman start -f Procfile.devでローカルサーバを立ち上げ
適当なユーザーでログイン
/users/areasにアクセスして以下を確認する
- サイドメニューが以下スクショのように表示されている
- 都道府県のカードが以下スクショのように表示されている
- サイドメニューの東京都（3）をクリックすると東京都のユーザー一覧に遷移する
- 都道府県のカードの宮城県のタイトルをクリックすると宮城県のユーザー一覧に遷移する
/users/areas/東京都にアクセスして以下を確認する
- サイドメニューが以下スクショのように表示されている
- ユーザーのカードが以下スクショのように表示されている
users/aras/茨城県にアクセスして以下を確認する
- 以下スクショのように「まだユーザーがいません」というメッセージが表示される

変更前

サイドメニューから選択した都道府県のユーザー一覧を表示
都道府県が選択されていない時は東京都を表示

変更後

都道府県一覧

ユーザーの多い順に都道府県の一覧を表示
都道府県カードのタイトルまたはサイドメニューから選択した都道府県のユーザー一覧ページに遷移

選択した都道府県のユーザー一覧

Shrimprin · 2024-08-29T03:05:10Z

@machida
お疲れ様です！
デザインの依頼をさせていただけますでしょうか 🙏
CSSクラスはReact版のものを流用していますが、手直しする必要があればご対応をお願いいたします。
ご確認いただきたいのは以下の2画面です。

よろしくお願いいたします 🙇‍♂️

machida · 2024-08-30T08:45:00Z

@Shrimprin

確認しました！！
バッチリでした。
このままこれでリリースしたいと思います。
ほんの少しだけいじったものをpushしました。

実データを反映させたら、また新たな問題が見つかるかもしれませんが（東京が画面を埋め尽くすなど）、それは別Issueで対応していきたいと思いますー。

Shrimprin · 2024-08-30T10:24:31Z

@machida
ご確認と修正ありがとうございます！
このままメンバーレビューへと進めさせていただきます〜

Shrimprin · 2024-08-30T10:37:36Z

@goruchanchan
お疲れ様です。
本PRのレビューをお願いできますでしょうか 🙏
もしご都合が悪ければ遠慮なくおっしゃってください！

よろしくお願いいたします 🙇‍♂️

goruchanchan · 2024-08-30T21:03:14Z

@Shrimprin レビュー依頼ありがとうございます！ぜひやらせていただきます！一週間程度で確認しますので今しばらくお待ちください🙇‍♂️

Shrimprin · 2024-09-01T05:20:13Z

@goruchanchan
ありがとうございます！よろしくお願いいたします 🙏

goruchanchan

@Shrimprin

お疲れ様です！レビュー致しました🙇‍♂️ご確認よろしくお願いします🙇‍♂️

goruchanchan · 2024-08-31T23:08:47Z

app/models/area.rb

-      if region == '海外'
-        country = ISO3166::Country.find_country_by_any_name(area)
+    def users_by_area(area)
+      if (subdivision = ISO3166::Country[:JP].find_subdivision_by_name(area))


条件式での代入は == と勘違いしそうで避けた方がいいじゃないかと思いました🙇‍♂️

goruchanchan · 2024-09-01T00:32:42Z

app/models/area.rb

@@ -62,6 +59,17 @@ def number_of_users_by_region
      end
    end

+    def sorted_users_group_by_areas


sorted が指すのは「エリア毎のユーザ数」だと思うのですが、created_at とどっちを指すのかがわかりにくいと思いました。
また、返すのは「複数のgroup」だと思うので sourted_user_groups_by_area_user_num はどうでしょうか？

また、ユーザを取得する段階で created_at でソートしてしまってもいいんじゃないかなと思いました

def sourted_user_groups_by_area_user_num users_grouped_by_areas = User.with_attached_avatar.order(created_at: :desc).group_by(&:area) users_grouped_by_non_nil_area = users_grouped_by_areas.map do |area, users| { users:, area: } unless area.nil? end.compact users_grouped_by_non_nil_area.sort_by { |hash| -hash[:users].size } end

sorted が指すのは「エリア毎のユーザ数」だと思うのですが、created_at とどっちを指すのかがわかりにくいと思いました。
また、返すのは「複数のgroup」だと思うので sourted_user_groups_by_area_user_num はどうでしょうか？

ご指摘ありがとうございます。
ご提案いただいた変数名の方がい理解しやすそうなので修正します。

また、ユーザを取得する段階で created_at でソートしてしまってもいいんじゃないかなと思いました

そのほうがコードがスッキリしそうですね！修正します。

goruchanchan · 2024-09-01T00:34:09Z

app/models/area.rb

-    def users(region, area)
-      if region == '海外'
-        country = ISO3166::Country.find_country_by_any_name(area)
+    def users_by_area(area)


ユーザを返すならユーザモデルの方が適当なのかと思ったんですが、いかがでしょうか？

おっしゃる通りですね！
Userモデルに移します。

goruchanchan · 2024-09-02T14:09:27Z

app/controllers/users/areas_controller.rb

+  end
+
+  def show
+    @area = params[:area_name]


いくつか area_name が登場すると思うのですが、 area ではダメなのでしょうか？

Shrimprin · 2024-09-03T23:20:31Z

コメントいただきありがとうございます！
以下5点を修正させていただきます。

条件式での代入をやめる
sorted_users_group_by_areas -> sourted_user_groups_by_area_user_numに修正
上記メソッドで、users取得時にcreated_atでソートする
users_by_areaをUserモデルに移す
パラメータarea_name -> areaに修正

Shrimprin · 2024-09-09T03:24:41Z

@goruchanchan
お疲れ様です！
コメントいただいた内容を修正しましたので再度ご確認をお願いできますでしょうか 🙏

goruchanchan

@Shrimprin お疲れ様です！ご対応ありがとうございます！細かなところで恐縮ですが２点ほど確認いただきたいです🙇‍♂️

goruchanchan · 2024-09-10T13:34:36Z

app/views/users/areas/_areas_menu.html.slim

+      ul.page-nav__items
+        - number_of_users_by_region[region].each do |area, number_of_users|
+          li.page-nav__item
+            = link_to "#{area}（#{number_of_users}）", users_area_path(area: area), class: 'page-nav__item-link a-text-link'


細かなところで恐縮ですが省略形で書けませんか？

Suggested change

= link_to "#{area}（#{number_of_users}）", users_area_path(area: area), class: 'page-nav__item-link a-text-link'

= link_to "#{area}（#{number_of_users}）", users_area_path(area:), class: 'page-nav__item-link a-text-link'

goruchanchan · 2024-09-10T13:51:56Z

app/controllers/users/areas_controller.rb

+
+  def show
+    @area = params[:area]
+    @users = User.users_by_area(@area).page(params[:page]).per(15)


User.users_by_area(@area) の呼び出し方を見ると下記ようなメソッド名でもいいかと思いました🙇‍♂️あくまで個人的にそう感じただけなので参考程度と思っていただければと思います🙇‍♂️

Suggested change

@users = User.users_by_area(@area).page(params[:page]).per(15)

@users = User.by_area(@area).page(params[:page]).per(15)

Shrimprin · 2024-09-10T22:54:18Z

@goruchanchan
お疲れ様です！
追加のコメントありがとうございます。
2点対応しましたので確認お願いできますでしょうか 🙏

goruchanchan

@Shrimprin お疲れ様です！LGTMと思いますので承認いたします！非React化とても勉強になりました！ありがとうございました🙏

Shrimprin · 2024-09-11T13:54:48Z

@goruchanchan
お疲れ様です！確認ありがとうございます。
こちらこそ命名や簡潔な書き方などいろいろとコメントいただけて勉強になりました～
引き続きよろしくお願いいたします 🙏

Shrimprin · 2024-09-11T13:55:25Z

@komagata
お疲れ様です。
メンバーからApproveいただきましたのでレビューをお願いできますでしょうか 🙏

komagata · 2024-09-12T16:47:49Z

app/controllers/users/areas_controller.rb

@@ -3,5 +3,12 @@
 class Users::AreasController < ApplicationController
  def index
    @number_of_users_by_region = Area.number_of_users_by_region
+    @sorted_user_groups_by_area_user_num = Kaminari.paginate_array(Area.sorted_user_groups_by_area_user_num).page(params[:page]).per(15)


15

マジックナンバーになっているので定数か変数にしてみてください。
他の場所のコードを参考にしてみてください〜。

komagata · 2024-09-12T16:49:25Z

test/models/user_test.rb

@@ -730,4 +730,20 @@ class UserTest < ActiveSupport::TestCase
  test '.users_job returns all users when invalid job is passed' do
    assert_equal User.all, User.users_job('destroy_all')
  end
+
+  test 'area' do


Suggested change

test 'area' do

test '#area' do

Shrimprin · 2024-09-13T09:41:03Z

@komagata
レビューありがとうございます！
コメントいただいた２点を修正しましたので再度ご確認をお願いいたします 🙏

komagata · 2024-09-18T04:53:01Z

app/views/users/areas/_areas_menu.html.slim

@@ -0,0 +1,10 @@
+- number_of_users_by_region.each do |region, _|


Suggested change

- number_of_users_by_region.each do |region, _|

- number_of_users_by_region.each do |region|

これは不要かも？

コードを確認したところ、このviewの後の処理でハッシュのvalueを使用しているため、ハッシュのブロック変数名を省略せずに記述するようにしました。

@Shrimprin コードの都合でそのように利用しているのだと思いますが、こうならないように書いた方が良いと思います。

「そもそものデータ構造を変える」「別のメソッドを用意する」などして他のプログラマーが見てもわかりやすいようにするのがいいと思います。

komagata · 2024-09-18T04:53:54Z

app/views/users/areas/_user_list.html.slim

+      - users.each do |user|
+        = render 'users/user', user:


Suggested change

- users.each do |user|

= render 'users/user', user:

= render users

こう書けた気がします。

users/userはレンダー元のviewファイルと別のディレクトリにあり短縮形が使えないため、非省略形のコレクションのレンダーで記述しました。

komagata · 2024-09-27T06:26:26Z

@Shrimprin コンフリクトの修正をお願いします。

- 既存コードの削除は未実施 - ページネーションは未実施

- メソッドが返すのはエリアごとに分けられた「複数のグループ」のため、メソッド名をわかりやすく修正 - メソッド名の変更に伴い、テストを修正 - メソッド名の変更に伴い、コントローラーからの呼び出しを修正 - ユーザー取得時にソートするように修正

- user_by_areaメソッドはusersを返すためUserモデルに移動 - テストやコントローラを合わせて修正

Shrimprin · 2024-09-28T13:59:07Z

@komagata
コンフリクトを修正しました。
ご確認よろしくお願いいたします 🙏

komagata · 2024-10-01T18:08:32Z

test/models/user_test.rb

+    assert_equal no_area_user.area, nil
+  end
+
+  test '#by_area' do


Suggested change

test '#by_area' do

test '.by_area' do

クラスメソッドなのでこうかもです。

komagata

,

komagata · 2024-10-02T12:11:37Z

@Shrimprin テストが落ちているようです。

Shrimprin · 2024-10-02T14:21:07Z

@komagata
ご確認ありがとうございます！
テストがパスしましたのでご報告します。

komagata

確認させて頂きました。OKです〜🙆‍♂️

Shrimprin · 2024-10-07T08:19:10Z

@komagata
ありがとうございます！

Shrimprin changed the title ~~都道府県ユーザー一覧の非React化~~ 都道府県別ユーザー一覧の非React化 Aug 25, 2024

Shrimprin force-pushed the feature/rewrite-users-by-area-from-react-to-html branch from 637446a to 2422cbb Compare August 28, 2024 14:37

Shrimprin assigned machida and Shrimprin Aug 29, 2024

machida removed their assignment Aug 30, 2024

Shrimprin marked this pull request as ready for review August 30, 2024 10:24

Shrimprin requested a review from goruchanchan August 30, 2024 10:36

goruchanchan reviewed Sep 2, 2024

View reviewed changes

Shrimprin force-pushed the feature/rewrite-users-by-area-from-react-to-html branch from 775cd7a to d3d4e9f Compare September 8, 2024 12:55

goruchanchan reviewed Sep 10, 2024

View reviewed changes

goruchanchan approved these changes Sep 11, 2024

View reviewed changes

Shrimprin requested a review from komagata September 11, 2024 13:55

komagata reviewed Sep 12, 2024

View reviewed changes

Shrimprin force-pushed the feature/rewrite-users-by-area-from-react-to-html branch from 132f6b4 to d1ab442 Compare September 13, 2024 09:15

komagata reviewed Sep 18, 2024

View reviewed changes

Shrimprin force-pushed the feature/rewrite-users-by-area-from-react-to-html branch from d1ab442 to 18ad9c4 Compare September 23, 2024 13:53

地域別ユーザーの一覧画面の非React版を作成

27c14a8

- 既存コードの削除は未実施 - ページネーションは未実施

Shrimprin and others added 16 commits September 28, 2024 15:54

既存テストと干渉するため、areaを持たせるfixtureを変更

8b7573a

areaから移動したメソッドのモデルテストを作成

969cac3

条件式での代入を修正

e6b1325

users_by_areメソッドをUserモデルのメソッドに移動

5c95c0b

- user_by_areaメソッドはusersを返すためUserモデルに移動 - テストやコントローラを合わせて修正

パラメータ名を簡潔に修正

65ba475

不要なルーティングを削除

157cca8

ユーザーグループ分けの一覧のカードを別々にした

92aea10

パラメータ名の修正漏れを対応

8267465

省略形に修正

49f1b0f

メソッド名が簡潔になるように修正

ec163fd

マジックナンバーを定数化

6b39ba1

テスト名を修正

872b14a

コレクションのレンダーを一括で行うように修正

0f7441e

ブロック変数を修正し、number_of_users_by_regionへの不要なアクセスを廃止

8918329

タブをTab Componentに対応

b1a3e89

Shrimprin force-pushed the feature/rewrite-users-by-area-from-react-to-html branch from 18ad9c4 to b1a3e89 Compare September 28, 2024 07:56

komagata reviewed Oct 1, 2024

View reviewed changes

クラスメソッドのテストのため、命名を「.」に修正

8c00ccd

komagata approved these changes Oct 2, 2024

View reviewed changes

komagata self-requested a review October 2, 2024 12:11

komagata approved these changes Oct 6, 2024

View reviewed changes

komagata merged commit 8c1405c into main Oct 6, 2024
4 checks passed

komagata deleted the feature/rewrite-users-by-area-from-react-to-html branch October 6, 2024 17:46

github-actions bot mentioned this pull request Oct 6, 2024

Release 2024-10-16 10:46:56 +0000 #8124

Merged

7 tasks

Ryooo-k mentioned this pull request Dec 19, 2024

user-iconを非React化した #8255

Open

	= link_to "#{area}（#{number_of_users}）", users_area_path(area: area), class: 'page-nav__item-link a-text-link'
	= link_to "#{area}（#{number_of_users}）", users_area_path(area:), class: 'page-nav__item-link a-text-link'

	@users = User.users_by_area(@area).page(params[:page]).per(15)
	@users = User.by_area(@area).page(params[:page]).per(15)

		@@ -0,0 +1,10 @@
		- number_of_users_by_region.each do \|region, _\|

	- users.each do \|user\|
	= render 'users/user', user:
	= render users

都道府県別ユーザー一覧の非React化 #8034

都道府県別ユーザー一覧の非React化 #8034

Conversation

Shrimprin commented Aug 25, 2024 • edited by goruchanchan Loading

Issue

概要

変更確認方法

変更前

変更後

都道府県一覧

選択した都道府県のユーザー一覧

Shrimprin commented Aug 29, 2024

machida commented Aug 30, 2024 • edited Loading

Shrimprin commented Aug 30, 2024

Shrimprin commented Aug 30, 2024

goruchanchan commented Aug 30, 2024

Shrimprin commented Sep 1, 2024

goruchanchan left a 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

Shrimprin commented Sep 3, 2024 • edited Loading

Shrimprin commented Sep 9, 2024

goruchanchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shrimprin commented Sep 10, 2024

goruchanchan left a comment

Choose a reason for hiding this comment

Shrimprin commented Sep 11, 2024

Shrimprin commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shrimprin commented Sep 13, 2024

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

komagata commented Sep 27, 2024

Shrimprin commented Sep 28, 2024

Choose a reason for hiding this comment

komagata left a comment • edited Loading

Choose a reason for hiding this comment

komagata commented Oct 2, 2024

Shrimprin commented Oct 2, 2024

komagata left a comment

Choose a reason for hiding this comment

Shrimprin commented Oct 7, 2024

Shrimprin commented Aug 25, 2024 •

edited by goruchanchan

Loading

machida commented Aug 30, 2024 •

edited

Loading

Shrimprin commented Sep 3, 2024 •

edited

Loading

komagata left a comment •

edited

Loading