ユーザーIDを3文字以上に限定する（n > 1） #6443

YukiWatanabe824 · 2023-04-14T05:13:02Z

Issue

ユーザーIDを3文字以上に限定する #6318

概要

現在のユーザーアカウント名(User.login_name)は文字数の下限がないため1文字のアカウント名も可能な状態となっている。
@xといった一文字ユーザーアカウント名はサジェスト表示が機能しないなど機能提供に支障が生じている。
そのためユーザーアカウント名は3文字以上との制限を追加するとともに、既存ユーザーの中で2文字以下のユーザーアカウント名には0を3文字以上になるように追加する。

ユーザーアカウント名の下限設定はモデルに対してバリデーションでminimumを設定した。また、ユーザーアカウント名への0追加はdata-migrate gemを使用して追加する処理をおこなう。

変更確認方法

任意のユーザーのアカウント名を2文字以下に修正(検証用ユーザーの作成)
feature/limit_userid_3characters_or_moreをローカルに取り込む
rake data:migrate:up VERSION=20230412211754を実行
作成したユーザーのユーザーアカウント名に「0」が追加され3文字以上になっていることを確認する
3文字以上になったユーザーアカウント名を編集して2文字以下にしようとするがアラートが出て変更できないことを確認する
http://localhost:3000/users/newにアクセスしてアカウント名の制限が箇条書きになっていることを確認する

今回のブランチを取り込むとバリデーションがかかるため先に検証用ユーザーを作ってしまうと楽です。
もし先にブランチを取り込んだ場合は別のブランチに移動してから検証用ユーザーを作成してください。

Screenshot

変更前

変更後

YukiWatanabe824 · 2023-04-14T06:43:51Z

@ogawa-tomo
お疲れさまです。
こちらレビューをお願いしたいのですがいかがでしょうか？

ogawa-tomo · 2023-04-14T11:10:09Z

@YukiWatanabe824
レビュー依頼ありがとうございます:raised_hands:
週末に見てみます！

ogawa-tomo

見ました！気になったことをコメントしてますが、私の見る限り方針よさそうに思います～

ogawa-tomo · 2023-04-15T00:57:37Z

db/data/20230412211754_change_user_id_more_3_characters.rb

+  def up
+    User.where('LENGTH(login_name) < ?', 3).find_each do |user|
+      user.login_name = user.login_name.ljust(3, '_')
+      user.save!(validate: false)


[memo]
既存のユーザを更新するときには、(validate: false)をつけておく（すでにinvalidなユーザーデータがあるので）
参考→#5959 (comment)

ogawa-tomo · 2023-04-15T01:16:26Z

db/data/20230412211754_change_user_id_more_3_characters.rb

+class ChangeUserIdMore3Characters < ActiveRecord::Migration[6.1]
+  def up
+    User.where('LENGTH(login_name) < ?', 3).find_each do |user|
+      user.login_name = user.login_name.ljust(3, '_')


アカウント名の補完にあえて禁止文字であるアンダーバーを用いている狙いって、ユーザーにユーザー名の変更を促すためってことで合ってますかね？
（変更後にログインすると警告が出っぱなしになる）

ついでに言うと、この警告はここにべたっと書かれてるみたいなんですが、書いてある通り「半角英数字と-（ハイフン）以外の文字が使われていないか」ぐらいのチェックしかできてないみたいですね。（正規表現をちゃんと読み解けていませんが…少なくとも、3文字未満のときのチェックはしていないことを確認しました）

bootcamp/app/views/application/_flash.html.slim

Lines 18 to 25 in ae5beff

- if current_user && current_user.login_name !~ /\A[a-z\d](?:[a-z\d]|-(?=[a-z\d]))*\z/i

input.a-toggle-checkbox#hide-flash(type='checkbox')

.flash.is-alert

.container

.flash__container

label.flash__close(for='hide-flash')

p.flash__message

| アカウント名に半角英数字と-（ハイフン）以外の文字が使われています。#{link_to '変更', edit_current_user_path}をお願いします。

もしもユーザーに変更を促すことが目的なら、わざわざDBのデータを変更せずにこっちで3文字未満のとき警告するように変更するだけでもいいのかなと思いました。

このPRは変更する必要ないと思いますが、このへんの背景は駒形さんにお聞きするとよいのですかね？

警告が出っぱなしになるのはユーザーにアカウント名変更を促すため、と私も勝手に思い込んでいました💦

@komagata
お疲れさまです。
こちらのIssueですが、起票されたとおりにアンダーバーを追加する修正をすると、修正対象のユーザーは↑の警告が常時表示される状態になります。
ユーザー名変更を促す効果はあると思いますが、リリースとセットでお知らせを流すなりの対応が必要かと思います。いかがいたしましょうか？

4/20のチーム開発MTGで解決しました🙆🏻‍♂️

ogawa-tomo · 2023-04-15T01:27:22Z

test/models/user_test.rb

@@ -196,6 +196,8 @@ class UserTest < ActiveSupport::TestCase
    assert user.invalid?
    user.login_name = '１２３４５'
    assert user.invalid?
+    user.login_name = 'あい'


アカウント名にはそもそも平仮名は使えないので、「アカウントが3文字未満ではいけない」ことだけを検証するテストケースとしては適さないかと思いました。アルファベット2文字にするとよいと思います～

ありがとうございます。
修正いたします！

ogawa-tomo · 2023-04-15T01:30:28Z

test/models/user_test.rb

@@ -196,6 +196,8 @@ class UserTest < ActiveSupport::TestCase
    assert user.invalid?
    user.login_name = '１２３４５'
    assert user.invalid?
+    user.login_name = 'あい'
+    assert user.invalid?
  end


ここのテストケース、test 'is valid username'というブロックなのに不正なアカウント名に対するテストがずらっと書かれてるのが気になりました。（ついでに言うと、正確にはusernameじゃなくてlogin_nameに関するテストですよね）

これもこのPRではスコープ外と思いますしこのままでいいと思いますが、駒形さんに相談して起票するとよいのですかね？

確かに違和感のある名称ですね。
私としてはこのIssueで触れている箇所なので修正してしまってもいいかと思います！やってみます。

スコープ外の修正や改善案は基本的には駒形さん、町田さんに相談すると良いですね。
プルリクかIssueからメンション送るか、Discordの「#チーム開発」か「#バグ、誤字脱字報告、機能要望🙇」から相談する流れです。

ogawa-tomo · 2023-04-15T01:50:16Z

app/views/users/form/_login_name.html.slim

@@ -8,3 +8,4 @@
      | 半角英数字と-（ハイフン）のみが使用できます。
      | 先頭と最後にハイフンを使用することはできません。
      | ハイフンを連続して使用することはできません。
+      | 3文字以上でなければいけません。


（このPRじゃなくていいと思いますが）ちょっと整理したい気はしますね:eyes:（禁止事項は箇条書きにするとか）

このPR↓でどばっと足されてから3年くらいそのままみたいですね。
#1163

確かに箇条書きで整理したいですね👀

@machida
こちらですが、アカウント作成時のアカウント名の禁止事項を追加したことに伴って、禁止事項を箇条書きにしてはどうかと案が出ています。イメージは以下です。

デザインが関わる部分ですが、どのようにすすめるのが良いでしょうか？

@YukiWatanabe824
こちら、僕がコミットしますー
なので、こういうデザインについて関わる作業があるときは、このPRのアサインに machida を追加しておいてくださいー

ogawa-tomo · 2023-04-15T07:01:06Z

[nits]
タイトルの「（n > 1）」ってなんだろう？と思っていたのですが、制限を何文字にするかが決まっていなかったときの名残ですね:eyes:
なのでこれは消しちゃっていいですね。

#6318

YukiWatanabe824 · 2023-04-16T02:40:50Z

@ogawa-tomo
お疲れさまです！
丁寧に見ていただいてありがとうございます！
レビュー参考になります🙇🏻

レビューのやり方でちょっとお聞きしたいのですが、
かなり過去のプルリクまでさかのぼって関連情報を探されてますが、関連情報の探しかたってどのようにやられてますか？
このプラクティスに入ってから情報量に圧倒されがちで目的の情報にたどり着くまで時間がかかることが多い状態でして💦

ogawa-tomo · 2023-04-16T07:29:15Z

@YukiWatanabe824

