Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kumo raku step12 add end date #1459

Merged
merged 8 commits into from
Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions myapp/app/controllers/tasks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@

class TasksController < ApplicationController
def index
@tasks = if params[:latest]
Task.latest
else
Task.all
end
@tasks = Task.search(params)
end

def show
Expand Down Expand Up @@ -60,6 +56,6 @@ def destroy
private

def task_params
params.require(:task).permit(:title, :description)
params.require(:task).permit(:title, :description, :end_date)
end
end
4 changes: 3 additions & 1 deletion myapp/app/models/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@ class Task < ApplicationRecord
validates :title, presence: true, length: { maximum: 40 }
validates :description, length: { maximum: 500 }

scope :latest, -> { order(created_at: :desc) }
def self.search(conditions)
order("#{conditions['sort_column']} #{conditions['sort_direction']}")
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こういう書き方にするとユーザが任意の操作でデータベースの並び順を帰れます。
あと間違ったパラメータを渡すとアプリケーションエラーが発生するので直しましょう、、

Search用で使用するメソッドではないと思います

Copy link
Author

@KumoRaku KumoRaku Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こういう書き方にするとユーザが任意の操作でデータベースの並び順を帰れます。

すみません、ここは理解できなかった。もう詳しく説明お願いいただければありがたいです。

あと間違ったパラメータを渡すとアプリケーションエラーが発生するので直しましょう、、

ユーザが間違ったパラメーターに編集するとエラーページに遷移するとは正しいと思ってますが、
一応リクエストパラメーターのバリデーションを追加します。パラメーターが正しいときだけ結果返すようにします。

Search用で使用するメソッドではないと思います

検索機能を実装したときこのメソッドにwhere処理を追加して、検索ボタンを押すとこのメソッドを呼んで結果をとるつもりですが、何かご懸念点ございますか?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shinさんがアドバイスした実装方法も考えてみましたが、今後たとえ並び方法が多くなったらコントローラー側は

def index
  @tasks = Task.expiring(params[:sort]).latest(params[:sort]).newest(params[:sort])... ...
end

みたいになちゃいます。latest.newestみたいなソースを読むと意味わかりづらいので、
同じ機能のscopeを全部繋ぐのは避けたいということを考えてます。mm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こういう書き方にするとユーザが任意の操作でデータベースの並び順を「変えれます」でした。。すみません誤字です。

この辺をもっと説明すると
taskで並び順を変えたいところ以外のカラムを入力しても動きます例えば現在はcreated_atを並び順変えを入れているのにdescriptionなど他のカラムをパラメーターに渡して入力しても並び順を変えれます。

こちらは開発者が意図してない並び順変更ができてしまい、
今後indexがない状態のカラムで膨大なデータがある場合、
並び順変更だけでtimeoutエラーとかDBへの負荷も高まる可能性があります。
あと意図してないカラムを他の人に漏出してしまうのも良くないと思うので
意図したカラム以外の入力はできなくしたいですね

あとアプリケーションエラーの補足説明ですが、
urlで入力するカラム名を変え入力すると500系エラーが表示されます。
一般にはアプリケーションの動作が止まることはないと思います404に移動するか、
並び順が変わったりなど動作は担保されるはずです。
image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shinさんがアドバイスした実装方法も考えてみましたが、今後たとえ並び方法が多くなったらコントローラー側は

def index
  @tasks = Task.expiring(params[:sort]).latest(params[:sort]).newest(params[:sort])... ...
end

みたいになちゃいます。latest.newestみたいなソースを読むと意味わかりづらいので、 同じ機能のscopeを全部繋ぐのは避けたいということを考えてます。mm

sortと並び順をパラメータに渡すことでlatest、newestなどはなくせるのでは?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scopeに全部書くを避けたいイメージはどれほどの列の並び変えを考えてます?
最初のちょうさんが書いた状態からasc descの変更ができればいいと思いましたが、、

end
5 changes: 5 additions & 0 deletions myapp/app/views/tasks/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
<% end %>
</div>

<div>
<%= form.label :end_date %>
<%= form.datetime_field :end_date %>
</div>

<div>
<%= form.submit %>
</div>
Expand Down
7 changes: 6 additions & 1 deletion myapp/app/views/tasks/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@
<th><%= Task.human_attribute_name :description %></th>
<th>
<span><%= Task.human_attribute_name :created_at %></span>
<small><%= link_to (I18n.t 'tasks.index.sort.latest'), tasks_path(latest: 'true') %></small>
<small><%= link_to (I18n.t 'tasks.index.sort.latest'), tasks_path(sort_column: 'created_at', sort_direction: 'desc') %></small>
</th>
<th>
<span><%= Task.human_attribute_name :end_date %></span>
<small><%= link_to (I18n.t 'tasks.index.sort.expiring'), tasks_path(sort_column: 'end_date', sort_direction: 'asc') %></small>
</th>
<th><%= I18n.t 'ops.edit' %></th>
</tr>
Expand All @@ -21,6 +25,7 @@
<td><%= link_to task.title, task %></td>
<td><%= task.description %></td>
<td><%= I18n.l(task.created_at.to_date, format: :long) %></td>
<td><%= task.end_date ? I18n.l(task.end_date, format: :long) : '-' %></td>
<td><%= link_to (I18n.t 'ops.edit'), edit_task_path(task) %></td>
</tr>
<% end %>
Expand Down
1 change: 1 addition & 0 deletions myapp/app/views/tasks/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

<h2><%= @task.title %></h2>
<p><%= "#{Task.human_attribute_name :description}: #{@task.description}" %></p>
<p><%= "#{Task.human_attribute_name :end_date}: #{@task.end_date ? I18n.l(@task.end_date, format: :long) : '-'}" %></p>

<%= link_to (I18n.t 'ops.edit'), edit_task_path(@task) %>
<%= link_to (I18n.t 'ops.delete'), task_path(@task),
Expand Down
2 changes: 1 addition & 1 deletion myapp/config/locales/common.ja.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ ja:
formats:
default: "%Y/%m/%d %H:%M:%S"
short: "%y/%m/%d %H:%M"
long: "%Y年%m月%d日(%a) %H時%M分%S秒 %Z"
long: "%Y年%m月%d日(%a) %H時%M分%S秒"
am: "午前"
pm: "午後"
ops:
Expand Down
1 change: 1 addition & 0 deletions myapp/config/locales/model.ja.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ ja:
# view側: User.human_attribute_name :name => "名前" / t("activerecord.attributes.user.name")と同じ
title: タイトル
description: 詳細
end_date: 終了期限

# 全てのmodelで共通して使用するattributesを定義
attributes:
Expand Down
1 change: 1 addition & 0 deletions myapp/config/locales/views/tasks/index/ja.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ ja:
title: 'タスク一覧'
sort:
latest: '新しい順'
expiring: '終了期限が近い順'
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddColumnEndDateToTasks < ActiveRecord::Migration[6.0]
def change
add_column :tasks, :end_date, :datetime
end
end
3 changes: 2 additions & 1 deletion myapp/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2022_12_07_134939) do
ActiveRecord::Schema.define(version: 2022_12_08_085448) do

create_table "tasks", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", force: :cascade do |t|
t.string "title", limit: 40, null: false
t.text "description"
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
t.datetime "end_date"
end

end
4 changes: 4 additions & 0 deletions myapp/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ services:
environment:
MYSQL_ROOT_PASSWORD: password
MYSQL_DATABASE: root
TZ: Asia/Tokyo
platform: linux/amd64
ports:
- "3316:3306"
Expand Down Expand Up @@ -38,11 +39,14 @@ services:
DB_NAME: myapp
DB_PASSWORD: password
DB_HOST: db
TZ: Asia/Tokyo

chrome:
image: seleniarm/standalone-chromium
ports:
- 4444:4444
environment:
TZ: Asia/Tokyo

volumes:
db-data:
Expand Down
1 change: 1 addition & 0 deletions myapp/spec/factories/tasks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
factory :task do
title { 'Spec' }
description { 'test' }
end_date { Time.new(2022, 12, 7, 10, 30) }
end
end
17 changes: 16 additions & 1 deletion myapp/spec/system/tasks_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

context 'when there are tasks,' do
let!(:task1) { FactoryBot.create(:task) }
let!(:task2) { FactoryBot.create(:task, title: 'Spec2', description: 'test2') }
let!(:task2) { FactoryBot.create(:task, title: 'Spec2', description: 'test2', end_date: Time.new(2023, 11, 1, 10, 30)) }
Copy link

@sourcewc sourcewc Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let!にする必要はないと思いました。
let!は遅延評価されてパフォーマンス的に良くないと思います。
あとlet!を使う必要があるようなデータを作成するようにも見えませんでした。
before内でまとめましょうか

before do
  FactoryBot.create(:task)
  FactoryBot.create(:task, title: 'Spec2', description: 'test2', end_date: Time.new(2023, 11, 1, 10, 30))
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

参考議論なども併せて確認してください
rubocop/rspec-style-guide#55 (comment)

rubocop/rubocop-rspec#94 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


before do
# 一覧画面を開く
Expand All @@ -30,8 +30,10 @@
expect(page).to have_content 'タスク一覧'
expect(page).to have_content 'Spec'
expect(page).to have_content 'test'
expect(page).to have_content '2022年12月07日(水) 10時30分00秒'
expect(page).to have_content 'Spec2'
expect(page).to have_content 'test2'
expect(page).to have_content '2023年11月01日(水) 10時30分00秒'
expect(Task.count).to eq 2
end

Expand All @@ -43,6 +45,15 @@
expect(page.text).to match(/Spec2.*Spec/)
expect(Task.count).to eq 2
end

it 'sorts correctly after push sort button: expiring' do
click_link '終了期限が近い順'

expect(page).to have_content 'タスク一覧'
# 正規表現で並び順をチェック
expect(page.text).to match(/Spec.*Spec2/)
expect(Task.count).to eq 2
end
end

context 'when error,' do
Expand Down Expand Up @@ -72,6 +83,7 @@
# titleとdescriptionを入力
fill_in 'task_title', with: 'Spec test new task'
fill_in 'task_description', with: 'Spec test new task description'
fill_in 'task_end_date', with: Time.new(2023, 11, 1, 10, 30)

# 登録
click_button 'タスクを登録する'
Expand All @@ -81,6 +93,7 @@
expect(page).to have_content 'タスク詳細'
expect(page).to have_content 'Spec test new task'
expect(page).to have_content 'Spec test new task description'
expect(page).to have_content '2023年11月01日(水) 10時30分00秒'
end
end

Expand Down Expand Up @@ -224,6 +237,7 @@
# titleとdescriptionを入力する
fill_in 'task_title', with: 'Spec first task'
fill_in 'task_description', with: 'Spec first task description'
fill_in 'task_end_date', with: Time.new(2023, 11, 1, 10, 30)

# 更新実行
click_button 'タスクを更新する'
Expand All @@ -233,6 +247,7 @@
expect(page).to have_content 'タスク詳細'
expect(page).to have_content 'Spec first task'
expect(page).to have_content 'Spec first task description'
expect(page).to have_content '2023年11月01日(水) 10時30分00秒'
end
end

Expand Down