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

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


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

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

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


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

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

+<% end %>
+<%= form_for(@ad ,url: "/ad/#{@ad.id}",html: {method: "patch",class: "form-group"}) do |f| %> <%#＃入稿フォーム %>
+  <div class="form-group">
+    <p>text</p>


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

[MUST]ここ直ってないよ

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

@@ -0,0 +1,13 @@
+# Read about fixtures at http://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html


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

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

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


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

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

+      flash[:notice] = 'Ad registered!'
+      redirect_to(ad_index_path)
+    else
+      render("ad/new")


[IMO]パスの書き方

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

+      flash[:notice] = 'Ad updated!'
+      redirect_to(ad_index_path)
+    else
+      render("ad/edit")


[IMO]パスの書き方

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

+
+describe 'Index Page Path' do
+  it 'visit index' do
+    visit '/ad'


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

tmk-hsn

一部コメントした！

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

+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

+  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

+    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

+      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

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+class AddReportColumnToAds < ActiveRecord::Migration[5.2]
+  def change


全体的に

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

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

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

+#
+# It's strongly recommended that you check this file into your version control system.
+
+ActiveRecord::Schema.define(version: 2019_06_04_073026) do


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

[MUST]直ってない

tmk-hsn

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

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

+<% end %>
+<%= form_for(@ad ,url: "/ad/#{@ad.id}",html: {method: "patch",class: "form-group"}) do |f| %> <%#＃入稿フォーム %>
+  <div class="form-group">
+    <p>text</p>


[MUST]ここ直ってないよ

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

+  <div class="each_ad"> <%# 広告を一つ一つ表示 %>
+    <% @ads.each do |ad| %>
+      <li>
+        <span>ad_id :<%= ad.id %></span>


[MUST]ここも日本語化

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

+<%= render  partial: "errorlog" %>
+<%= form_for(@ad ,url: ad_index_path,html: {method: "post",class: "form-group"}) do |f| %> <%#＃入稿フォーム %>
+  <div class="form-group">
+    <p>text</p>


[MUST]ここも

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

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+class AddReportColumnToAds < ActiveRecord::Migration[5.2]
+  def change


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

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

+#
+# It's strongly recommended that you check this file into your version control system.
+
+ActiveRecord::Schema.define(version: 2019_06_04_073026) do


[MUST]直ってない

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

+          'advertiser_id' => nil,
+          'image' => nil
+        },
+                                 id: @ad.id }


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

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

+          'advertiser_id' => nil,
+          'image' => nil
+        },
+                                 id: @ad.id }


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

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

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


[MUST]直ってない

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

+    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

@@ -18,7 +18,7 @@ def create
      flash[:notice] = 'Ad registered!'


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

tmk-hsn

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

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

      redirect_to(ad_index_path)
    else
-      render('ad/new')
+      render(new_ad_path)


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

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

-  <div class="form-group">
-    <p>text</p>
+  <div class="form-group h3">
+    <p>広告文</p>


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

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

+  </thead>
+  <tbody>
+    <tr>
+     <% @ads.each do |ad| %><%# 広告を一つ一つ表示 %>


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

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

-  <div class="form-group">
-    <p>text</p>
+  <div class="form-group h3">
+    <p>広告文</p>


「テキスト」でよろしく

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

		@@ -0,0 +1,13 @@
		# Read about fixtures at http://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html

		@@ -18,7 +18,7 @@ def create
		flash[:notice] = 'Ad registered!'

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