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

プランナー新規登録機能と画面の追加 #10

Merged
merged 5 commits into from
Feb 4, 2021

Conversation

Zuck3-r
Copy link
Owner

@Zuck3-r Zuck3-r commented Feb 2, 2021

プランナーの新規登録画面と、機能の実装
image

@@ -0,0 +1,3 @@
// Place all the styles related to the Planners controller here.

Choose a reason for hiding this comment

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

使ってないのであれば消しましょう!
必要なとに作成すればいいので

Copy link
Owner Author

Choose a reason for hiding this comment

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

これは、ファイルごと消しちゃって良い感じですかね?

Choose a reason for hiding this comment

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

Yes!

@@ -0,0 +1,12 @@
require 'rails_helper'

RSpec.describe "Planners", type: :request do

Choose a reason for hiding this comment

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

"Planners"でなく以下のようにしておきましょうか😃

RSpec.describe PlannersController, type: :request do


RSpec.describe "Planners", type: :request do

describe "GET /new" do

Choose a reason for hiding this comment

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

describe 'GET /planner/new' do にして、

it { is_expected.to eq 200 }

とかだけの方がシンプルかなと思いました😃

expect(response).to have_http_status(:success)
end
end

Choose a reason for hiding this comment

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

ここは空行いらないです。

後、POST のテストがないので、追加してください!
ちゃんと想定通りのデータが作られているかのテストが一番重要なので✏️

config/routes.rb Outdated
@@ -1,3 +1,4 @@
Rails.application.routes.draw do
# For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html
root 'planners#new'
resources :planners

Choose a reason for hiding this comment

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

基本的に、必要なアクションだけに絞り込むために、onlyオプションをつけておきましょう😃

全アクション使う予定です???
であれば以下

resources :planners, only: %i(index new create show edit update destroy)

Copy link

@MurakiSari MurakiSari Feb 2, 2021

Choose a reason for hiding this comment

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

resources じゃなくて resource の方が良さそう。

というのもパスにidが不要になるので、他のプランナーの編集画面などに悪意のある人が入り込んだり、とかゆうのが防げた気がします。笑

色々と調べてみてください✏️
https://qiita.com/ryuuuuuuuuuu/items/e5960c7fecad4ef1301b

@MurakiSari
Copy link

後、画面など作成した場合は、画面のキャプチャを一番上の説明のところに貼っておいてください😊

require("@rails/ujs").start()
require("turbolinks").start()
require("@rails/activestorage").start()
require("channels")

Choose a reason for hiding this comment

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

空行が

</head>

<body>
<%= yield %>
</body>
</html>
</html>

Choose a reason for hiding this comment

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

空行が.....

| ">
= value
body
= yield

Choose a reason for hiding this comment

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

空行が...........

Comment on lines +12 to +16
- flash.each do |key, value|
| <p class="alert alert-
= key
| ">
= value

Choose a reason for hiding this comment

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

このフラッシュメッセージはヘッダーに表示させちゃって良いのかな??

他、ログインボタンとかヘッダーに表示させないなら良さそうだけど、
Matching Financial Plannerとかとかぶらないのかな🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

これ、先に大雑把に作ってたやつで確認したんですが、重ならなかったので置く事にしました。
image

Copy link

@MurakiSari MurakiSari left a comment

Choose a reason for hiding this comment

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

@Zuck3-r
指摘しました😃

@Zuck3-r
Copy link
Owner Author

Zuck3-r commented Feb 2, 2021

テストのpostの部分だけどう実装するのか分からなかったので、中途半端になっています。
画面の方も作りましたので、フラッシュメッセージと一緒にスクショを載せておきます。
image

9b8d7b2

post planners_new_path, params: planner
expect(response).to redirect_to root_url
describe 'POST /planners'do
context 'param is correct' do

Choose a reason for hiding this comment

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

パラメーターが不正なときのspecも書いてもらって良いですか??

@MurakiSari
Copy link

MurakiSari commented Feb 3, 2021

  • 作成した画面のキャプチャは一番上の説明欄に貼っていただきたいです🙏
  • コミットを一つ一つ綺麗に積んでいきたいので、できれば「空白行追加」だけのコミットとかはなくしたいです
    • 後から空白行の修正に気づいたとは、できればその空白行のファイルを編集したコミットにまとめて一緒にコミットする
       (今回で言えばgemfileを編集した時に空白行入れ忘れてたので、b72b898 に含めて欲しいという感じです)

やり方や、意味がわからなかったら聞いてください♪

@MurakiSari
Copy link

@Zuck3-r
コメントしました😃

@MurakiSari
Copy link

@Zuck3-r
このPRって、もう修正完了してるんだっけ??

Copy link

@MurakiSari MurakiSari left a comment

Choose a reason for hiding this comment

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

@Zuck3-r

以下のコミットの積み方に関しては、まだ修正されてないみたいだけど、
内容的には良さそうです!
#10 (comment)

@Zuck3-r Zuck3-r force-pushed the create-planner-controller branch 2 times, most recently from aa79e44 to fd5a2c2 Compare February 3, 2021 16:59
@Zuck3-r
Copy link
Owner Author

Zuck3-r commented Feb 3, 2021

@MurakiSari
commit統一の修正、忘れてました。申し訳ない🙇‍♂️
やりました

@Zuck3-r Zuck3-r merged commit 167277b into master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants