#1 ユーザー登録の実装 by toyokappa · Pull Request #6 · toyokappa/test_app

toyokappa · 2017-07-02T22:17:06Z

fix #1

yhirano55 · 2017-07-02T23:17:23Z

Gemfile



 gem 'rails', '~> 5.1.2'
+gem 'bcrypt', '3.1.11'


このあたりを読んでバージョン指定の仕方を知ると良いかも。
http://qiita.com/awakia/items/5745938c192ca1139c63#gemfile%E3%81%A7%E3%81%AEversion%E6%8C%87%E5%AE%9A%E3%81%AE%E4%BB%95%E6%96%B9

yhirano55 · 2017-07-03T21:50:27Z

Gemfile

+gem 'uglifier',       '3.2.0'
+gem 'coffee-rails',   '4.2.2'
+gem 'turbolinks',     '5.0.1'
+gem 'jbuilder',       '2.7.0'


依存解決のためにbundlerを使っているので、ここまでバージョンを決め打ちする必要はありません。
OSSのGemfileなどを参考にしてみてください。

~> 5.1.2 と書くと、 5.1.x 系の例えば、マイナーバージョンアップされた、 5.1.3 などがリリースされたとき、それがインストールされます。bundlerの取り扱いは、基本的な知識なので理解しないと厳しそうです。

yhirano55 · 2017-07-03T22:05:10Z

commitの問題を指摘します。趣味ならば何でも良いと思うのですが、一応、業務想定としてマジレスしておきます。おそらく業務であれば、研修でない限り、通過しないPRになっています。

Commitは、適切な粒度でまとめる必要があります。最初はわかりにくいと思うので、以下のルールで対応するとよいかもしれません。

実装とテストを対にする（ app/models/user.rb と test/models/user_test.rb を対にする）
Model, Controller, Viewを混ぜずに分ける（configも分ける）
そのPRに関係のないCommitは極力積まない

Commitメッセージは通常は、日本語ではなく、英語を利用します。

なお、Gitの使い方に慣れていないと思うので、まずそれを慣れた方がよさそうです。

Commitする
Pushする

これらはできると思うのですが、それに加えて下記を覚えないと厳しそうです。

Commitしたけれど、それをresetして作り直す（ git reset ）
直前のCommitのメッセージを修正する（ git commit --am
Commitの順序を入れ替える (git rebase)
Commit AとBを統合する（git rebase）

yhirano55 · 2017-07-03T22:07:32Z

db/schema.rb

+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["email"], name: "index_users_on_email"
+  end


modelでuniquenessを保証するのであれば、通常は、table定義時でもそれを行う必要があります。

yhirano55 · 2017-07-03T22:16:58Z

ローカルのサーバーを起動して動作すること（bin/rails server）
テストが通ること（bin/rails test）

この2つは、reviewしてもらうときの前提なので、必ず確認するようにしましょう。

yhirano55 · 2017-07-08T03:46:30Z

Gemfile

-  gem 'sqlite3', '1.3.11'
-  gem 'byebug', '9.0.0', platform: :mri
+  gem 'sqlite3'
+  gem 'byebug', platforms: [:mri, :mingw, :x64_mingw]


修正不要ですが、このオプション指定は下記との勘違い?（あ、でもjrubyがないか）

# Windows does not include zoneinfo files, so bundle the tzinfo-data gem gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]

あーすみませんデフォルトですね。失礼。

group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console gem 'byebug', platforms: [:mri, :mingw, :x64_mingw] end

yhirano55 · 2017-07-08T03:53:33Z

db/migrate/20170705223629_create_users.rb

+      t.string :email
+      t.string :password_digest
+
+      t.timestamps


テーブルの作成・更新・削除など、DBに関する変更は、すべてmigrationに書きます。

schema.rb は、 rails db:migrate すると自動生成されるので、手作業で修正してはなりません。

Modelに presence: true と必須であることを保証する場合は、テーブル定義でも t.string :user_name, null: false と「Nullを許容しませんよ」という定義が必要です。

インデックスを張るときも、migrationで行います。ユニークであることもここで保証できます。

yhirano55 · 2017-07-08T04:01:13Z

app/models/user.rb

@@ -0,0 +1,15 @@
+class User < ApplicationRecord
+  before_save { email.downcase! }


validationを通過した後なので、save前にdowncaseする必要はなさそう。

もしもdowncaseを強制したいなら、before_validation あたりでもよさそう。

値を破壊的変更をする email.downcase! よりも self.email = email.downcase と書く方が一般的です。

