お住まいの地域が未登録時はnilが保存され、既存のデータベース内の空文字列はnilへと変換する by SuzukiShuntarou · Pull Request #8446 · fjordllc/bootcamp

SuzukiShuntarou · 2025-03-14T06:55:18Z

Issue

お住まいの地域に空文字列が保存されないようにし、データベース内の空文字列や不正な値をnullへと変換するスクリプトを実行する #7628

概要

変更確認方法

変更確認方法１→変更確認方法２の順番で実施をお願いいたします。
- 変更確認方法１でブラウザからデータベースの更新を実施しているのはテストデータにお住まいの地域『country_codeとsubdivision_code』に空文字列のデータがないため（テストデータに設定がない場合はnilで保存されている）です。
- 空文字列が登録されたユーザデータは本PRのマージ後はnilとなるため、テストデータでは用意せずに変更確認方法１の手順で手動で作成する手順としております。

変更確認方法１：本番環境のデータベース更新の動作確認

mainブランチでforeman start -f Procfile.devでローカル環境を立ち上げる。
ユーザー名 hatsuno パスワード testtest でログインする。
右上のユーザアイコンをクリックし『登録情報変更』をクリックする。
登録情報変更ページから『お住まいの地域（都道府県・州まで）』で『登録しない』を選択し、画面下にある『更新する』ボタンをクリックする。
ログアウトし、ユーザー名 hajime パスワード testtest でログインする。
右上のユーザアイコンをクリックし『登録情報変更』をクリックする。
『お住まいの地域（都道府県・州まで）』で『登録する』を選択し、『国』だけ選択し『都道府県・州』は『選択してください』のまま画面下にある『更新する』ボタンをクリックする。
別ターミナルを起動し、ローカル環境を立ち上げているディレクトリへ移動後にrails cを実行してコンソールを立ち上げる。
コンソール上で以下を入力し確認する。
1.User.where(email: 'hatsuno@fjord.jp')を入力し、country_codeとsubdivision_codeに空文字列が保存されていることを確認する。

2.User.where(email: 'hajime@fjord.jp')を入力し、country_codeには選択した国のコード、subdivision_codeには空文字列が保存されていることを確認する。

3.User.where(email: 'komagata@fjord.jp')を入力し、country_codeとsubdivision_codeにそれぞれ以下のコードが保存されていることを確認する。
bug/fix-empty-string-for-country-code-and-subdivision-codeをローカルに取り込む。
1. git fetch origin bug/fix-empty-string-for-country-code-and-subdivision-code
2. git checkout bug/fix-empty-string-for-country-code-and-subdivision-code

11.rake data:migrate:up VERSION=20250314075905を実施する。

db/data/20250314075905_convert_blank_of_country_code_and_subdivision_code_to_nil.rbのupメソッドをローカル環境で実施するコマンド。
本番環境ではgem data-migrateで実施する。ローカル環境では上記のコマンドで動作確認をする。Wiki
成功するとターミナル上に以下のようなログが出力される。

コンソール上でcountry_codeとsubdivision_codeが正しくnilに変更されていることを確認する
1. User.where(email: 'hatsuno@fjord.jp')はどちらもnil
2. User.where(email: 'hajime@fjord.jp')はsubdivision_codeだけがnil
3. User.where(email: 'komagata@fjord.jp')は変更なし。

変更確認方法２：登録情報の`country_code`と`subdivision_code`の保存時に未登録の場合は`nil`が保存される動作確認

bug/fix-empty-string-for-country-code-and-subdivision-codeをローカルに取り込む。（既に済の場合は省略）
1. git fetch origin bug/fix-empty-string-for-country-code-and-subdivision-code
2. git checkout bug/fix-empty-string-for-country-code-and-subdivision-code
foreman start -f Procfile.devでローカル環境を立ち上げる。
ユーザー名 komagata パスワード testtest でログインする。
右上のユーザアイコンをクリックし『登録情報変更』をクリックする。
別ターミナルを起動し、ローカル環境を立ち上げているディレクトリへ移動後にrails cを実行してコンソールを立ち上げる。
コンソール上で以下を入力しcountry_codeとsubdivision_codeに登録情報が設定されていることを確認する。

User.where(email: 'komagata@fjord.jp')

登録情報（コンソール上）

ブラウザで登録情報変更ページから『お住まいの地域（都道府県・州まで）』で『登録しない』を選択し、画面下にある『更新する』ボタンをクリックする。
再度コンソール上でUser.where(email: 'komagata@fjord.jp')を入力し、country_codeとsubdivision_codeにnilが保存されていることを確認する。
ブラウザで右上のユーザアイコンをクリック後に『登録情報変更』をクリックし『登録情報変更』ページへアクセスする。
『お住まいの地域（都道府県・州まで）』で『登録する』を選択し、『国』だけ選択し『都道府県・州』は『選択してください』のまま画面下にある『更新する』ボタンをクリックする。
コンソール上でUser.where(email: 'komagata@fjord.jp')を実行し、country_codeには選択した国のコード、subdivision_codeにはnilが保存されていることを確認する。
ブラウザで右上のユーザアイコンをクリック後に『登録情報変更』をクリックし『登録情報変更』ページへアクセスする。
『お住まいの地域（都道府県・州まで）』で『登録する』を選択し、『国』と『都道府県・州』を選択して画面下にある『更新する』ボタンをクリックする。
コンソール上でUser.where(email: 'komagata@fjord.jp')を実行し、country_codeには選択した国のコード、subdivision_codeには選択した都道府県・州のコードが保存されていることを確認する。

