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

Develop #1

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Develop #1

wants to merge 30 commits into from

Conversation

hirohiroiida
Copy link
Owner

なかなか時間がかかってしまいましたが、たくさんの事勉強になりました。
すいません。コミットがざっくりしてます。
本当は一箇所変更するたびにコミットすると考えると、なかなか面倒だなーと思わされました。
すごく見にくいと思います。よろしくお願いします。

Comment on lines 24 to 31
def edit
@user = User.find(params[:id])
end

def update
@user = User.find(params[:id])

if @user.update(user_params)

Choose a reason for hiding this comment

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

User.findだとURLのidを書き換えたら他の人の情報を編集できてしまいます。この書き方は初心者にほんとによくある書き方なので気をつけましょう。

@user = current_user # または User.find(current_user.id)

にしないとですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

気をつけます。変更します!

private

def user_params
params.require(:user).permit(:name, :email, :admin, :password, :password_confirmation, :image)

Choose a reason for hiding this comment

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

:adminを許可するのはまずいです。なぜでしょう?

Copy link
Owner Author

Choose a reason for hiding this comment

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

変更を可能にしてしまうからですね!変更します!

Comment on lines 52 to 54
@question = current_user.questions.find(params[:id])
@question.update!(question_params)
redirect_to "/questions/#{@question.id}", notice: "タスク『#{@question.title}』を更新しました"

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.

変更します!

@question = current_user.questions.new(question_params)

if @question.save
redirect_to "/questions/#{@question.id}", notice: "タスク『#{@question.title}』を登録しました"

Choose a reason for hiding this comment

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

redirect_to @questionまたはredirect_to question_path(@question)の方がRailsっぽくてよいです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

変更します!統一感もありますもんね!

Comment on lines 1 to 7
<% if @answer.errors.present? %>
<ul class="alert alert-danger" role="alert">
<% @answer.errors.full_messages.each do |message| %>
<li><%= message %></li>
<% end %>
</ul>
<% end %>

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.

他エラーのパーシャルもあって、showでのみ使うエラー文だったため、パーシャル化しなかったのですが、やっぱりパーシャルのが見やすいですよね!変更します!

<%= form_with scope: :session, local: true do |f| %>
<div class="form-group">
<%= f.label :email %>
<%= f.text_field :email, class: 'form-control', id: 'sessions_email' %>

Choose a reason for hiding this comment

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

email_fieldを使いましょう。

Copy link
Owner Author

Choose a reason for hiding this comment

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

変更します。気づきませんでした。

def index
@q = Question.ransack(params[:q])
@questions = @q.result(distinct: true).page(params[:page])
@search_path = '/questions'

Choose a reason for hiding this comment

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

questions_pathのほうが良いですね。
基本的に文字列を直接書くのはプログラミング全般において推奨されてません。

Copy link
Owner Author

Choose a reason for hiding this comment

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

変更します!

<li class="media mb-3 border-bottom pb-3">
<div class="d-flex gap-3">
<div class="flex-shrink-0">
<% if question.user.image.attached? %>

Choose a reason for hiding this comment

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

N+1問題起きてるのでぜひ確認 & 解消してみてください。

Copy link
Owner Author

Choose a reason for hiding this comment

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

挑戦します!

) みたいにそれぞれpathに変更

N+1問題を修正
user/newでadminを登録出来ていたので、そのあたり修正
それぞれのformのところを、text_fieldとなっていたのでemail_fieldに修正
question/indexのquestionを新しい順に変更
@@ -5,7 +5,7 @@ def create
redirect_to admin_question_path(params[:question_id]), notice: '解答しました'
else
@question = Question.find(params[:question_id])
render 'admin/questions/show'
render admin_question_path(@question)

Choose a reason for hiding this comment

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

renderとredirectをちゃんと理解できてない気がします。
たぶんこれ動かない気がするんですが、動作試してもらえますか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

戻すの忘れてました。
一般の方でエラーが出て、調べてもpathでの書き方が見つからなく、
rails ガイドをみると、'admin/questions/show'こういう文字列になっていたので、
そちらに合わせていたのですが、、、、
変更しました!

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.

一般の方でエラーが出て、調べてもpathでの書き方が見つからなく、

ここの文面的に_pathが何を返す関数なのかを理解してなさそうな気がしました。_pathってどんな返り値でしょう?

Copy link
Owner Author

Choose a reason for hiding this comment

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

urlの扱いやすい版みたいな感じで考えていました。コンソールで調べると、

 pry(main)> app.users_path
=> "/users"

となっているので、urlの文字列が返り値です!
ネットでもう一度調べると、
pathは相対ルートパスとも書いていて、やっぱりurl を扱いやすくしたものなのかと思いました。

自分がいまいち理解してなかったのは、レンダーでした。
render :new の場合、レンダーは引数にハッシュを指定する。この場合、キー(template)に、値(erbファイルをさす文字列)
なので、erbファイルの位置を指定が必要で、urlを指定してもだめだったのですね!
合ってますでしょうか!?。。。

Choose a reason for hiding this comment

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

概ねあってます!

urlを扱いやすくしたもの

という表現はちょいと気になりますね。
URL, ホスト, ドメイン, パス, クエリなどを調べてみてください。その中のパスが_pathに該当します。

参考
https://developer.mozilla.org/ja/docs/Learn/Common_questions/What_is_a_URL

Copy link
Owner Author

Choose a reason for hiding this comment

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

確認しました。
urlのパスの部分を扱いやすくしたものが正解ですね!ざっくりしすぎてました。

@@ -22,11 +22,11 @@ def create
end

def edit
@user = User.find(params[:id])
@user = current_user.find(params[:id])

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.

動きませんでした!editについてあまり意識していなかったので、チェックし忘れてました。。。
変更します!

end

def update
@user = User.find(params[:id])
@user = current_user.find(params[:id])

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.

変更しました!

@@ -0,0 +1,7 @@
<% if answer.errors.present? %>

Choose a reason for hiding this comment

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

answerに特化しすぎなので object.errors.present?とかに命名を変更したほうが良くないでしょうか?
で、ファイルの置き場はviews/shared配下に置く感じですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

すいません。その場合、renderの変数部分は
<%= render partial: 'shared/alert',locals: {object: @object } %>
こんな感じになるかと思うのですが、answerモデルにどう紐付けするのでしょう!?

Choose a reason for hiding this comment

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

この質問で解決した認識で良いですかね?
https://tech-essentials.work/courses/7/tasks/54/outputs/1450

Copy link
Owner Author

Choose a reason for hiding this comment

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

すいません。大丈夫です。解決しました。

@@ -28,15 +28,15 @@
<div class="badge bg-success text-wrap">解決済み</div>
<% else %>
<div class="badge bg-danger text-wrap">未解決</div>
<% if session[:user_id].present? && question.user_id == current_user.id %>
<% if session[:user_id].present? && @question.user_id == current_user.id %>

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.

やってみます!
user_loginedみたいなヘルパーメソッドを作成してみます!!

Copy link
Owner Author

Choose a reason for hiding this comment

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

作成しました!!

Comment on lines 25 to 31
u = User.find(params[:id])
if current_user == u
@user = u
elsif
flash[:danger] = 'アカウントの編集権限がありません'
redirect_to questions_path
end

Choose a reason for hiding this comment

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

ちょっと冗長ですね。

@user = User.find(params[:id])
if current_user != @user
  flash[:danger] = 'アカウントの編集権限がありません'
  redirect_to questions_path
end

でいい気がしますね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど。ワンクションあっていいなーと思ったのですが、勉強なります!変更します!

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.

None yet

2 participants