招待URL作成時に研修生を支払い方法別に選択可能にした・研修生の場合は会員登録時の支払い方法の指定を必須にした #8013
Conversation
|
@mousu-a |
|
@ham-cap わからないところがあれば質問させていただきますのでその時はよろしくお願いいたします🙇♂️ |
app/models/user.rb
Outdated
| @@ -27,7 +27,9 @@ class User < ApplicationRecord | |||
|
|
|||
| INVITATION_ROLES = [ | |||
| [I18n.t('invitation_role.adviser'), :adviser], | |||
| [I18n.t('invitation_role.trainee'), :trainee], | |||
| [I18n.t('invitation_role.trainee(invoice_payment)'), :trainee_invoice_payment], | |||
There was a problem hiding this comment.
提案です！
I18n.t('invitation_role.trainee(invoice_payment)')このコードに関してです〜。
# ja.yml
invitation_role:
trainee: 研修生(${payment_method})
# app/models/user.rb
I18n.t('invitation_role.trainee', payment_method: 請求書払い)みたいな形でも良いかもしれません。判断はお任せします🙇♂️
There was a problem hiding this comment.
こちらご提案に従って修正してみました！🙏
| checkBoxes.forEach((checkBox) => { | ||
| if (checkBox) { | ||
| checkBox.addEventListener('click', () => { | ||
| if (!checkBox.checked) { |
There was a problem hiding this comment.
細かいですが1行で書けそうです！
if (!checkBox.checked) checkBox.checked = true
app/javascript/card.js
Outdated
| '.selectable-credit-card-box' | ||
| ) | ||
| if (selectableCreditCardCheckBox.checked) { | ||
| stripe.createToken(card).then(function (result) { |
There was a problem hiding this comment.
前の実装にならって、ということなのだと思いますが、特別な理由がなければファイル内でfunctionかアローどちらかに統一したいかもです！（今回変更を加えたコードだけでも）
（今は特別functionを使う理由もなさそうです）
app/controllers/users_controller.rb
Outdated
| @@ -68,6 +68,7 @@ def create | |||
| @user.free = true if @user.trainee? | |||
| @user.build_discord_profile | |||
| @user.payment_method_of_trainee = params[:user][:payment_method_of_trainee] | |||
| @user.credit_card_payment = params[:credit_card_payment] == '1' | |||
There was a problem hiding this comment.
すみません、このコードが何をやっているのかちょっと説明ほしいです🙏(72行目、73行目)
There was a problem hiding this comment.
72行目の@user.payment_method_of_trainee = params[:user][:payment_method_of_trainee]については、試行錯誤の痕跡が残ってしまっているだけですので、Userモデルのattr_accessorと併せて削除いたします🙏
73行目についてはcheck_box_tagにチェックが入っていた場合に送られてくる値1を真偽値に変換したうえで代入しているのですが、改めて確認したところ特に真偽値に変換せずとも動作には問題がないようなので、== '1'の部分を削除させていただきます。
@user.credit_card_payment自体はUserモデルの中でバリデーションをかける際の条件に使用しています。
|
|
||
| test 'GET /users/new as an adviser' do | ||
| visit '/users/new?role=adviser' | ||
| assert_equal 'FBCアドバイザー参加登録 | FJORD BOOT CAMP（フィヨルドブートキャンプ）', title |
There was a problem hiding this comment.
システムテストは重たいテストなので必要最小限に絞りたいかもです！
（GET /users/newのテストだけで良さそうに思います）
（今回追加した３つのシステムテスト（FBC参加登録以外のテスト）が今までなかったのは必要ないからかもしれません）
There was a problem hiding this comment.
今回のissueで求められている要件に沿ってタイトル表記のバリエーションを4種類に増やしているので、アドバイザー・研修生・メンター・一般受講生それぞれの登録フォーム上で正しく表示されているかどうかの確認は必要だと思うのですがいかがでしょうか👀
むしろ今までもアドバイザーとそれ以外の場合で2種類存在していたタイトル表記を片方しかチェックしていなかったのが不備だったような気もしています🤔
There was a problem hiding this comment.
むしろ今までもアドバイザーとそれ以外の場合で2種類存在していたタイトル表記を片方しかチェックしていなかったのが不備だったような気もしています🤔
そう言われればそうな気がしてきましたね🤔
一旦判断はkomagataさんにお任せしたいと思います！
| @@ -0,0 +1,54 @@ | |||
| function keepCheckBoxChecked(checkBoxes) { | |||
| checkBoxes.forEach((checkBox) => { | |||
| if (checkBox) { | |||
There was a problem hiding this comment.
checkBoxが存在しない場合はそもそもcheckBoxがクリックされることはないのでこのif文は必要ないかもです！
その場合checkBox?.addEventListenerとする必要があるかもしれません🤔
There was a problem hiding this comment.
引数checkBoxesの要素二つのうち少なくとも一つは必ずnullになるためif文で存在確認をしているのですが、たしかにオプショナルチェーンを使用すればnullが含まれていることを許容できるようになるのでそのように修正いたしました🙌
| '.selectable-credit-card-box' | ||
| ) | ||
| const checkBoxes = [selectableInvoiceCheckBox, selectableCreditCardCheckBox] | ||
| if (checkBoxes.includes(null)) { |
| @@ -35,8 +45,18 @@ document.addEventListener('DOMContentLoaded', () => { | |||
| // Create an instance of the card Element. | |||
| const card = elements.create('card', { style: style, hidePostalCode: true }) | |||
|
|
|||
| // Add an instance of the card Element into the `card-element` <div>. | |||
| card.mount('#card-element') | |||
| if (!userRole || checkedCreditCardCheckBox) { | |||
There was a problem hiding this comment.
こちらはStripeのカード番号入力欄のマウントを、/users/newにパラメーターなしでアクセスした場合(!userRoleがtrue)とページがレンダリングされた段階でクレジットカード払いにチェックが入っている場合(checkedCreditCardCheckBoxがtrue)に限定するためのものです。
マウントするタイミングを限定せず、単純に#card-elementにマウントするということでも機能としては動作するのですが、それだと招待URL作成ページの段階で請求書払いを選択しているとき等、#card-elementの要素が見当たらない場合にブラウザのコンソールにエラーが出てしまいます。
これを回避するために場合分けをしてマウントすべきタイミングを明示しています。
なお、チェックボックスがselectableな場合はチェックの状態によって動的にマウントする必要があるため、この直下でselectableCreditCardCheckBoxに対してイベントリスナーを登録することで対応しています。
また、この部分のselectableCreditCardCheckBoxの存在確認のためのif文については別途ご指摘いただいているとおりオプショナルチェーンで実装するのがベターだと思いますのでそのように修正させていただきます🙏
app/javascript/card.js
Outdated
| card.mount('#card-element') | ||
| } | ||
|
|
||
| if (selectableCreditCardCheckBox) { |
There was a problem hiding this comment.
selectableCreditCardCheckBox?.addEventListenerとすればこのif文は必要ないかもです！
| if (checkBoxes.includes(null)) { | ||
| return | ||
| } | ||
| const cardForm = document.getElementById('card') |
There was a problem hiding this comment.
こちらの変数名CreditCardFormではどうでしょうか。
cardFormではわかりにくいかもです🤔
There was a problem hiding this comment.
たしかにちょっと分かりにくいですね👀
修正しました！
・これまで管理者用の項目だった「請求書払いの設定」のUIを削除し、支払い方法（請求書orクレジットカード）を選択するためのチェックボックスを一般ユーザー向けの項目として新たに作成した。併せて、登録フォームに遷移する前の招待URL作成ページのロール欄でどれを選択したかによって、登録フォームが表示された初期状態でどちらの支払い方法にチェックが入っているか（または入っていないか）を振り分けるように修正した。
…装・チェックボックスのパーシャルの名称を変更 ・コントローラーで研修生の支払い方法に関する情報を@pay_by_invoiceと@pay_by_credit_cardに格納していたが、params[:role]の参照で事足りるため、これら二つのインスタンス変数は廃止した。 ・「研修生（支払い方法を選択）」を選んだ場合は参加登録フォーム上でチェックボックスのチェックを変更できることから、カード番号入力フォームの表示/非表示を切り替えができる必要があったため実装した。 ・チェックボックスのパーシャルは既存のUIを流用したため名称が_invoice_paymentのままになっていたが、実態に即していないため変更した。
…wアクション内の条件分岐の書き方を変更
・招待URL作成ページで「研修生（請求書払い）」を選択した場合は登録フォームの「請求書払い」のチェックボックスはtrueでなくてはならないため、そ
のバリデーションを追加した。
・ビューでparamsを直接呼び出すとURLに含まれるパラメーターに変更があった場合の対応が煩雑になる恐れがあるため、ビューで必要な情報はコントロー
ラーでインスタンス変数に代入して使用することとした。
・UsersControllerのnewアクションではparams[:role]の値によって分岐する処理をif文で記述していたが、case文で記述した方が可読性が高いと判断した
ため変更した。
・クレジットカードのチェックボックスのDOM要素を取得する際getElementsByNameを使用していたが、対象の要素が見つからない場合でも空のNodeListが返ってしまい、条件分岐が正しく動作しないため、要素が見つからない場合にnullを返すquerySelectorに置き換えた。
・招待URL作成ページで選択された支払い方法のチェックボックスは登録フォームが表示された時点でチェック済みになっているが、これまではそのチェックを外すことができてしまっていた。そのため、誤操作でチェックを外してしまい無駄にバリデーションエラーになる可能性も考えられたため、ユーザーがチェックを外せないようにした。
・バリデーションで引っかかった場合はrenderで再度newテンプレートが呼び出されるが、招待URL作成ページからの遷移でない場合は必要なパラメータがURLに含まれておらず、チェックボックスが操作不能になる不具合が出ていたため修正した。
・招待URL作成ページにて「研修生（支払い方法を選択）」が選択された場合、登録ページのチェックボックスの挙動が、 (1)「請求書払い」と「クレジットカード払い」のいずれか一方しかチェックできない、 (2)「クレジットカード払い」が選択されている時のみカード情報の入力フォームを表示する、 となるように実装した。 それに伴い、カード情報の入力フォームの表示/非表示の制御と2種類のチェックボックスの状態制御を同時に行う必要があったため、card-form-display-switcher.jsで行っていた処理をpayment-methods-check-boxes.jsに統合した。
・DOMツリーにクレジットカード情報の入力フォームが存在すると、カード情報が空欄の場合や有効な番号でない場合にフォーム全体の送信を止める機能が実装されているが、招待URL作成ページで「研修生（支払い方法を選択）」を選んだ場合、クレジットカードフォームが空欄の状態であってもフォームが送信されなければならない（請求書払いを選択している）状況があり得ることになるため、「クレジットカード払い」のチェックボックスにチェックがない場合には当該バリデーションを無効化するように変更した。
・「研修生(クレジットカード払い)」を選択した場合の登録フォームでStripeのカード番号の入力欄が正しくマウントされていなかったため修正。
・支払い方法を選択できる場合の登録フォームにあるselectableCreditCardCheckBoxにチェックが入っているかどうかがフォームをそのまま送信するかクレジットカードのバリデーションを通すかの条件となっており、請求書払いが確定している登録フォーム上では存在しない要素のプロパティを呼び出すことになってしまいエラーとなっていたため修正した。
・クレジットカードのバリデーションを作動させる条件にcheckedCreditCardCheckBoxが含まれておらず正常に動作していなかったため修正した。
・メンター用の登録フォームの場合は「FBCメンター参加登録」と表示されていなければならないところ、一般受講生と同じ「FBC参加登録」となってしまっていたため修正した。
・クレジットカード情報の入力を伴う他のテストに倣ってVCRでログをとるように修正した。
・引数checkBoxesの要素である二つのチェックボックスのうち少なくとも一つは必ずnullが渡ってくるためif文で存在確認をしていたが、オプショナルチェーン演算子(?.)を使用するほうがより簡潔に書けるため変更した。
ham-cap
force-pushed
the
feature/require-payment-method-selection-for-trainees-when-creating-invitation-url
branch
from
283312e
to
7c76d88
Compare
|
@mousu-a |
| @@ -181,6 +185,10 @@ class User < ApplicationRecord | |||
| validates :hide_mentor_profile, inclusion: { in: [true, false] } | |||
| validates :github_id, uniqueness: true, allow_nil: true | |||
| validates :other_editor, presence: true, if: -> { editor == 'other_editor' } | |||
| validates :invoice_payment, inclusion: { in: [true], message: 'にチェックを入れてください。' }, if: -> { role == 'trainee_invoice_payment' } | |||
There was a problem hiding this comment.
すみません、このコードなのですが、
if role == 'trainee_invoice_payment' {
validation invoice_paymentはtrueでなければならない
失敗したらmessageを表示
}ということをやっているのだと思いますが、「trueかどうか」だけを検証するためにinclusionを使うのは意図に合っていない気がします🤔
自分としては下記の様な形を考えています。
validate :custom_validation, if: -> { role == 'trainee_invoice_payment' }
private
def custom_validation
return invoice_payment == true
errors.add(:base, "エラーメッセージ")
endただその場合invoice_paymentの値をどうやって取得するのか、errorsにaddする形でメッセージを表示できるのか、などの問題は出てきそうです🤔
There was a problem hiding this comment.
こちらの件、個人的にはここでinclusionを使うことにそこまで違和感を覚えていないので、意図に合っていないと思われる理由についてもう少し詳しく教えていただけると嬉しいです🙏
自分なりの考えを先に書いておくと、今回のケースはカスタムバリデーションを使わなければならないほど複雑な処理を必要とするものではないと考えていて、むしろバリデーションの実行箇所とメソッドの定義場所が離れてしまうことによる可読性の低下のほうがデメリットとして大きく感じます。
また、inclusionの使い方について改めて確認してみました。
Railsガイドの説明によると
このヘルパーは、指定の集合に属性の値が含まれているかどうかを検証します。
とあるので、今回の「trueであることを確認する」というのもユースケースとしてそれほど逸脱したものではないと個人的には思います。
加えて、真偽値の存在チェックのためにはinclusion: [true, false]を使用すべきであることがpresenceオプションの項目で紹介されていることなども勘案すると、「真偽値がtrueであることを確認する」という目的のためにinオプションに[true]だけを渡して厳密に判定するという方法は、inclusionの標準的な使い方の範疇にあるように感じます。
あくまでドキュメントを参照した上での自分なりの解釈なので、見落としている点などがあれば教えてもらえると嬉しいです👀
There was a problem hiding this comment.
@ham-cap
返信ありがとうございます！
すみません、値が集合の中に含まれていることを確認するのにその集合がtrue1つだけ、というところに違和感を持ってしまった次第です。（２つ以上の要素を持つ配列を集合に指定するイメージを持っていました）
カスタムバリデーションを使わなければならないほど複雑な処理を必要とするものではない
理解出来ましたのでこれでApproveとさせていただきます〜！
There was a problem hiding this comment.
Issue
概要
現在、管理者用メニューにはアドバイザー、研修生、メンター用の招待URL作成ページがあり、所属企業、ロール（FBC内での会員種別）、コース（FBCで受講するコース）の三つを選択することでそれぞれに合った内容の会員登録ページへ飛ぶURLを作成することができます。
このPRでは、これまで一つだった研修生のロールを支払い方法別に三つに分割し、それぞれの支払い方法があらかじめ選択された会員登録ページを表示するように変更しました。
具体的には、管理者だけが閲覧・変更できる項目の「請求書払いの設定」欄を廃止し、研修生の会員登録ページにのみ表示される「支払い方法」欄を新たに設置して「請求書払い」と「クレジットカード払い」をチェックボックスで表示するようにしました。
招待URL作成時に選択したロールによってそのチェックボックスの初期状態や操作の可否を適切に指定して表示されるようになっています。
変更確認方法
招待URL作成ページ
feature/require-payment-method-selection-for-trainees-when-creating-invitation-urlをローカルに取り込むkomagataやmachida）でログインするhttp://localhost:3000/admin/invitation_urlにアクセスし、ロールのプルダウンに「アドバイザー」、「研修生（請求書払い）」、「研修生（クレジットカード払い）」、「研修生（支払い方法を選択）」、「メンター」の5つの選択肢が表示されていることを確認する。研修生（請求書払い）の会員登録ページ
/admin/invitation_url）で「研修生（請求書払い）」のロールを選択して作成したURLへアクセスする。を確認する。
研修生（クレジットカード払い）の会員登録ページ
/admin/invitation_url）で「研修生（クレジットカード払い）」のロールを選択して作成したURLへアクセスする。を確認する。
4242 4242 4242 4242」、有効期限「12 / 50」、セキュリティナンバー「111」を入力のうえ、「参加する」をクリックしてユーザー登録ができることを確認する。研修生（支払い方法を選択）の会員登録ページ
/admin/invitation_url）で「研修生（支払い方法を選択）」のロールを選択して作成したURLへアクセスする。を確認する。
Screenshot
招待URL作成ページ
変更前
変更後
会員登録ページ（請求書払い）
会員登録ページ（クレジットカード払い）
会員登録ページ（支払い方法を選択）