-
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
お試し延長 開始・終了機能を作成 #4030
お試し延長 開始・終了機能を作成 #4030
Conversation
6e3cba4
to
9a78d96
Compare
@maeda-seina |
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.
@napple29 さん
細かく丁寧にコミットされていて、変更意図がわかりやすかったです!🙇♂️
また、issue・PRに「あえて変更していないところ」の記述がわかりやすく書かれていてとても理解しやすかったです🙏
PRに載せていただいている変更箇所は実際に手を動かして確認したところオッケーだと思いました!
細かいところにはなりますが、何点かコメントしておりますので、ご確認のほどよろしくお願いいたします。
(命名の部分につきましては、変更しなくてよさそうと判断されたらそのままにしておいてください🙏 )
test/system/admin/campaigns_test.rb
Outdated
assert_text 'お試し延長を更新しました。' | ||
assert_text '春のお試し祭り' | ||
assert_text (yesterday).strftime("%Y年%m月%d日(#{wd[yesterday.wday]}) %H:%M") | ||
assert_text (five_days_later).strftime("%Y年%m月%d日(#{wd[five_days_later.wday]}) %H:%M") |
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.
46行目の(yesterday)
と47行目の(five_days_later)
についてですが、29行目のtoday
と30行目のseven_days_later
は()
がないので、どちらかで統一した方が良いのではないか?と思いました!
test/models/campaign_test.rb
Outdated
class CampaignTest < ActiveSupport::TestCase | ||
test 'recently campaign' do | ||
later_campaign = campaigns(:campaign2) | ||
earlier_campaign = campaigns(:campaign1) |
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.
こちらの、later_campaign
とearlier_campaign
の変数の命名についてなんですが、laterは「後の」という意味があり、earilerは「以前の」という意味があるので、どちらが前のキャンペーンで、どちらが後のキャンペーンなのか少しわかりづらい気がしました。
現在開催されているものが、:campaign1
で、過去に開催されたものが:campaign2
ということで、少し考えたところ現在のものがlatest_campaign
とかcurrent_campaign
、過去のものがpast_campaign
とかが直感的にわかりやすいかもしれない?と思いました!
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.
参考になります、、🙏🏻修正しました!
9a78d96
to
c714228
Compare
@maeda-seina |
@napple29 さん 確認させて頂きました!👍 |
@maeda-seina @komagata |
test/fixtures/campaigns.yml
Outdated
# Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html | ||
|
||
campaign1: | ||
start_at: <%= Time.zone.today %> |
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.
start_at: <%= Time.zone.today %> | |
start_at: <%= Time.current %> |
こちらの方がスマートかもです〜
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/campaigns_test.rb
Outdated
test 'update a campaign' do | ||
yesterday = Time.zone.today - 1.day | ||
five_days_later = Time.zone.today + 5.days | ||
wd = %w[日 月 火 水 木 金 土] |
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.
複数回出てくるのであればテストクラスの定数にしちゃった方がいいかもです〜
with_options if: -> { start_at && end_at } do | ||
validate :end_at_be_greater_than_start_at | ||
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.
Rails 7から日付の比較に ComparisonValidator
を使えるようになるので、コメントを残しておきたいなと思いました。
こうしておけば、Rails 7に上げた後に気づいた人が直してくれるかなと思うので。
# TODO: Rails 7に更新後、 `ComparisonValidator` を使うように直す。
# refs: https://github.com/rails/rails/pull/40095
# validates :end_at, greater_than: :start_at
with_options if: -> { start_at && end_at } do
validate :end_at_be_greater_than_start_at
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.
@sinsoku
ありがとうございます!!教えていただいた通り、コメントで追記させていただきました!勉強になりました🙇🏻♀️
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.
Rails 7から日付の比較に ComparisonValidator を使えるようになるので、
ヘェ〜知らなかった😲
c714228
to
01d9dd0
Compare
@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.
conflictの修正お願いします〜
d652791
to
93c526a
Compare
93c526a
to
e12555b
Compare
@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ですー🙆♂️
with_options if: -> { start_at && end_at } do | ||
validate :end_at_be_greater_than_start_at | ||
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.
Rails 7から日付の比較に ComparisonValidator を使えるようになるので、
ヘェ〜知らなかった😲
@machida コードレビューはOKです。デザイン面の確認お願いできれば〜 |
@komagata |
@machida レビューを無事終えられましたので、デザインをお願いします 念の為管理画面のデザインの確認をお願いしたいです。
以下は現状だと該当部分をif文でキャンペーン適用中か分岐させて表示が変更されるようになっています。
↓また、関連がありそうなファイルです。
|
デザイン入れていきますー |
#3910 お試し延長 開始・修了機能
概要
お試し延長キャンペーンの開始・終了機能を作成しました。
お試し延長キャンペーンは料金支払いサービスStripeと関連しており、ユーザーのお試し終了日などはStripeのAPIから情報を得られるため、このIssueでは「◯年◯月◯日〜✖️年✖️月✖️日にキャンペーンがある」また、「そちらと連動してトップページなどが切り替わる」という部分を実装しました。
※今回のPRでは、キャンペーン中のお試し延長日数を7日間として表示していますが、今後この部分はパーシャルにして一括で置き換えられる予定です!
管理画面(お試し延長登録)
お試し延長キャンペーンを行った際に、管理ページでキャンペーン期間(◯年◯月◯日〜✖️年✖️月✖️日)とタイトルを設定できるよう実装しました
変更前
お試し延長タブなし
変更後
一覧
新規登録画面
管理画面(お試し延長ユーザー)
変更前
変更後
トップページ・管理画面・ウェルカムページ
管理画面でお試し延長を設定することで以下が自動的に切り替わるよう実装しました。
各ページ、既存の画面と変わりはないので、キャンペーン適用内外で比較しています。
トップページ
キャンペーン外
キャンペーン中
料金ページ
キャンペーン外
http://localhost:3000/pricing
にアクセスキャンペーン中
参加登録
キャンペーン外
キャンペーン中
ウェルカムぺージ
キャンペーン外
キャンペーン中
動作確認手順
お手数ですが、
talk
に関連するseed
がこのブランチには取り込まれていないので、最新のmain
をpull --rebase
したローカルブランチにこちらをpull
してくださるようお願いします🙇🏻♀️feature/function-about-extension-of-trial-period
をローカルの任意のブランチにpullする新たにCampaignテーブルを作成したため、
$ rails db:migrate
を行う$ rails db:seed
を行うと、seedした時点で入会したユーザー(お試し 延長さん)が新たに作成されるお試し延長キャンペーンのデータがないサイトの状態を確認する
お試し延長のデータがあるサイトの状態を確認する
http://localhost:3000/admin/users
のお試し延長タブの一覧に該当ユーザーが表示されているかに加えて、4の項目
確認項目が多くお手数をおかけしてすみませんが、よろしくお願いします!
不明点があればお知らせください🙇🏻♀️