かなり過去のプルリクまでさかのぼって関連情報を探されてますが、関連情報の探しかたってどのようにやられてますか？

自分はVSCodeを使っているのですが、GitLensという拡張機能を入れています！
https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens
これがけっこう便利で、行にマウスホバーするとその行の最終コミットやPRの情報を教えてくれます。今回もこれでPRを見つけました。

RubyMineは使ったことがないのですが、たぶん同じような機能があるんじゃないかと思います。

このプラクティスに入ってから情報量に圧倒されがちで目的の情報にたどり着くまで時間がかかることが多い状態でして💦

それはめちゃめちゃ分かります～少し変更するだけでも色々調べなきゃってなったりしますし。

YukiWatanabe824 · 2023-04-18T22:13:05Z

@ogawa-tomo
情報ありがとうございます！

GitLensめちゃくちゃ便利そうですね👀
ずっとvimでプラクティスを進めてきてVScodeが素に近いので参考になります💪
さっそくGitLens入れてみました☺️

machida · 2023-04-19T05:36:37Z

@YukiWatanabe824 コミットしておきましたー

YukiWatanabe824 · 2023-04-19T11:18:13Z

@machida
了解です！対応ありがとうございます！

ogawa-tomo

こちら、私のレビューは継続しているという認識でいいですかね…？:eyes:（まだapproveしていないので）

イシューの内容と直接関係ない部分でたいへん恐縮ですが、テスト名が気になったのでコメントしておきました:bow:

ogawa-tomo · 2023-04-20T11:26:34Z

test/models/user_test.rb

@@ -170,7 +170,7 @@ class UserTest < ActiveSupport::TestCase
    assert user.save(context: :retire_reason_presence)
  end

-  test 'is valid username' do
+  test 'is valid unsupportable login_name' do


まず、usernameをlogin_nameに変えてくださってありがとうございます。bootcampでは「ユーザーの名前」を表す概念は複数あるので、正確にしたほうがよいと思っていました。

ただ、is valid unsupportable login_nameというテスト名は分かりにくいと思いました。これは、「validなログイン名とunsupportableなログイン名の両方をテストしてますよ」という意図ですかね…？少なくとも、「有効である」を意味する単語と「無効である」を意味する単語は対にしたほうがよいと思います。つまり、validとinvalidにするか、supportableとunsupportableにするか、です。

もともと私がこのコメントで意図していたのは、validなログイン名に対するテストとinvalidなログイン名に対するテストは分けるといいんじゃないかということでした。テスト名と内容が合っていないので。（分かりにくくてすみません）
https://github.com/fjordllc/bootcamp/pull/6443/files#r1167353907

test 'is valid login_name' do # 有効なログイン名に関するテスト end test 'is invalid login_name' do # 無効なログイン名に関するテスト end

あるいは両方のテストを一緒にしてしまうなら、validとかinvalidという単語自体をタイトルから省いてしまうという手もあります。こっちのほうがシンプルかもしれないです。「ログイン名に関するテストですよ」ということで。

test 'login_name' do # 有効なログイン名と無効なログイン名に関するテスト end

ここのすぐ下のtwitter_accountに関するテストなどはそうなっていますね。

bootcamp/test/models/user_test.rb

Lines 201 to 215 in 0cff850

test 'twitter_account' do

user = users(:komagata)

user.twitter_account = ''

assert user.valid?

user.twitter_account = 'azAZ_09'

assert user.valid?

user.twitter_account = '-'

assert user.invalid?

user.twitter_account = 'あ'

assert user.invalid?

user.twitter_account = ':'

assert user.invalid?

user.twitter_account = 'A' * 16

assert user.invalid?

end

逆に、厳密にやるならテストケースひとつひとつに名前をつけてしまうという手もあります。

... test 'login_name should be more than or equal to three characters' do ... end ...

event_testなどはそうなっていますね。でもlogin_nameでこれをやるとケースが多すぎて面倒かも…

bootcamp/test/models/event_test.rb

Lines 110 to 120 in 0cff850

test 'should be invalid when start_at >= end_at' do

event = events(:event1)

event.end_at = event.start_at - 1.hour

assert event.invalid?

end

test 'should be invalid when open_start_at >= open_end_at' do

event = events(:event1)

