Feature ad making by HashimotoLogly · Pull Request #5 · HashimotoLogly/newgrad_development

HashimotoLogly · 2019-06-10T04:53:58Z

OJTの広告入稿画面の作成を行いました。

CherryPickやResetを使いながらCommit内容を修正していたため、一部Commit内容が乱雑になっているかもしれません。申し訳ございません。
一通りファイルを確認しましたが、不可解な点が御座いましたらご連絡ください。

対応issue #4

tmk-hsn · 2019-06-10T04:57:32Z

app/jobs/application_job.rb

[imo]このファイルはいらないから消して大丈夫

tmk-hsn · 2019-06-10T04:58:23Z

app/helpers/ad_helper.rb

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


[imo]このファイルも消して動作するなら、消してもOK

app/views/ad/edit.html.erb

tmk-hsn · 2019-06-10T05:00:49Z

app/views/ad/edit.html.erb

[nits]pタグで囲まれた項目名は日本語で（他箇所も）

[MUST]ここ直ってないよ

app/views/ad/index.html.erb

tmk-hsn · 2019-06-10T05:09:44Z

test/fixtures/ads.yml

[nits]RSpec使ってるから、このファイルは消してOK

tmk-hsn · 2019-06-10T05:09:48Z

test/models/ad_test.rb

[nits]RSpec使ってるから、このファイルは消してOK

tmk-hsn · 2019-06-10T05:10:19Z

app/controllers/ad_controller.rb

[IMO]パスの書き方

tmk-hsn · 2019-06-10T05:10:27Z

app/controllers/ad_controller.rb

[IMO]パスの書き方

tmk-hsn · 2019-06-10T05:12:06Z

spec/features/path_spec.rb

[MUST]パスの書き方をxxx_yyy_pathにして欲しい

tmk-hsn

一部コメントした！

tmk-hsn · 2019-06-13T03:08:24Z

db/migrate/20190528085221_create_ads.rb

+class CreateAds < ActiveRecord::Migration[5.2]
+  def change
+    create_table :ads do |t|
+      t.integer :advertiser_id, null: false, default: 0 # 広告主ID


[MUST]
ID系のカラムはdefault 0なしでOKです。
ID = 0 の広告主は間違いなく存在しないので。
デフォルト値自体も、広告単体で（広告主に紐づかない状態で）登録されることはないから指定しないでOK。
ただし、null: falseはいるので注意。

tmk-hsn · 2019-06-13T03:18:38Z

db/migrate/20190528085221_create_ads.rb

+  def change
+    create_table :ads do |t|
+      t.integer :advertiser_id, null: false, default: 0 # 広告主ID
+      t.string :image, null: false, default: '' # 広告の画像URL


[IMO]
画像もバリデーションかけてるならデフォルト値は指定しなくても良いよ〜(not: nullは必要)

参考までに
http://tech.hapicky.com/archives/234

tmk-hsn · 2019-06-13T03:22:52Z

db/migrate/20190528085221_create_ads.rb

+    create_table :ads do |t|
+      t.integer :advertiser_id, null: false, default: 0 # 広告主ID
+      t.string :image, null: false, default: '' # 広告の画像URL
+      t.integer :price, null: false, default: 0 # 広告の価格


[ask]ここのpriceは具体的にどういう価格なのか、教えてもらってもいい？
[ask]それを踏まえて、広告の価格をデフォルト値0で設定して大丈夫かな？
[IMO]上記の質問に答えてもらった上で、default 0 は削除して大丈夫です

tmk-hsn · 2019-06-13T03:23:34Z

db/migrate/20190528085221_create_ads.rb

+      t.integer :advertiser_id, null: false, default: 0 # 広告主ID
+      t.string :image, null: false, default: '' # 広告の画像URL
+      t.integer :price, null: false, default: 0 # 広告の価格
+      t.string :text, null: false, default: '' # 広告の説明文


[IMO]ここも、入力必須にしているならnull: false は必要だけどデフォルト値はなくても大丈夫だよ。

tmk-hsn · 2019-06-13T03:25:58Z

db/migrate/20190604073026_add_report_column_to_ads.rb

全体的に

[ask]それぞれの項目は本当にintegerで大丈夫？
[ask]NULL制約、デフォルト値の検討はどうなっていますか？
[ask]これらのカラムはクリックされた結果だったりimpされた結果だったりすると思うけど、これらのカラムをadsに入れた理由を教えてもらえる？

[MUST]これらって結局adsテーブルに残すんだっけ？

tmk-hsn · 2019-06-13T04:07:29Z

db/schema.rb

このadsテーブル含めた全体的な設計について、明日質問させて欲しい

[MUST]直ってない

tmk-hsn

@bbjjki4 直ってない部分が多すぎる。こういったことは2度とないようにしてほしい

tmk-hsn · 2019-06-21T01:34:50Z

app/views/ad/edit.html.erb

[MUST]ここ直ってないよ

tmk-hsn · 2019-06-21T01:35:13Z

app/views/ad/index.html.erb

[MUST]ここも日本語化

tmk-hsn · 2019-06-21T01:35:31Z

app/views/ad/new.html.erb

[MUST]ここも

tmk-hsn · 2019-06-21T01:36:50Z

db/migrate/20190604073026_add_report_column_to_ads.rb

[MUST]これらって結局adsテーブルに残すんだっけ？

tmk-hsn · 2019-06-21T01:37:00Z

db/schema.rb

[MUST]直ってない

tmk-hsn · 2019-06-21T01:37:26Z

spec/controllers/ad_controller_spec.rb

[MUST]インデントおかしい

tmk-hsn · 2019-06-21T01:37:44Z

spec/controllers/ad_controller_spec.rb

[MUST]インデントおかしい

tmk-hsn · 2019-06-21T01:38:31Z

spec/features/path_spec.rb

+    visit '/ad/new'
+  end
+
+  it 'is Forms valid?' do


[MUST]直ってない

tmk-hsn · 2019-06-21T01:38:38Z

spec/features/path_spec.rb

+    page.attach_file('ad_image', '/Users/hashimototakuma/Downloads/PNG_transparency_demonstration_1.png')
+  end
+
+  it 'is create action valid?' do


[MUST]直ってない

tmk-hsn · 2019-06-21T03:17:08Z

app/controllers/ad_controller.rb

ごめん、レビュー漏れてたけど、メッセージは日本語で登録してほしい
（他箇所も）

tmk-hsn

細かいところだけど修正しておいて

tmk-hsn · 2019-06-27T05:44:50Z

app/controllers/ad_controller.rb

render('new') で行けたらそれで

tmk-hsn · 2019-06-27T05:45:26Z

app/views/ad/edit.html.erb

ここは「テキスト」いいよ

tmk-hsn · 2019-06-27T05:45:52Z

app/views/ad/index.html.erb

コメント不要（処理見れば何やってるかわかる）

tmk-hsn · 2019-06-27T05:46:19Z

app/views/ad/new.html.erb

「テキスト」でよろしく

tmk-hsn

@bbjjki4 LGTM! お疲れ。

このブランチはmasterにマージしてください。
その後、別ブランチでコンフリクトが起こっていたら解消して、引き続きデザインを綺麗にしてください。

tmk-hsn suggested changes Jun 10, 2019

View reviewed changes

HashimotoLogly force-pushed the feature_ad-making branch 2 times, most recently from d0bfea6 to ab607ff Compare June 11, 2019 07:22

tmk-hsn suggested changes Jun 13, 2019

View reviewed changes

tmk-hsn reviewed Jun 13, 2019

View reviewed changes

HashimotoLogly added 2 commits June 20, 2019 12:25

fix path name for rendering

da88770

delete unused files

c6e61bb

tmk-hsn suggested changes Jun 21, 2019

View reviewed changes

fix texts and db

447eca2

HashimotoLogly force-pushed the feature_ad-making branch from f1dcfc2 to 447eca2 Compare June 21, 2019 02:54

add ad_cotroller_spec.rb

dca848f

tmk-hsn reviewed Jun 21, 2019

View reviewed changes

HashimotoLogly added 4 commits June 21, 2019 12:23

fix notification to Japanese

3359d06

fix button name

a14df47

decorate edit,new,errorlog,index html

183e852

fix edit path & Rspec

9a83d44

tmk-hsn suggested changes Jun 27, 2019

View reviewed changes

fix from 広告文 to テキスト, render path, delete comment

c8826cd

tmk-hsn approved these changes Jun 27, 2019

View reviewed changes

Conversation

HashimotoLogly commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tmk-hsn left a comment

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!

tmk-hsn left a comment

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!

tmk-hsn left a comment

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!

tmk-hsn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers