-
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
[アンケート機能]アンケート用の質問の一覧、作成、編集ができるようにしたい #5370
Conversation
@SatoshiHaramura |
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.
@daiki0381
お疲れさまです。
ボリュームが多いので、まだ全てを確認出来ていないのですが、取り急ぎテーブルとView周りを確認してコメントさせていただきましたので、ご確認お願い致します🙏
また、その他のコードも並行して確認していきますので、随時コメントさせていただこうと思います🙏
あと、質問形式
の段落
と記述式
の違いが分からなかったのですが、何かご存知でしょうか?(もしかして、質問形式
という文言ではなくて、回答形式
という文言なのかなとも思ったりしました)
それにしても、アンケート機能は壮大ですね😅
| 新規作成 | ||
- @survey_questions.each do |survey_question| | ||
div | ||
= survey_question.question_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.
質問一覧のスクショを見たところ、タイトル
の他に以下の項目も表示しておく必要があるのかなぁとも思いました。(今回のスコープ内でしょうか?)
- 形式(ラジオボタンや段落など)
- 作成日時
- 更新日時
- 実施
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.
こちらeb39e37で修正しました。実施に関してはsurveyの範疇なので、今回のissueでは表示していないです。
li.checkboxes__item | ||
= f.check_box :reason_for_choice_required, class: 'a-toggle-checkbox js-warning-form' | ||
= f.label :reason_for_choice_required 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.
スクショの文言では、「選択した理由が必要な
場合はチェック」となっていそうです。
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.
こちら8b7dda7で修正しました。
li.checkboxes__item | ||
= f.check_box :reason_for_choice_required, class: 'a-toggle-checkbox js-warning-form' | ||
= f.label :reason_for_choice_required 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.
スクショの文言では、「選択した理由が必要な
場合はチェック」となっていそうです。
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.
こちら8b7dda7で修正しました。
t.text :question_description | ||
t.integer :question_format, default: 0 | ||
t.boolean :answer_required, default: false | ||
t.references :user, foreign_key: true |
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.
こちら2d7bab4で修正しました。
fe7d3f3
to
a4aa88d
Compare
@machida 【相談事項】
|
段落を選択した場合は、下記の画像のようなアンケートフォームの質問の回答部分が |
ご説明ありがとうございます🙇♂️ |
@daiki0381 回答遅れてすいません🙇♂️メンターは誰でも編集可能でお願いします🙏 |
6d96934
to
2d7bab4
Compare
@SatoshiHaramura |
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.
@daiki0381
お疲れさまです。
修正ありがとうございました!
1通りコードを見まして、理解が不足している点がありましたので質問させて頂きました🙏よろしくお願い致します。
td(style="border: 1px solid black; padding: 5px 15px;") | ||
= survey_question.question_title | ||
td(style="border: 1px solid black; padding: 5px 15px;") | ||
= "作成: #{survey_question.created_at.strftime("%Y年%-m月%-d日 (#{%w[日 月 火 水 木 金 土][survey_question.created_at.wday]})")}" |
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.
formatオプションでlongを指定すると、より簡潔になるかと思いましたので、よろしければご検討お願い致します🙏
= "作成: #{survey_question.created_at.strftime("%Y年%-m月%-d日 (#{%w[日 月 火 水 木 金 土][survey_question.created_at.wday]})")}" | |
= "作成: #{l survey_question.created_at, format: :long}" |
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.
ご提案ありがとうございます🙏 lメソッドを使用するとすっきり書けるんですね✨ 勉強になりました!こちら9f7c3c0で修正しました!
def create | ||
@survey_question = SurveyQuestion.new(survey_question_params) | ||
@survey_question.creator_id = current_user.id | ||
switch_initialization |
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.
switch_initializationメソッドが、ここで必要な理由が分からなかったので、お手数ですが教えていただきたいです🙏
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.
こちら削除しても問題なく機能したため51a05cdで削除しました。紛らわしくて申し訳ないです🙇♂️
app/models/user.rb
Outdated
@@ -126,6 +126,18 @@ class User < ApplicationRecord | |||
through: :organizers, | |||
source: :regular_event | |||
|
|||
has_many :create_questions, |
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.
create_questionsが必要な理由が分からなかったので、お手数ですが教えて頂きたいです🙏
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.
こちらは下記のように1:多の関係でuserの外部キーを2つ登録しているので、アソシエーションを2つ設定しています。外部キーは別名で登録しているので、class_name: 'SurveyQuestion'としています。またcreate_questionsという命名は分かりにくいと思うので、39e541cで修正しました。
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.
@daiki0381
UserモデルとSurveyQuestionモデルの1:多の関連付けであれば、Userモデルには以下のような関連付けのみでよさそうな気もするのですが、いかがでしょうか??
has_many :survey_questions, dependent: :destroy
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.
@daiki0381
失礼致しました🙇♂️外部キーを2つ登録していると、こうなってしまうのですね😓
またcreate_questionsという命名は分かりにくいと思うので、39e541cで修正しました。
あと、名前を変更されたようですが、user.creators
はユーザーの作成者、user.updaters
はユーザーの更新者と訳されると思います。ただ、実態はアンケート質問なので、誤認しやすいなと思いました。
何か適切な命名があればなぁと思いましたがいかがでしょうか。
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.
あと、名前を変更されたようですが、user.creatorsはユーザーの作成者、user.updatersはユーザーの更新者と訳さると思います。ただ、実態はアンケート質問なので、誤認しやすいなと思いました。
その通りですね💦 こちらa5ee7feで修正しました!
@SatoshiHaramura |
@daiki0381 |
@SatoshiHaramura |
@daiki0381 |
@SatoshiHaramura |
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.
@daiki0381
お疲れさまです。
テストについてコメントさせて頂きましたので、ご確認お願い致します🙏
|
||
require 'application_system_test_case' | ||
|
||
class SurveyQuestionsTest < ApplicationSystemTestCase |
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とeditも作成されているので、併せてそれらのテストケースも追加しておいた方がよいかなと思いましたがいかがでしょうか?
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.
こちら98caef4で修正しました。
@SatoshiHaramura ありがとうございます🙇♂️ ご指摘頂いた箇所を修正しましたので、再度レビューお願いします🙏 |
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.
@daiki0381
ご対応ありがとうございました!
動作とコードを確認して大丈夫そうだと思いましたのでApproveさせて頂きます🙆♂️
大規模な機能を実装されて、シンプルにすごいなぁと思いました😄
テーブルの設計やJSでの表示/非表示の方法など、多くのことを学ばせて頂きました😊
@SatoshiHaramura こちらこそご丁寧にレビューして頂きありがとうございました🙇♂️ |
@machida |
@daiki0381 デザイン了解ですー |
@machida ありがとうございます🙇♂️ 承知しました! |
98caef4
to
279198d
Compare
279198d
to
478f5a2
Compare
543bcdb
to
98350fa
Compare
@komagata レビューありがとうございます🙇♂️ ご指摘頂いた箇所を修正しましたので、再度レビューお願いします🙏 |
@komagata こちらレビューが通ったらマージをお願いします🙏 |
@machida こちら個別の箇所で議論が続いている感じです〜。レビュー完了までもうしばらく時間がかかりそうです〜 |
@komagata 了解ですー |
@komagata レビューありがとうございます🙇♂️ ご指摘頂いた箇所を修正しましたので、再度レビューお願いします🙏 |
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です〜🙆♂️
@komagata ありがとうございます! |
Issue
概要
アンケート (survey) が複数の質問 (survey_questions) を持つ。アンケートを作成する際に、そのアンケートでどの質問をするかを survey_questions の中からいくつか選択するというのを構想している。このissueではsurvey_questionsを作る。
質問の形式は以下の5種類
質問が共通で持つもの
それ以外の持つ情報は質問の形式で異なる。
ナビゲーション
質問一覧
質問作成・編集
段落
記述式
ラジオボタン
下記はラジオボタンを選択している状態で入力していないとバリデーションがかかる。
チェックボックス
下記はチェックボックスを選択している状態で入力していないとバリデーションがかかる。
均等目盛
下記は均等目盛を選択している状態で入力していないとバリデーションがかかる。
選択した理由
ラジオボタン、チェックボックス
ラジオボタン、チェックボックスは、その選択肢の中でその項目を選択した場合は、それを選択した理由を聞くことができるようにする。理由を聞く場合は、その選択肢に「選択した理由が必要な場合はチェック」にチェックを入れる。
均等目盛
均等目盛の場合は、どれを選択したかに関わらず、それを選択した理由を聞くか、聞かないかを選択する。
理由を聞く場合は、「選択した理由が必要な場合はチェック」にチェックを入れる。
質問個別
質問個別ページはその質問を実施したアンケート一覧である。現時点では、アンケート一覧表示は不要。=> 今回のissueでは対応しない。
ER図
変更確認方法
feature/create_survey_questions
をローカルに取り込むbin/rails s
でローカル環境を立ち上げる変更後
質問一覧
質問作成
段落
記述式
ラジオボタン
チェックボックス
均等目盛
質問編集
段落
記述式
ラジオボタン
チェックボックス
均等目盛