event.open_end_at = event.open_start_at - 1.day

assert event.invalid?

end

ここまでしなくても、たとえばテストケースひとつひとつにコメントをつけてやるとちょっと親切になるかもと思います。今の状態だとそれぞれのアサーションが何を意味しているのかわかりにくいので。

test 'is invalid login_name' do ... # 3文字未満は不可 user.login_name = 'xx' assert user.invalid? ... end

…と勢いでばーっと書いてしまいましたが、そもそもこのイシューの内容とは直接関係ないですし、けっこう好みの問題でもあると思うので、どこまでやるかはお任せします！
ただ、現状のテスト名にはちょっと違和感があったのでコメントしました:bow:

レビューありがとうございます！

自分以外の方が実装した部分の整理整頓まで気が回ってなかったので非常に参考になります。
せっかくなのでテスト名まで対応しようと思います。

YukiWatanabe824 · 2023-04-20T13:24:18Z

@ogawa-tomo
すみません💦勢い余って駒形さんの方にレビューを依頼していました。
まだaprroveされてないのでogawa-tomoさんのレビューの段階ですね。

↑のテストについて対応いたします。

YukiWatanabe824 · 2023-04-21T03:49:45Z

@ogawa-tomo
お疲れさまです。
修正しましたのでお手すきの際にご確認お願いいたします。

テスト名については付近のテストと統一して属性名(login_name)のみにしました。
一つのテストの中でアサーションが多い？のでなんとも煩雑な感は否めないところですが、is validとis invalidに分けてほかも同様に分けていくとするとどんどん対象が広がりそうな気がしてしまいました😅

ogawa-tomo · 2023-04-21T12:53:27Z

どんどん対象が広がりそうな気がしてしまいました😅

よく分かります～こだわりだすとキリがないですし、今回はこれで十分だと思います！

ogawa-tomo

@YukiWatanabe824
私としては良いと思います:+1:

YukiWatanabe824 · 2023-04-21T13:45:03Z

@ogawa-tomo
ながながとお付き合いいただきありがとうございました🙇🏻
また次回もよろしくおねがいします。

@komagata
お疲れさまです。
こちら受講生のレビューが完了しましたのでご確認お願いいたします。

komagata

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

YukiWatanabe824 changed the title ~~Feature/limit userid 3characters or more~~ ユーザーIDを3文字以上に限定する（n > 1） Apr 14, 2023

YukiWatanabe824 marked this pull request as ready for review April 14, 2023 06:39

YukiWatanabe824 requested a review from ogawa-tomo April 14, 2023 06:44

ogawa-tomo reviewed Apr 15, 2023

View reviewed changes

machida self-assigned this Apr 19, 2023

machida assigned YukiWatanabe824 and unassigned machida Apr 19, 2023

YukiWatanabe824 and others added 7 commits April 19, 2023 23:25

feat: アカウント名にバリデーション追加と文字数修正用のdata-migrateファイルを追加

f808096

test: バリデーション追加に対応してモデルテストを追加

403c438

コンフリクト解消

805ba72

文字数制限を伝えるテキストをビューに追加

5d9a597

refactor: より適したテスト名称に修正、テストで使用するデータを仕様に合わせた内容に修正

b2a26d7

会員登録フォームの注意書きを修正

5121bd7

追加する文字列を「0」に修正

d9407e9

YukiWatanabe824 force-pushed the feature/limit_userid_3characters_or_more branch from b8af6a8 to d9407e9 Compare April 19, 2023 14:25

rubocopによる修正

0941cd4

YukiWatanabe824 requested a review from komagata April 19, 2023 21:16

ogawa-tomo reviewed Apr 20, 2023

View reviewed changes

YukiWatanabe824 removed the request for review from komagata April 20, 2023 13:15

test:テスト名を付近の他のテストと統一させたものに修正

a3706e4

ogawa-tomo approved these changes Apr 21, 2023

View reviewed changes

YukiWatanabe824 requested a review from komagata April 25, 2023 09:19

komagata approved these changes Apr 30, 2023

View reviewed changes

komagata merged commit 32b2d48 into main Apr 30, 2023

komagata deleted the feature/limit_userid_3characters_or_more branch April 30, 2023 06:57