yhirano55 · 2017-07-08T04:03:12Z

test/models/user_test.rb

+  end
+
+  test "ユーザー名：存在性" do
+    @user.user_name = "    "


モデルの名前が User なので、その名前を表す属性は name でよいのでは?（とここで思いました）

yhirano55 · 2017-07-08T04:05:44Z

config/routes.rb

  get '/contact', to: 'static_pages#contact'
+  get '/signup' , to: 'users#new'
+  post '/signup', to: 'users#create'
+  resources :users


修正しなくてもよいのですが、routingは、controllerが実装されてはじめて使えるようになるので、コミット順序からしたら、コントローラの実装の後でよいかと思います。

yhirano55 · 2017-07-08T04:07:27Z

test/integration/site_layout_test.rb

    assert_select "a[href=?]", root_path
    assert_select "a[href=?]", about_path
    assert_select "a[href=?]", contact_path
+    assert_select "a[href=?]", signup_path, count: 2


capybaraを使った方が楽そうです。

yhirano55 · 2017-07-08T04:12:01Z

app/views/shared/_error_messages.html.erb

+      <% end %>
+    </ul>
+  </div>
+<% end %>


修正するまでもないですが、パーシャルから、アサインされた変数を参照する場合、直接インスタンス変数を呼ばずに、 render "shared/error_messages", user: @user といった感じで、必要な変数をbindして、渡す方が望ましいです。なお、 shared なパーシャルであれば、 render "shared/error_message", resource: @user といった感じで渡して、汎用的にした方がよいかなと思いました。

yhirano55 · 2017-07-08T04:13:28Z

app/views/users/new.html.erb

+    <%= form_for(@user, url: signup_path) do |f| %>
+      <%= render "shared/error_messages" %>
+
+      <%= f.label :user_name, "ユーザー名" %>


i18nを設定すれば、 <%= f.label :user_name %> だけで十分なので、覚えてみてもよいかもしれませんね。

yhirano55 · 2017-07-08T04:18:38Z

app/views/users/show.html.erb

+<div class="row">
+  <aside class="col-md-4">
+    <section class="user_info">
+      <%= gravatar_for @user, size: 180 %>


provideもgravatar_forも、メソッドの名前から何を返すのか想像できませんでした。修正不要ですが、実際のチーム作業のときは恐らくNGになるかと思います。

理想を言うと、コードを読まなくてもメソッド名を見て、どういう結果が返されるのかが分かること、となります。

yhirano55 · 2017-07-08T04:20:27Z

app/controllers/users_controller.rb

+
+  private
+
+    #ストロングパラメーター


自分用かもしれませんが、上のコメントもそうですが、コメントも保守の対象なので、何を残すかは精査したいところですね（修正不要です）

yhirano55 · 2017-07-08T04:25:22Z

test/integration/user_signup_test_test.rb

+    assert_template "users/show"
+    assert_not flash.empty?
+    assert_match "ユーザー登録が完了しました！", response.body
+  end


内容的には、コントローラのテストっぽいので、コントローラでテストしてもよさそう。

yhirano55 · 2017-07-08T04:34:35Z

schema.rb を手で編集している部分を、migrationで表現して、それ以外は特に問題ないです。

以降の方針として、今回の目的をアプリケーションを作ることに焦点をあてて、テストコードは一切書かずに モンキーテストのみ でもよいかなと思いました（もちろん、どういうやり方をするかはおまかせします）テストは今回の要件に含めずに、 RSpecやCapybaraと共に学習してみる方が効率良いかなと思いました。

yhirano55 reviewed Jul 2, 2017

View reviewed changes

yhirano55 reviewed Jul 3, 2017

View reviewed changes

toyokappa force-pushed the signup-users branch 2 times, most recently from f2ca6f3 to 1f8bff6 Compare July 5, 2017 21:46

toyokappa added 8 commits July 6, 2017 07:30

Update gems and add bcrypt gem

aca287f

Create users table

f226c93

Create user model

d77ec73

Add signup_path and users resourse to routes

423f7d7

Add signup_path to views

8907c17

Update views for signup

80950b8

Create show view

09c4b91

Create users controller

1aacdf0

toyokappa force-pushed the signup-users branch from 1f8bff6 to 1aacdf0 Compare July 8, 2017 01:03

yhirano55 reviewed Jul 8, 2017

View reviewed changes

		@@ -0,0 +1,15 @@
		class User < ApplicationRecord
		before_save { email.downcase! }

Conversation

toyokappa commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhirano55 commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhirano55 commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhirano55 commented Jul 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants