Masahikokawai step12 #340

masahikokawai · 2019-06-03T08:23:59Z

・ステップ12: 終了期限を追加しよう

cd3184b...569af83
※ 終了日は追加してなく、作成日でそのままソートしてます。。

kenkaton · 2019-06-05T03:06:19Z

私もこちら（ #339 (comment) ）と同じエラーが出たので、今後新規でbundle installする方は皆さん引っかかっちゃうかもしれないですね。
５月中旬あたりに対応されたものの影響のようです。

masahikokawai · 2019-06-05T03:23:22Z

@kenkaton
#340 (comment)

了解です。
Gemfile.lock 修正したのpush しますね！

masahikokawai · 2019-06-05T03:29:47Z

@kenkaton
#340 (comment)
Gemfile.lock 修正したのpushしました。
8306573

kenkaton

いくつかコメントしましたが、以下も対応お願いします。

ステップ12の終了期限は…追加しましょう:sob:w
せっかくI18nを利用しているので、view内の日本語表記は基本的に書き換えたほうがいいと思います:pray:
タスクにステータスを追加したので、model specに追記してテストしましょう(正常系だけでなく、異常系もテストするといいかも):muscle:

あと、次回からでいいのですが、プルリクはある程度まとまりのある粒度で出したほうがやっぱりいいかもしれないですね。
今回でいうと、step12、step13、step14/15の３つくらいに分けたほうがよかったかも？
様々な変更点を一つのPRにまとめてしまうと、レビュアーの確認すべき箇所が分散してしまって抜け漏れが多くなっちゃいそうです:sob:
ただ、step16/17のような関係が深いstepは同時にPR出しちゃってもいいと思うのでなかなか難しいのですがw

kenkaton · 2019-06-05T04:48:02Z

app/controllers/tasks_controller.rb

@@ -4,7 +4,19 @@ class TasksController < ApplicationController
  before_action :set_task, only: %i[show edit update destroy]

  def index


index内の条件分岐が多くなってきているので行数が長くなり、Rubocopでも引っかかっているようです。

ここはrubocop対象外とさせてください。

ここなんですが、以下のようなコードにすれば大丈夫そうです:+1:

@tasks = Task.name_like(params[:name]).status(params[:status]).order(finished_on: params[:sort] || :desc).page(params[:page])

13行を1行にまとめる力技w
ちょっと横にながーーくなるのが気になりますが、改行を入れるなりして見やすく整えると良いかもしれないです:bowtie:

やっていることは以下の２点ですね！

if params[:commit].nil?で条件分岐をしない

scopeで nil か false を返した時は all と同じ扱いになるため

params[:sort]での条件分岐を||で行う

コードありがとうございます！！

ただ、個人的にはrubocop先生が全てではないですし、
メンテする我々が見やすいコードが優先されたほうが良いのでは？
と思ってます。(元のコードのが良いとも言っているわけでは決してないです)

ですので、横に長いのならこのままで行きたいです。。

なるほど、確かにメンテのしやすいコードがいいと思います！
ラクマのコードも全てrubocopに引っかからないわけではないそうですし、臨機応変に書くのがいいですね。
こちらはこのままでオッケーです！

あと、私のコードだと初期表示のソートが変わっちゃいますね…
完全に読み間違えてました:scream:

ワンライナーにしちゃうと、プルリクの差分的にもわかりにくいんですよね。。

kenkaton · 2019-06-05T04:55:36Z

app/controllers/tasks_controller.rb

@@ -16,7 +28,7 @@ def create

    respond_to do |format|


現状ではhtml形式のレスポンスしか返していないようなので、この辺りのrespond_to do |format|関連のコードは不要かもしれないですね。

HTMLのみなので削除しました。
9e90e53

kenkaton · 2019-06-05T05:11:13Z

app/models/task.rb

@@ -4,4 +4,7 @@ class Task < ApplicationRecord
  enum status: { waiting: 1, work_in_progress: 2, completed: 3 }

  validates :name, presence: true, length: { maximum: 20 }
+
+  scope :name_like, -> (name) { where('name like ?', "%#{name}%") if name.present? }
+  scope :status, -> (status) { where(status: status) if status.present? }


scopeを使用していて、とてもいいと思います:+1:

kenkaton · 2019-06-05T05:13:06Z

app/views/kaminari/_first_page.html.erb

@@ -0,0 +1,3 @@
+<li class="page-item">
+  <%= link_to_unless current_page.first?, raw(t 'views.pagination.first'), url, remote: remote, class: 'page-link' %>


おお！link_to_unless メソッドなんてあるんですね！
全然知りませんでした:flushed:

kenkaton · 2019-06-05T05:23:18Z

app/views/layouts/application.html.erb

+      <%= link_to "Rails Training", "#", id: "logo", class: "navbar-brand" %>
+      <ul class="navbar-nav ml-auto">
+        <li class="nav-item">
+          <%= link_to "Home", "#", class: "nav-link" %>


ヘッダー内のリンク先が指定されていないので（root_path等になるのかな？）指定していただけるといいなと思います！

pathを設定しました。
021db0d

kenkaton · 2019-06-05T05:36:29Z

spec/system/tasks_spec.rb

+    end
+  end
+
+  context '一覧検索' do


検索機能では少なくとも以下のパターンはテストした方がいいと思いました。

タスク名で検索できること（実装済）

ステータスで検索できること（実装済）

タスク名が未入力でステータスが未選択のとき、全件表示されること（未実装）

タスク名が入力済みでステータスも選択済みのとき、正しく絞り込まれること（未実装）

spec に次のケースを足しました。
900cd64

タスク名が未入力でステータスが未選択のとき、全件表示されること（未実装）

タスク名が入力済みでステータスも選択済みのとき、正しく絞り込まれること（未実装）

spec に次のケースを足しました。
900cd64

タスク名が未入力でステータスが未選択のとき、全件表示されること（未実装）

タスク名が入力済みでステータスも選択済みのとき、正しく絞り込まれること（未実装）

すみません、ちょっと説明不足でした。

タスク名が未入力でステータスが未選択のとき、全件表示されること（未実装）

こちらのケースは 両方とも未入力の状態で検索ボタンを押した ときです。
あまりないケースだとは思いますが、タスク一覧画面表示直後にそのまま検索ボタンを押した時にエラーが起こっていないかの確認です。

何も入力値なしの検索のケース追加しました。
21177e5

kenkaton · 2019-06-05T06:00:51Z

app/views/tasks/index.html.erb


 <br>

-<%= link_to t('buttons.new'), new_task_path %>
+<%= link_to t('buttons.new'), new_task_path, class: "btn btn-primary pull-right" %>


ちょっと個人的な意見になるんですが、タスクの新規作成ボタンはタスクの一覧表より上に表示したほうがUIとしてはいいように感じました！（対応するかどうかはお任せします）

画面上部、右に配置するようにしました。
013c0c0

kenkaton · 2019-06-05T06:04:39Z

app/views/tasks/index.html.erb

+総件数: <%= @tasks.total_count %>
+
+<div class="table-responsive">
+  <table border="table table-striped table-bordered table-hover">


bootstrapのtable関連クラスが反映されていないので、修正お願いします:pray:

今更ですが、反映されてないですね。。
確認します。

f752151
にて修正しました。

kenkaton · 2019-06-05T06:10:42Z

app/views/tasks/index.html.erb

+<br>
+
+<%= paginate @tasks %>
+ページ: <%= @tasks.current_page %>/<%= @tasks.total_pages %>


おおー、こういったちょっとした工夫はすごくいいですね:tada:
ちょっと英語になっちゃいますが、kaminariのtotal_pagesメソッド関連だとこことか読むとなかなか面白いかも？

レコードが0の時、

total_pages: 0 current_page: 1

となるissueですね。

current_pageメソッドは廃止予定にしてparams[:page]を使うように
total_pagesは、1を返すようになるかもしれないですね！
勉強になりました！

masahikokawai · 2019-06-05T07:00:18Z

ステップ12の終了期限は…追加しましょう😭w

ソートするのが目的ならと思ったのですが、、追加しますね！

せっかくI18nを利用しているので、view内の日本語表記は基本的に書き換えたほうがいいと思います🙏

こちらも修正します！

タスクにステータスを追加したので、model specに追記してテストしましょう(正常系だけでなく、異常系もテストするといいかも)💪

本PRに含まれてないので恐縮ですが、1つ前のPRでmodel spec に追加してます。
https://github.com/Fablic/training/pull/339/files#diff-ffb5b5242cf0ba53067d359c8b8ead99R45

あと、次回からでいいのですが、プルリクはある程度まとまりのある粒度で出したほうがやっぱりいいかもしれないですね。
今回でいうと、step12、step13、step14/15の３つくらいに分けたほうがよかったかも？
様々な変更点を一つのPRにまとめてしまうと、レビュアーの確認すべき箇所が分散してしまって抜け漏れが多くなっちゃいそうです😭
ただ、step16/17のような関係が深いstepは同時にPR出しちゃってもいいと思うのでなかなか難しいのですがw

おっしゃる通りですね！見る方も負担になりますしね。。
次回からある程度のまとまりでPR出すようにします！

kenkaton · 2019-06-05T08:33:27Z

タスクにステータスを追加したので、model specに追記してテストしましょう(正常系だけでなく、異常系もテストするといいかも)💪

本PRに含まれてないので恐縮ですが、1つ前のPRでmodel spec に追加してます。
https://github.com/Fablic/training/pull/339/files#diff-ffb5b5242cf0ba53067d359c8b8ead99R45

なるほど。
上記PRがmaster(masahikokawai)ブランチにmerge済みのようなので、こちらのブランチ(masahikokawai-step12)にも反映するのが良さそうですね。
PRの粒度もですが、commitの粒度も適切なものになっていると、git cherry-pickコマンドで特定のcommitのみ別ブランチにも反映させるということができるので便利かもしれません:+1:

$ rails g kaminari:config $ rails g kaminari:views bootstrap4 -e erb

#339 (comment) ffi/ffi#701

masahikokawai · 2019-06-06T00:06:35Z

上記PRがmaster(masahikokawai)ブランチにmerge済みのようなので、こちらのブランチ(masahikokawai-step12)にも反映するのが良さそうですね。
PRの粒度もですが、commitの粒度も適切なものになっていると、git cherry-pickコマンドで特定のcommitのみ別ブランチにも反映させるということができるので便利かもしれません👍

rebase して取り込みましたー

リクエストフォーマットはHTMLのみなので不要のため

kenkaton · 2019-06-06T06:46:52Z

もしかしたら私の環境だけかもしれないんですが、bundle installした時にMysql2関連でエラーが出てしまうようです。
その場合には下記コマンドを実行すればインストールできるようになるみたいなので、今後のPRに注記として書いておくと良いかもしれませんね。
bundle config --local build.mysql2 "--with-ldflags=-L/usr/local/opt/openssl/lib"

masahikokawai · 2019-06-06T06:58:19Z

@kenkaton
#340 (comment)
了解です、ありがとうございます！

エラーの内容も貼っておいてもらえると助かりますー

kenkaton · 2019-06-06T07:02:41Z

app/views/layouts/application.html.erb

@@ -10,6 +10,25 @@
  </head>

  <body>
-    <%= yield %>
+    <nav class="navbar navbar-expand-lg navbar-dark bg-dark" style="background-color: #71da71;">
+      <%= link_to "Rails Training", "#", id: "logo", class: "navbar-brand" %>


あ、ここもパスを通しちゃいましょうか

一旦、同じくタスクのroot_pathにしてしまいますね。

87cb929
にてセットしました。

kenkaton · 2019-06-06T07:15:01Z

spec/system/tasks_spec.rb

    end

-    scenario '一覧のソート順が登録日の降順であること' do
+    scenario '一覧の初期表示のソート順が登録日の降順であること' do


この辺りは登録日ではなく、終了期限でソートされてますね

ちなみにそう思った理由って聞いてもいいですか？

はっ！
私のコードの読み間違い＆勘違いでした…
修正すべき箇所は以下のみでした
#340 (comment)

#340 (comment)
にてメッセージ直しました。

kenkaton · 2019-06-06T07:21:02Z

@kenkaton
#340 (comment)
了解です、ありがとうございます！

エラーの内容も貼っておいてもらえると助かりますー

エラー遡っても消えちゃってたんですが、こんな感じですー

To see why this extension failed to compile, please check the mkmf.log which can be found here:

/Users/user_name/path/to/vendor/bundle/ruby/2.6.0/extensions/x86_64-darwin-18/2.6.0-static/mysql2-0.5.2/mkmf.log

extconf failed, exit code 1

Gem files will remain installed in
/Users/user_name/path/to/vendor/bundle/ruby/2.6.0/gems/mysql2-0.5.2 for inspection.
Results logged to
/Users/user_name/path/to/vendor/bundle/ruby/2.6.0/extensions/x86_64-darwin-18/2.6.0-static/mysql2-0.5.2/gem_make.out

An error occurred while installing mysql2 (0.5.2), and Bundler cannot continue.
Make sure that `gem install mysql2 -v '0.5.2' --source 'https://rubygems.org/'` succeeds before bundling.

In Gemfile:
  mysql2

kenkaton · 2019-06-06T08:46:27Z

spec/system/tasks_spec.rb

+      expect(tds[8]).to have_content 'task2'
+    end
+
+    scenario '一覧のソート順が登録日の昇順/降順と切り替わること' do


終了期限でソートされるのはこちらだけでした…

確かにー
作成日から終了期限に変更しているのにメッセージ修正漏れてましたorz
修正します！

~~登録日~~ から終了期限に変更しました。
ebc7910

kenkaton · 2019-06-06T08:54:32Z

LGTMです！

masahikokawai · 2019-06-06T08:55:25Z

@kenkaton
確認ありがとうございました！！
(量多くてすみませんでした。。)

masahikokawai self-assigned this Jun 3, 2019

masahikokawai force-pushed the masahikokawai-step12 branch 4 times, most recently from 3abbab7 to cd3184b Compare June 4, 2019 05:12

masahikokawai changed the title ~~[WIP] Masahikokawai step12~~ Masahikokawai step12 Jun 4, 2019

kenkaton reviewed Jun 5, 2019

View reviewed changes

masahiko kawai added 9 commits June 6, 2019 09:04

bootstrap4の導入と画面への適用

b353678

kaminariを導入してページネーションを表示する

8afe971

$ rails g kaminari:config $ rails g kaminari:views bootstrap4 -e erb

タスク一覧に検索機能を追加

b7f69b8

ソート処理を追加

31a96b8

ソート項目を1つの項目にのみ変更

536e13e

ソートのsystem spec を追加

b35f758

rubocop auto correct

657adcb

rubygemsからffi-1.11.0が削除されたのでffi-1.11.1に修正

5e2722f

#339 (comment) ffi/ffi#701

一覧検索のspecにケースを追加

900cd64

masahikokawai force-pushed the masahikokawai-step12 branch from 8306573 to 900cd64 Compare June 6, 2019 00:04

masahiko kawai added 7 commits June 6, 2019 10:24

respond_toメソッドを削除

9e90e53

リクエストフォーマットはHTMLのみなので不要のため

ヘッダーメニューのリンク先を設定

021db0d

タスク登録のボタン位置を画面上部に変更

013c0c0

タスクに終了日のカラムを追加

3fca6b6

タスク登録処理に終了期限の項目を追加

4fd5367

rubocop auto correct

6f25f68

sysytem spec の修正

31273d9

masahiko kawai added 2 commits June 6, 2019 14:28

タスク一覧のtableタグにcssが当たってないので修正

f752151

i18nを使用するに修正

900f2b3

kenkaton reviewed Jun 6, 2019

View reviewed changes

masahiko kawai added 3 commits June 6, 2019 16:49

ヘッダーメニューにリンク先をセット

87cb929

system spec にタスク一覧でのテストケースを追加

21177e5

i18nの修正に伴いsystem specの修正漏れを修正

bbd6f2f

kenkaton reviewed Jun 6, 2019

View reviewed changes

system specのメッセージ修正漏れの修正

ebc7910

masahikokawai merged commit 151aa36 into masahikokawai Jun 6, 2019

masahikokawai deleted the masahikokawai-step12 branch June 6, 2019 09:00

masahikokawai mentioned this pull request Jun 13, 2019

Masahikokawai step16 #348

Merged

		@@ -4,7 +4,19 @@ class TasksController < ApplicationController
		before_action :set_task, only: %i[show edit update destroy]

		def index

		@@ -0,0 +1,3 @@
		<li class="page-item">
		<%= link_to_unless current_page.first?, raw(t 'views.pagination.first'), url, remote: remote, class: 'page-link' %>

Masahikokawai step12 #340

Masahikokawai step12 #340

Conversation

masahikokawai commented Jun 3, 2019 • edited

・ステップ12: 終了期限を追加しよう

・ステップ13: ステータスを追加して、検索できるようにしよう

・ステップ14: ページネーションを追加しよう

・ステップ15: デザインを当てよう

kenkaton commented Jun 5, 2019

masahikokawai commented Jun 5, 2019

masahikokawai commented Jun 5, 2019

kenkaton left a comment • edited

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

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

masahikokawai Jun 6, 2019 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahikokawai commented Jun 5, 2019

kenkaton commented Jun 5, 2019

masahikokawai commented Jun 6, 2019

kenkaton commented Jun 6, 2019

masahikokawai commented Jun 6, 2019 • edited

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

kenkaton commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenkaton commented Jun 6, 2019

masahikokawai commented Jun 6, 2019

masahikokawai commented Jun 3, 2019 •

edited

kenkaton left a comment •

edited

masahikokawai Jun 6, 2019 •

edited

masahikokawai commented Jun 6, 2019 •

edited