-
Notifications
You must be signed in to change notification settings - Fork 72
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
個別のお問い合わせにコメントがつけられるようにした #8262
base: main
Are you sure you want to change the base?
Conversation
59b517e
to
e796126
Compare
@@ -22,7 +22,12 @@ def create | |||
@comment.user = current_user | |||
@comment.commentable = commentable | |||
if @comment.save | |||
render :create, status: :created | |||
# Inquiry以外はvueでJSONを使用している。コメント機能のJS変換が完了すれば下の'Inquiry'による分岐は不要なので削除してください。 | |||
if params[:commentable_type] == 'Inquiry' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらのInquiry
による分岐ですが、bootcampアプリ自体の機能のための分岐ではなく、「コメント機能をvueからJS、Hotwireに変更していく過程」で発生している分岐です。
本来は不必要な分岐のため、上記のようにコード内コメントで注釈をつけています。
@@ -35,7 +35,7 @@ def params_for_keyword_search(searched_values = {}) | |||
end | |||
|
|||
def receiver | |||
commentable.user | |||
commentable.respond_to?(:user) ? commentable.user : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今までのcommentable
に該当するモデルはすべてuser
カラムを持っていましたが、Inquiry
は外部からの問い合わせのため、user
カラムを持っていません。
そのため、commentable
のuser
を使用した処理について、Inquiry
のコメントが該当しないようにcomment.rb
やafter_create_callback.rb
にて変更を加えています。
@Judeeeee さん、お疲れ様です🍵 (追記) |
@ayu-0505 |
@Judeeeee さん、お疲れ様です🍵 |
e796126
to
68c437e
Compare
@@ -15,6 +15,17 @@ | |||
Capybara.disable_animation = true | |||
Minitest::Retry.use! if ENV['CI'] | |||
|
|||
Capybara.register_driver :selenium_chrome_headless_with_clipboard do |app| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ヘッドレスブラウザーではnavigator.clipboard
メソッドが使用できないので、使用できるよう設定したドライバーを追加しました。
クリップボードテストの時のみ使用します。
611919b
to
c9f979d
Compare
@sugiwe 急ぎではありませんので、お手隙のときにご確認いただけたらと思います。 |
@ayu-0505 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayu-0505
お疲れ様です☕️ 先日はペアプロありがとうございました!
ひと通り確認しまして、ブラウザ上での動作確認は全てうまく行きました!説明文章もわかりやすくて助かりました◎
気になった箇所などにコメントを入れましたので、ご確認をお願いいたします🙏
@@ -15,7 +15,7 @@ def set_user | |||
def set_comments | |||
@comments = | |||
Comment | |||
.where.not(commentable_type: 'Talk') | |||
.where.not(commentable_type: %w[Talk Inquiry]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
相談部屋内のコメントは除外するという処理を書き換えて、問い合わせ内のコメントもまとめて除外するようにしたということですよね!
この処理によってコメント一覧ページで取得するコメントとしては意図したものになっているようですが、ユーザーページのヘッダータブ(「プロフィール」「ポートフォリオ」など並んでいる部分)の「コメント(99)」の数量の部分において、お問い合わせ内のコメントの数が加算されてしまっているようです👀
そして、偶然なのですが最近担当したレビューで確認した箇所で似たような処理(相談部屋のコメントがヘッダータブのコメント数に加算されてしまっていたのを除外する処理)があり、これと絡めてうまく処理できそうなので記載させていただきます📝
app/models/comment.rb
内にwithout_talk
というスコープ定義がありまして、ここでまさに今回修正をしていただいたcommentable_type: 'Talk'
が残っています。- この部分を今回修正していただいた
%w[Talk Inquiry]
に修正し、さらに、withput_talk
というスコープ名は今回別箇所で命名されていたprivate_comment
がとてもわかりやすいと思いましたので、withput_private_comment
というスコープ名に変更されると良さそうです - その上で、
.where.not(commentable_type: %w[Talk Inquiry])
を.withput_private_comment
とすると、ヘッダータブの部分の「コメント(99)」の数字の部分が意図通りにカウントされるのではと思います - 上記に加えて、
without_talk
というスコープ名で設定をしている箇所が複数(3ファイル5か所くらいのはずです)ありますので、これらを一通りwithput_private_comment
という名称に変更する必要もあるかと思います
変更ファイルも増えていくので、without_talk
のスコープ名変更までここでまとめてやるべきかどうかというのがちょっと自信がないのですが、Inquiry
の追加ありきの修正なのでまとめてやるしかないのかなとも思っております、ご検討をお願いできますでしょうか🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!
この部分、全く気づいていなかったので大変ありがたいです🙏
変更ファイル自体は増えてしまいますが、Inquiryに関わる部分であることは間違いないので今回で修正しました。
grepで確認できた以下のファイルを変更しました。
テストも通過し、目視確認もできたので、変更できているかと思います。
$ grep 'without_talk' -r ./
.//app/models/comment.rb: scope :without_talk, -> { where.not(commentable_type: %w[Talk Inquiry]) }
.//app/views/users/_activity_counts.html.slim: = link_to_if !user.comments.without_talk.empty?,
.//app/views/users/_activity_counts.html.slim: user.comments.without_talk.size, user_comments_path(user)
.//app/helpers/page_tabs/users_helper.rb: comment_count = user.comments.without_talk.length
.//test/models/comment_test.rb: test '.without_talk' do
.//test/models/comment_test.rb: non_talk_comment_count = Comment.without_talk.count
app/javascript/new-comment.js
Outdated
import CSRF from 'csrf' | ||
import TextareaInitializer from 'textarea-initializer' | ||
import MarkdownInitializer from 'markdown-initializer' | ||
import initializeComment from 'initialize_comment.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize_comment.js
というファイル名ですが、別ファイルのinitializeAnswer.js
に合わせてキャメルケースのinitializeComment.js
にしてはいかがでしょうか?
命名については場所によってハイフンやアンダースコアなどかなり混ざってしまっている現状で、全体を整理するのは不可能に近いと思うのですが、似た機能の命名は寄せておくと良いかもと思いました、もし理由があっての命名でしたらそのままでも問題ないとは思いますので、ご検討お願いいたします!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たしかに、(大部分を参考にしたことで結果的に)機能が似通ったinitializeAnswer.js
とネーミングを合わせることで、コードの類似を読む人に連想させやすくなりそうです。
こちらもなかなか思いつかなかった視点なので助かります🙏
app/javascript/new-comment.js
Outdated
import { toast } from './vanillaToast.js' | ||
|
||
document.addEventListener('DOMContentLoaded', () => { | ||
const comment = document.querySelector('.new-comment') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
上と同様の話になるのですが、comment
という変数名はnewComment
にしておくと、似たファイルのnewAnswer
と命名が揃ってわかりやすいかなと思ったのですが、いかがでしょうか👀
(これも些細な話ではあるので、作業全体の検討の中でcomment
にしたいという判断でしたらそのままでも問題ないと思います!)
@@ -5,7 +5,7 @@ def after_commit(comment) | |||
if comment.commentable.class.include?(Watchable) | |||
create_watch(comment) | |||
notify_to_watching_user(comment) | |||
elsif comment.sender != comment.receiver | |||
elsif comment.receiver.present? && comment.sender != comment.receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この部分ですが、comment.receiver
があるかどうかの条件を追加したことにより、この行全体としてcomment.receiver
が以下のように2回登場した状態となっております。
- 前半では「
comment.receiver
があるかどうか」を確認している(comment.receiver
が主語) - 後半では「
comment.sender
がcomment.receiver
と異なるかどうか」を確認している(comment.sender
が主語)
ロジックとしては問題はないと思いますが、日本語で言うと「comment.receiverが存在していて、なおかつ、comment.sender が comment.receiverと異なるものだったら」ということで主語が前半と後半で違っていますので、以下のようにcomment.sender
とcomment.receiver
を入れ替えるのはいかがでしょうか?
elsif comment.receiver.present? && comment.sender != comment.receiver # 変更前
elsif comment.receiver.present? && comment.receiver != comment.sender # 変更後
こうすると、
「comment.receiverが存在していて、なおかつ、comment.sender とは異なるものだったら」
となり、comment.receiver
が主語として統一されて読みやすくなるのではと思いました。
(どちらにせよcomment.receiver
が2回登場しておりますので、1回だけの記載にできないかも考えたのですが、ちょっとトリッキーになるようでしたので、そこまでしなくて良さそうでした)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たしかに、 comment.receiver
を主語として統一した方が理解がスムーズになりそうなので、こちらも変更いたしました🙏
@@ -30,7 +30,7 @@ def search(word, document_type: :all) | |||
result_for(document_type, word).sort_by(&:updated_at).reverse | |||
end | |||
|
|||
delete_comment_of_talk!(searchables) # 相談部屋の内容は検索できないようにする | |||
delete_private_comment!(searchables) # 相談部屋とお問い合わせのコメント内容は検索できないようにする |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「相談部屋とお問い合わせのコメント」をまとめるにあたり、comment_of_talk_and_inquiry
とかでなくprivate_comment
にしたのは素敵な命名だなと思いました✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そう言っていただけると嬉しいです✨
c9f979d
to
30fa3b9
Compare
@sugiwe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayu-0505
お疲れ様です☕️
大変お待たせしてしまいすみませんでした💦
修正ありがとうございました!
1点だけ反映漏れかな?と思う部分がありましたのでコメントいたしました。
ご確認よろしくお願いいたします!
app/models/comment.rb
Outdated
@@ -18,7 +18,7 @@ class Comment < ApplicationRecord | |||
|
|||
mentionable_as :description, hook_name: :after_commit | |||
|
|||
scope :without_talk, -> { where.not(commentable_type: 'Talk') } | |||
scope :without_private_comment, -> { where.not(commentable_type: 'Talk') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちら、変数名は修正できているのですが where.not
で除外すべきcommentable_typeが Talk
だけのままになっているようです👀(別箇所で設定されていたように、配列で Talk
とInquiry
の2つを除外する感じかと思います✨)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
後ほどリファクタリングを行う
30fa3b9
to
cfb0173
Compare
Issue
概要
個別のお問い合わせページにおいて、コメントがつけられるようにしました。
現在コメント機能を提供しているVueファイルを使用せずに実装するために、slimファイルやJSファイルを新規で作成しています。
(また、大まかな実装方針としてはQ&AのAnswer部分を非Vue化 #8033を参考にしております。)
いくつか変更点にて追加でコメントしています。
変更確認方法
feature/add-comment-form-in-inquiry
をローカルに取り込む。git fetch origin pull/8262/head:feature/add-comment-form-in-inquiry
(2度目以降は--force
をつけてください)git switch feature/add-comment-form-in-inquiry
rails db:seed
で初期データにする。foreman start -f Procfile.dev
でローカルサーバーを立ち上げる。コメントを投稿しました!
のトーストが数秒表示されることを確認。Copied
と数秒表示され、クリップボードにハッシュパラメータ付きURLが保存される。本当に宜しいですか?
のダイアログが表示される。(以下のコードを
rails c
で行うと素早くできます。userはmachidaさん、commentable_id
はinquiry1 に該当しています。)前のコメント(8/コメント全件数)
、または前のコメント(未表示コメント件数)
という表示があることを確認。Screenshot
変更前
変更後