Skip to content
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

管理画面のお試し延長一覧にユーザー数を表示するようにした #6206

Merged
merged 6 commits into from
Feb 18, 2023

Conversation

lef237
Copy link
Contributor

@lef237 lef237 commented Feb 11, 2023

Issue

概要

管理画面のお試し延長一覧にユーザー数を表示するようにしました。

研修生を除いた生徒の中で、お試し延長期間のキャンペーン時に入った生徒の数を表示します。

左から、入会者、現役生、退会者に分かれています。

変更確認方法

  1. feature/number-of-users-for-trial-campaignをローカルに取り込む
  2. 念のためrails db:resetをおこなう
  3. bin/setupをおこなう
  4. rails sして、komagataさんでログインする。
  5. http://localhost:3000/admin/campaignsへとアクセスする
  6. 入会者、現役生、退会者が、それぞれ11, 10, 1になっていることを確認する

Screenshot

変更前

仮にお試し延長延長のキャンペーンを作成しても、以下のような画面になる。
image

変更後

このブランチにチェックアウトしてから動作確認すると、このような画面になる。
image

@lef237 lef237 force-pushed the feature/number-of-users-for-trial-campaign branch from ed6a9b4 to fe4b0cc Compare February 11, 2023 10:56
@lef237 lef237 changed the title Feature/number of users for trial campaign 管理画面のお試し延長一覧にユーザー数を表示するようにした Feb 11, 2023
@lef237 lef237 self-assigned this Feb 11, 2023
@lef237 lef237 marked this pull request as ready for review February 11, 2023 11:24
@lef237 lef237 requested a review from shizimi50 February 11, 2023 11:26
@lef237
Copy link
Contributor Author

lef237 commented Feb 11, 2023

@shizimi50
お疲れ様です! こちら、レビューをお願いしてもよろしいでしょうか?
(もしご都合が悪ければ遠慮なく仰って大丈夫です!)

@shizimi50
Copy link
Contributor

@lef237
レビュー依頼ありがとうございます。
明後日までの確認でも問題ないでしょうか。🙌🏻

@lef237
Copy link
Contributor Author

lef237 commented Feb 11, 2023

@shizimi50
大丈夫です! 1週間でも2週間でも、時間が掛かっても大丈夫です!👌

Copy link
Contributor

@shizimi50 shizimi50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lef237
テストに関して少しだけ気になったのでコメントさせていただきました🙏
動きついては、問題なく動作しておりましたので、その点OKかと思います!

visit_with_auth admin_campaigns_path, 'komagata'
assert_text '入会者'
assert_text '現役生'
assert_text '退会者'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テキスト表示を確認するだけの場合は、システムテストを作成する必要はない気がしました🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かに必要ありませんでした👀 削除しました!

title: "お試し延長期間のテスト"
created_at: Sat, 11 Feb 2023 12:02:58.965327000 JST +09:00
updated_at: Sat, 11 Feb 2023 12:02:58.965327000 JST +09:00
trial_period: 7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらのテストデータですが、追加いただいたテストデータを使っている箇所がないように思ったので、追加する必要はないのかなと思ったのですがいかがでしょう。

また、もしテストデータを追加するようでしたら、

campaign2:
start_at: "2021-03-20 00:00:00"
end_at: "2021-03-31 23:59:00"
title: '二つ前に終了したキャンペーン'
trial_period: 5

他のテストデータと時間の形式を合わせるのが良いのかなと思いました!🙋

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

削除致しました!

また、db/fixtures/campaigns.ymlファイルについても、他のテストデータと時間の形式を合わせるように修正致しました🕓
https://github.com/fjordllc/bootcamp/pull/6206/files#diff-529d6ca31b9f4c8ee8ee098bd691a049f27be697de976b56df27263e51b6166aR2-R3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lef237

因みに、私のローカルではタイムゾーンの問題か、登録時とはズレた状態で時間が表示されているのですが、
lef237 さん側では、どうでしょうかいかがでしょうか。
スクリーンショット 2023-02-14 22 10 29

今回、lef237さんに修正いただいた箇所以外の時間もずれているので、db/fixtures/campaigns.yml に問題があるわけではないというのはわかるのですが、、、🤔
もしわかればと思い、お聞きしました🙏