Screenshot

変更前：コンソール上で確認（画面の変更はなし）

country_codeとsubdivision_codeが共に未登録の場合はともに空文字列
country_codeだけ登録、subdivision_codeは未登録の場合はsubdivision_codeだけ空文字列

変更後：コンソール上で確認（画面の変更はなし）

country_codeとsubdivision_codeが共に未登録の場合はともにnil
country_codeだけ登録、subdivision_codeは未登録の場合はsubdivision_codeだけnil

SuzukiShuntarou · 2025-03-16T01:19:20Z

@mousu-a
お疲れ様です！もし可能であればこちらのissueのレビューをしていただけないでしょうか？🙏
難しい場合は仰ってください。🙇‍♀️どうぞよろしくお願いいたします！

mousu-a · 2025-03-16T01:48:34Z

@SuzukiShuntarou
レビューご依頼ありがとうございますー！
ぜひ引き受けさせていただきます🙆‍♂️
しばしお待ちください🙇‍♂️

mousu-a

@SuzukiShuntarou
お疲れ様です🍵
変更確認方法とてもわかりやすかったです！ありがとうございました😊
二点ほど気になる点にコメントさせていただきました！
ご確認のほどよろしくお願いいたします🙏

mousu-a · 2025-03-16T03:45:34Z

app/models/user.rb

  end
+
+  def convert_blank_of_country_code_to_nil
+    self.country_code = nil if country_code.blank?


提案です！
好みの部分だと思いますが、他のほとんどのメソッドはselfをつけていないので既存の実装に合わせてselfをなくしてもいいかも？と思いました。

ご指摘ありがとうございます！実はこのselfについては自分も悩んでいたところで、実装時に理由までちゃんと調べきれていなかったので助かりました。今回のご指摘を踏まえて調べた内容を共有いたします。

動作

selfを付けない場合、『登録しない』を選択して保存するとバリデーションエラーとなります。

理由

こちらチェリー本の『selfの付け忘れで不具合が発生するケース』が該当となります。（もしお持ちでしたらチェリー本も読んでいただければ分かりやすいかと思います。）結論としては selfを付けない場合は『country_codeというローカル変数として解釈され、ローカル変数への代入と判断される』ことがバリデーションエラーとなる原因みたいです。

一方、if country_code.blank?の場合はモデルの属性のcontry_codeを参照されるため、selfを省略しても問題がなくモデルの属性に対してblank?メソッドを呼び出す形となります。

補足

こちらで似たようなことをしているので参考になりそうです。

返信ありがとうございます！

selfを付けない場合、『登録しない』を選択して保存するとバリデーションエラーとなります。

あーすみません！気づきませんでした🙇‍♂️

実装時に理由までちゃんと調べきれていなかったので助かりました。

そういっていただけると助かります🙏
参考まで詳細にありがとうございます🙇‍♂️
とてもわかりやすく、腹落ちできました！

mousu-a · 2025-03-16T03:53:43Z

test/models/user_test.rb

    assert_equal 2, user.latest_micro_report_page
  end
+
+  test 'convert to nil during saving when country_code is empty string' do


質問です！
このテストと下のテストですが、ストーリーが似通っているので一緒にしてしまってもいいのかなと思いました。（1つのテスト内でcountry_code,subdivision_code どちらもテストする）
何か分けた理由があれば教えていただきたいです🙏

ありがとうございます！
私も当初は１つのテストで実装しようと検討していたのですが、モデル側でメソッドを別々に定義したため、対応するテストも別々にすべきかと思い、分割いたしました。度々恐れ入りますが、アドバイスいただければ嬉しいです🙏

なるほどです。
各メソッドに対応したテストのため対応するテストも別々に、ということですね👀

以下それに関して自分の考えを言語化してみました！ご参考までに🙇‍♂️

個人的には、a_method, b_methodに対するtest #a_method,test #b_methodという形のテストであれば、"対応するテストもそれぞれ用意する"、というロジックがすんなり入ってくるのですが、今回のような場合はどうなんでしょうか🤔

今回の場合はバリデーションで実行されるメソッドのテストで、やっていること(中身)はほとんどシステムテストに近いように思います。なのでパッと見た時に、「メソッドごとに分割せずまとめてしまっても良いのかな？」と感じたのだと思います。

パッと見た時に感じた違和感を言語化してみたのですが、ちょっとハッキリ「コレ！」って確信が持てないです😅
このあたりの判断はkomagataさんにお任せしたいと思います🙇‍♂️

お答えいただきありがとうございました！！🙏

mousu-a · 2025-03-16T04:02:32Z

db/data/20250314075905_convert_blank_of_country_code_and_subdivision_code_to_nil.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class ConvertBlankOfCountryCodeAndSubdivisionCodeToNil < ActiveRecord::Migration[6.1]


勉強になります！！🙏

SuzukiShuntarou · 2025-03-16T12:53:39Z

@mousu-a
早速のレビューありがとうございます！頂いたコメントに直接返信いたしましたので、ご確認をお願いいたします🙏

変更確認方法とてもわかりやすかったです！ありがとうございました😊

動作確認が複雑になってしまったかも？と思っていたので嬉しいです。ありがとうございます！

mousu-a

@SuzukiShuntarou
お疲れ様です🍵
コメント返信させていただきました！

LGTMです✨
疑問点が解消できましたのでこちらからはApproveとさせていただきます〜！

SuzukiShuntarou · 2025-03-17T05:04:06Z

@mousu-a
レビューありがとうございました！私も勉強になりました。

@komagata
メンバーレビューが完了いたしました。レビューをよろしくお願いいたします。

komagata · 2025-03-20T21:07:28Z

app/models/user.rb

+    self.country_code = nil if country_code.blank?
+  end
+
+  def convert_blank_of_subdivision_code_to_nil


この二つ、やっていることは簡単なのに名前が長くて複雑な感じが増しちゃうので１つのメソッド処理したほうがいいように思いました。

@komagata
レビューありがとうございます！一つのメソッドにまとめ、メソッド名も短くいたしました。

SuzukiShuntarou · 2025-03-21T03:38:22Z

@komagata
レビューありがとうございます！ご指摘いただいた箇所を修正いたしました。
ご確認をよろしくお願いいたします。

komagata

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

SuzukiShuntarou self-assigned this Mar 14, 2025

SuzukiShuntarou marked this pull request as ready for review March 16, 2025 01:15

SuzukiShuntarou requested a review from mousu-a March 16, 2025 01:17

mousu-a reviewed Mar 16, 2025

View reviewed changes

mousu-a approved these changes Mar 16, 2025

View reviewed changes

komagata reviewed Mar 20, 2025

View reviewed changes

SuzukiShuntarou added 6 commits March 21, 2025 11:22

country_codeとsubdivision_codeをallow_nilにしバリデーションチェック前に空文字列をnilに変更するようにした

a72cdee

country_codeとsubdivision_codeが空文字列の時に保存時にnilに変換されることを確認するテストを追加

35fe16d

nilに変換することで落ちていたシステムテストを修正した

4d3ba7a

assert_nilでnilとなっていることを確認する方法に変更

62b9c9b

rubocopの指摘箇所を修正した

dc4745e

本番環境のデータベースの空文字列をnilに更新するマイグレーションファイルを作成した

1025a12

SuzukiShuntarou force-pushed the bug/fix-empty-string-for-country-code-and-subdivision-code branch from cbead86 to 1025a12 Compare March 21, 2025 02:22

SuzukiShuntarou added 2 commits March 21, 2025 11:36

空文字列をnilに変換するメソッドを１つのメソッドで処理するようにした

b9c1bc9

空文字列をnilに変換する確認するモデルテストを１つにまとめた

35a1b21

komagata approved these changes Mar 21, 2025

View reviewed changes

komagata merged commit 755208a into main Mar 22, 2025
3 checks passed

komagata deleted the bug/fix-empty-string-for-country-code-and-subdivision-code branch March 22, 2025 21:48

github-actions bot mentioned this pull request Mar 22, 2025

Release 2025-04-02 13:22:58 +0000 #8478

Merged

15 tasks

		@@ -0,0 +1,19 @@
		# frozen_string_literal: true

		class ConvertBlankOfCountryCodeAndSubdivisionCodeToNil < ActiveRecord::Migration[6.1]

Conversation

SuzukiShuntarou commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

概要

変更確認方法

変更確認方法１：本番環境のデータベース更新の動作確認

変更確認方法２：登録情報のcountry_codeとsubdivision_codeの保存時に未登録の場合はnilが保存される動作確認

Screenshot

変更前：コンソール上で確認（画面の変更はなし）

変更後：コンソール上で確認（画面の変更はなし）

Uh oh!

SuzukiShuntarou commented Mar 16, 2025

Uh oh!

mousu-a commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mousu-a left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mousu-a Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

動作

理由

補足

Uh oh!

mousu-a Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mousu-a Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

mousu-a Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mousu-a Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou commented Mar 16, 2025

Uh oh!

mousu-a left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou commented Mar 17, 2025

Uh oh!

komagata Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

SuzukiShuntarou commented Mar 21, 2025

Uh oh!

komagata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SuzukiShuntarou commented Mar 14, 2025 •

edited

Loading

変更確認方法２：登録情報の`country_code`と`subdivision_code`の保存時に未登録の場合は`nil`が保存される動作確認

mousu-a commented Mar 16, 2025 •

edited

Loading

mousu-a left a comment •

edited

Loading

SuzukiShuntarou Mar 16, 2025 •

edited

Loading

mousu-a Mar 16, 2025 •

edited

Loading

mousu-a Mar 16, 2025 •

edited

Loading

mousu-a left a comment •

edited

Loading