-
Notifications
You must be signed in to change notification settings - Fork 71
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
お問い合わせ一覧・個別ページを作成 #7387
お問い合わせ一覧・個別ページを作成 #7387
Conversation
60d29f2
to
3b29a13
Compare
@machida |
@a-kuroki-gs お待たせしました🙇♂️ デザインの調整を入れましたー |
.card-list-item__row | ||
.a-meta | ||
p | ||
= inquiry.body.truncate(100) |
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.
@machida
デザイン入れていただき、ありがとうございます!
1点こちらのコードについて確認なのですが、
Issueでは、「一覧画面においてメール本文は200字程度表示する」仕様になっていたかと思います。
こちらデザインに合わせて仕様を修正した、という認識でよろしいでしょうか?
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.
@a-kuroki-gs すいません、伝え忘れてました🙇♂️ ここは一旦100文字に変更でお願いします。思ったより200文字が多かった...
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.
@machida
承知しました!ご連絡ありがとうございます🙏
お疲れ様です! ご確認よろしくお願いいたします🙏 |
わかりました〜明日から時間をとって確認させていただきます! |
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.
お待たせしてしまい申し訳ないです🙇♂️
確認方法上の動作は問題なさそうでした👍
いくつかコメントさせていただきましたので、ご確認のほどよろしくお願いいたします🙇♂️
= link_to admin_inquiry_path(inquiry), class: 'card-list-item-title__link a-text-link' do | ||
| #{inquiry.name} 様 (#{inquiry.email}) |
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.
📝 こちらミーティングで確認したところ、この動きで良いとのことでした。
.page-content-header__end | ||
.page-content-header__row | ||
h1.page-content-header__title | ||
| #{@inquiry.name}(#{@inquiry.email}) |
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.
細かいのですが、indexの個別のカード方では#{inquiry.name} 様 (#{inquiry.email})
になっているので、こちらもその表示に合わせてもいいのかなと思いました👀
(machidaさんが手を入れられたところだと思うので、確認の上での対応になるかもですが)
その上で、もしindexと共通の表示にする場合は、この部分はDecoratorでメソッドにして使い回すのが良いかなと思いました。
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.
個別と一覧での表示を合わせることにしました。
またご指摘いただいたように、
その上でDecoratorを新たに作成し、テストも追加しております👍
test/system/admin/inquiries_test.rb
Outdated
|
||
test 'show inquiry details' do | ||
visit_with_auth '/admin/inquiries', 'komagata' | ||
all('.card-list-item__inner')[0].click |
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.
別でコメントした「タイトル部分のみがリンクになっていること」と絡むのですが、現状のテストの場合、ここで詳細ページへのリンクを踏めていないと思います。
(パスしているのは、結果的にindex上でもアサーションできているからだと思います。)
もしタイトル部分のみをリンクにするのであれば、テストの意義の観点から、ここでクリックする要素を変更して、正しく詳細ページに遷移するようにすべきかな思います👀
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.
以下のように、クリックできる要素でテストするよう修正しました。
find('.card-list-item-title', text: 'inquiry1 様 ([email protected])').click |
test/fixtures/inquiries.yml
Outdated
inquiry1: | ||
name: 'inquire1' | ||
email: '[email protected]' | ||
created_at: '2017-01-01 00:00:00' | ||
body: 'お問い合わせのテスト1です。' | ||
|
||
inquiry2: | ||
name: 'inquire2' | ||
email: '[email protected]' | ||
created_at: '2017-01-01 00:00:00' | ||
body: 'お問い合わせのテスト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.
開発環境にもデータを追加し、inquire
となっていたスペルミスをinquiry
に修正しました。
6c9942e
to
0e32b0c
Compare
@junohm410 |
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.
お疲れ様です。修正点確認いたしました!大丈夫だと思います。
最後に一点だけ、前回のご指摘させていただいた点以外で気になったことがあったのでコメントいたしました。こちらは本来、初回のレビューで気づくべきでした🙇♂️申し訳ないです。
ご確認のほど、何卒よろしくお願いいたします🙏
def index | ||
@inquiries = Inquiry.all | ||
end |
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.all
だと順番が保証されないので、Inquiry.order(created_at: :desc)
にした方が良さそうだと思いましたがいかがでしょうか?
(updated_at
でソートしている他のリソースもありますが、お問い合わせは現状更新されることはなさそうなのでcreated_at
でいいと考えました。)
こちら、本来は前回のレビューで気づくべきでした🙇♂️申し訳ないです。
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.
@junohm410
ありがとうございます!
確かにおっしゃる通り、all
ではなく順序を指定して取得すべきでした。
(
updated_at
でソートしている他のリソースもありますが、お問い合わせは現状更新されることはなさそうなのでcreated_at
でいいと考えました。)
こちらに関しては私もcreated_at
を基準にする方針で良いと思います。
気付けなかった点でしたので、ご指摘いただけて助かりました!ありがとうございます。
再度修正しておりますので確認よろしくお願いいたします👍
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.
お疲れ様です。ご対応ありがとうございました🙏
問題ないと思いましたので、私の方からはApproveとさせていただきます🙏
@junohm410 @komagata |
# frozen_string_literal: true | ||
|
||
module InquiryDecorator | ||
def sender_name_and_email |
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.
👍
|
||
class InquiryDecoratorTest < ActiveDecoratorTestCase | ||
setup do | ||
@inquiry = decorate(inquiries(:inquiry1)) |
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.
👍
test/system/admin/inquiries_test.rb
Outdated
assert_text inquiries(:inquiry1).body | ||
end | ||
|
||
test 'click on the link of back to inquiries index' do |
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.
system testはとても重い時間のかかる処理なので、リンクのテストとかはしなくて大丈夫です~。
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.
@komagata
リンクのテストを削除しております。
ご確認よろしくお願いいたします🙏
598cb59
to
a25728e
Compare
|
||
class Admin::InquiriesController < AdminController | ||
def index | ||
@inquiries = Inquiry.order(created_at: :desc) |
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.
orderをちゃんとしていしていて、created_atにしてるのがよいとおもいます~!
span.a-meta__value | ||
= l inquiry.created_at |
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.
span.a-meta__value | |
= l inquiry.created_at | |
span.a-meta__value = l inquiry.created_at |
短い行は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.
p | ||
= inquiry.body.truncate(100) |
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.
p | |
= inquiry.body.truncate(100) | |
p = inquiry.body.truncate(100) |
h1.page-header__title | ||
= title |
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.
h1.page-header__title | |
= title | |
h1.page-header__title = title |
h1.page-main-header__title | ||
| お問い合わせ一覧 |
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.
h1.page-main-header__title | |
| お問い合わせ一覧 | |
h1.page-main-header__title お問い合わせ一覧 |
span.a-meta__label | ||
| 受信日 | ||
span.a-meta__value | ||
= l @inquiry.created_at |
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.
span.a-meta__label | |
| 受信日 | |
span.a-meta__value | |
= l @inquiry.created_at | |
span.a-meta__label 受信日 | |
span.a-meta__value = l @inquiry.created_at |
お問い合わせのタブが増えたので、その分を修正
一覧画面のViewにのみリンクを追加 要素を正しくネストさせるために、共通しているコードもひとまず分割する
共通部分のファイル名は_inquiry_commonにする
decoratorのテストも追加
29da23b
to
056f3c7
Compare
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.
確認させていただきました。OKです~👌
Issue
概要
管理ページにお問い合わせ一覧ページ・詳細ページを追加。
変更確認方法
feature/add-inquiry-index-page
をローカルに取り込むforeman start -f Procfile.dev
でアプリを起動するお問い合わせ
のタブがあることお問い合わせ
のタブをクリックすると、お問い合わせ一覧画面にお問い合わせが表示されていることお問い合わせ一覧へ
をクリックすると、お問い合わせ一覧画面に遷移することScreenshot
変更前
変更後