github-actions bot mentioned this pull request Apr 30, 2023

Release 2023-05-04 08:08:45 #6441

Merged

19 tasks

djkazunoko mentioned this pull request Jul 7, 2023

複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ユーザーIDを3文字以上に限定する（n > 1） #6443

ユーザーIDを3文字以上に限定する（n > 1） #6443

YukiWatanabe824 commented Apr 14, 2023 •

edited

Loading

YukiWatanabe824 commented Apr 14, 2023

ogawa-tomo commented Apr 14, 2023

ogawa-tomo left a comment

ogawa-tomo Apr 15, 2023

ogawa-tomo Apr 15, 2023

YukiWatanabe824 Apr 16, 2023 •

edited

Loading

ogawa-tomo Apr 15, 2023

YukiWatanabe824 Apr 16, 2023

ogawa-tomo Apr 15, 2023

YukiWatanabe824 Apr 16, 2023

ogawa-tomo Apr 15, 2023

YukiWatanabe824 Apr 16, 2023

machida Apr 19, 2023

ogawa-tomo commented Apr 15, 2023

YukiWatanabe824 commented Apr 16, 2023

ogawa-tomo commented Apr 16, 2023

YukiWatanabe824 commented Apr 18, 2023

machida commented Apr 19, 2023

YukiWatanabe824 commented Apr 19, 2023

ogawa-tomo left a comment

ogawa-tomo Apr 20, 2023

YukiWatanabe824 Apr 20, 2023

YukiWatanabe824 commented Apr 20, 2023

YukiWatanabe824 commented Apr 21, 2023

ogawa-tomo commented Apr 21, 2023

ogawa-tomo left a comment

YukiWatanabe824 commented Apr 21, 2023

komagata left a comment

	- if current_user && current_user.login_name !~ /\A[a-z\d](?:[a-z\d]\|-(?=[a-z\d]))*\z/i
	input.a-toggle-checkbox#hide-flash(type='checkbox')
	.flash.is-alert
	.container
	.flash__container
	label.flash__close(for='hide-flash')
	p.flash__message
	\| アカウント名に半角英数字と-（ハイフン）以外の文字が使われています。#{link_to '変更', edit_current_user_path}をお願いします。

	test 'twitter_account' do
	user = users(:komagata)
	user.twitter_account = ''
	assert user.valid?
	user.twitter_account = 'azAZ_09'
	assert user.valid?
	user.twitter_account = '-'
	assert user.invalid?
	user.twitter_account = 'あ'
	assert user.invalid?
	user.twitter_account = ':'
	assert user.invalid?
	user.twitter_account = 'A' * 16
	assert user.invalid?
	end

	test 'should be invalid when start_at >= end_at' do
	event = events(:event1)
	event.end_at = event.start_at - 1.hour
	assert event.invalid?
	end

	test 'should be invalid when open_start_at >= open_end_at' do
	event = events(:event1)
	event.open_end_at = event.open_start_at - 1.day
	assert event.invalid?
	end

ユーザーIDを3文字以上に限定する（n > 1） #6443

ユーザーIDを3文字以上に限定する（n > 1） #6443

Conversation

YukiWatanabe824 commented Apr 14, 2023 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

YukiWatanabe824 commented Apr 14, 2023

ogawa-tomo commented Apr 14, 2023

ogawa-tomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YukiWatanabe824 Apr 16, 2023 • edited Loading

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

Choose a reason for hiding this comment

ogawa-tomo commented Apr 15, 2023

YukiWatanabe824 commented Apr 16, 2023

ogawa-tomo commented Apr 16, 2023

YukiWatanabe824 commented Apr 18, 2023

machida commented Apr 19, 2023

YukiWatanabe824 commented Apr 19, 2023

ogawa-tomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YukiWatanabe824 commented Apr 20, 2023

YukiWatanabe824 commented Apr 21, 2023

ogawa-tomo commented Apr 21, 2023

ogawa-tomo left a comment

Choose a reason for hiding this comment

YukiWatanabe824 commented Apr 21, 2023

komagata left a comment

Choose a reason for hiding this comment

YukiWatanabe824 commented Apr 14, 2023 •

edited

Loading

YukiWatanabe824 Apr 16, 2023 •

edited

Loading