Copy link
Contributor Author

@lef237 lef237 Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ご質問ありがとうございます。

結論から言うと、問題ないと思います!

Adminが管理しているFBC参加者の登録日時が全員09:00になっているように、fixtures内で00:00:00を指定すると、9時間遅れた09:00:00として表示されます。

自分も09:00:00がスタートになっています。JavaScriptのカレンダーのプラクティスのときに、WSL2(Debian)の設定時刻を、標準時のロンドンに合わせていたので、それが理由かもしれません。

Bootcampのアプリ内で、config.time_zone = "Tokyo"になっているので、この開始日と終了日で大丈夫だと思います🙆
(自分もそのように表示されています)

image

このような形で参加者の登録日時も全部09:00になっていると思います。

そのため、テストデータのユーザーであるyameoも、2014年01月01日(水) 09:00が登録日時として表示されていると思います。

補足ですが、既にOutdatedになっているWed, 01 Jan 2014 00:00:00.000000000 JST +09:00のような書き方をすると、強制的に画面表示をDescriptionのようにすることができますが、他のデータとの兼ね合いも考えると、2014-01-01 00:00:00で書くのが良いと改めて思いました。

改めて結論を言うと、このままで大丈夫だと思います👌

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lef237
返信遅くなりすみません🙏
質問への回答ありがとうございます!
改めて、自分と同じ状況下で皆さん開発されていると理解できました😀

本件、動作には問題なかったのと、fixtureの件も修正いただきましたのでapproveさせていただきます!

@lef237 lef237 force-pushed the feature/number-of-users-for-trial-campaign branch from fe4b0cc to 62f330d Compare February 14, 2023 00:55
@lef237
Copy link
Contributor Author

lef237 commented Feb 14, 2023

@shizimi50
レビューありがとうございます!
コメント頂いた箇所について、修正致しました。よろしくお願いします🙏

@shizimi50 shizimi50 self-requested a review February 14, 2023 13:43
@lef237
Copy link
Contributor Author

lef237 commented Feb 15, 2023

@shizimi50
ご質問に関してコメント返し致しました!
よろしくお願いします🙏

@lef237
Copy link
Contributor Author

lef237 commented Feb 15, 2023

@shizimi50
丁寧にレビューして頂き、ありがとうございました!🙌

@lef237
Copy link
Contributor Author

lef237 commented Feb 15, 2023

@komagata
お疲れ様です!
メンバーレビューが完了したため、リーダーレビューをお願い致します🙏

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit aca0b86 into main Feb 18, 2023
@komagata komagata deleted the feature/number-of-users-for-trial-campaign branch February 18, 2023 02:24
@github-actions github-actions bot mentioned this pull request Feb 18, 2023
10 tasks
@@ -53,6 +59,14 @@ header.page-header
| #{l campaign.start_at}
td.admin-table__item-value.is-text-align-right
| #{l campaign.end_at}
td.admin-table__item-value.is-text-align-right
= students_joined_by_campaign = User.students.where(created_at: campaign.start_at..campaign.end_at).count
Copy link
Contributor Author

@lef237 lef237 Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メモ📝:ここのコードにミスがありました。

具体的には、
https://github.com/fjordllc/bootcamp/pull/6206/files?diff=unified&w=0#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bL256-L266

このモデルメソッドを使っており、「退会していない現役生」を取ってきているため、「退会していない現役生のうちキャンペーン中に入った人数」になってしまっています。

本来であれば、この63行目のコードは「キャンペーン中に入った全ての生徒数」になっていなければなりません。

そのため、次のコードは、

https://github.com/fjordllc/bootcamp/pull/6206/files?diff=unified&w=0#diff-e1a23c503e3d27e805ae0dc92e589ae1d8e90ca1a99da8973fac0576a89d5abaR66

「退会していない現役生のうちキャンペーンに入った人数」から「キャンペーン中に参加した全ての生徒のうち退会した生徒」を引き算してしまっているため、マイナスの値が生じてしまっています。

#6188 (comment)

この問題を解決するためには、

https://github.com/fjordllc/bootcamp/pull/6206/files#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bL262-L264

この3行をなくした形で、データ探索をする必要があります。

修正したPull Requestを、こちらで作成しました。
#6267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants