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

修了プラクティスと修了したプラクティスの数がずれている問題の解消 #7699

Merged
merged 9 commits into from
May 20, 2024

Conversation

kyokucho1989
Copy link
Contributor

@kyokucho1989 kyokucho1989 commented Apr 16, 2024

Issue

概要

プラクティスを修了した数が、場所によって異なる問題がありましたが、これは仕様でした。
両者の差異をわかりやすくするため、表記方法とデザインを修正します。

現状

スクリーンショット 2024-04-20 22 57 43

左の「修了プラクティス」と右の「修了したプラクティス」の数が異なる。

修正後

数を以下のように定義する。

A = 修了したプラクティスの数
B = 修了したプラクティスの内、必須の数
C = コースの全体の必須のプラクティスの数

左の修了プラクティス

A (必須:B)

右の修了したプラクティス

通常:B/C*100 %
クリック後:修了: A (必須: B/C)

ユーザー一覧ページの進捗の部分もデザインが変わっています。
クリックすると % ⇔ x/x 表記に切り替わります。

変更確認方法

  1. bug/mismatch_complete_practice_counts をローカルに取り込む
  2. bin/setup を実行してseedデータを投入する
  3. 任意のアカウントでログインしてユーザー一覧ページへアクセス
  4. 進捗率が%表示になっていることを確認
  5. %表記をクリックすると修了数の内訳が表示されることを確認
  6. ユーザー詳細ページ(user:harikiroくん) users/527754862 へアクセス
  7. %表記をクリックすると修了数の内訳が表示されることを確認
  8. ユーザー登録情報の修了数が表示されていることを確認

Screenshot

変更前

スクリーンショット 2024-04-20 22 57 43

変更後

スクリーンショット 2024-05-26 4 17 51

クリックすると表記が切り替わります。
gif2

@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch from 49328aa to 966cb5c Compare April 16, 2024 19:17
@kyokucho1989 kyokucho1989 marked this pull request as ready for review April 16, 2024 19:41
@kyokucho1989
Copy link
Contributor Author

@machida
おはようございます。こちら、実装が修了しました。デザインの調整をお願いしたいです。

@machida
Copy link
Member

machida commented Apr 17, 2024

@kyokucho1989 了解ですー

@machida
Copy link
Member

machida commented Apr 19, 2024

@kyokucho1989 デザイン入れましたー

@machida machida removed their assignment Apr 19, 2024
@kyokucho1989
Copy link
Contributor Author

@machida
ありがとうございます!

@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch 2 times, most recently from 065f5e9 to e635d01 Compare April 20, 2024 11:52
@kyokucho1989
Copy link
Contributor Author

@hirano-vm4
こんにちは! こちらのPRについてレビューしていただけるとありがたいです!

@hirano-vm4
Copy link
Contributor

@kyokucho1989

お疲れ様です!レビュー承知しました🙆

明日から順次確認させていただきます〜!

@@ -465,7 +465,11 @@ def completed_percentage
end

def completed_fraction
Copy link
Contributor

@hirano-vm4 hirano-vm4 Apr 22, 2024

Choose a reason for hiding this comment

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

completed_fractioncompleted_fraction_in_metasはコードしてはとしては問題ないと思いました。

ただ、userモデルがかなりfatな状態なので、データを整形してviewの表示のために使うメソッドであればdecoratorに置くことも検討してもよいのかな?と感じましたがいかがでしょうか?👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解です!トライしてみます!

@hirano-vm4
Copy link
Contributor

@kyokucho1989

お疲れ様です!コード確認しました。表示やコードの中身はLGTMと思いました👀

1点だけコメントさせてもらったので、確認いただければと思います!

@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch 2 times, most recently from ebcef74 to b60258b Compare April 23, 2024 11:04
@kyokucho1989
Copy link
Contributor Author

@hirano-vm4
おはようございます。
修正しました!再度確認をお願いします!

@non_required_subject_completed_user.completed_practices << practices(:practice5)
@non_required_subject_completed_user.completed_practices << practices(:practice61)

assert_equal @non_required_subject_completed_user.completed_fraction_in_metas, fraction_in_metas
Copy link
Contributor

@hirano-vm4 hirano-vm4 Apr 24, 2024

Choose a reason for hiding this comment

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

Suggested change
assert_equal @non_required_subject_completed_user.completed_fraction_in_metas, fraction_in_metas
assert_equal fraction_in_metas, @non_required_subject_completed_user.completed_fraction_in_metas

テスト内容は良いと思いました🙆

細かい点ですが、assert_equal メソッドを使用する際には、第一引数に「期待する値」を、第二引数に「テスト対象の値」を置くのが一般的なイメージなので、上記のようにするのはどうでしょうか?

以前、私もどこかのプラクティスで教えていただいたので共有となります🙏

賛否もあるようですが、completed_fraction のテストはこのようになっているので、統一されるかな?とも思いました😄

参考:assertEqualsやassert_equalの引数はなぜ expected, actual の順なのか、調べてみた

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解しました!変更しました!

@hirano-vm4
Copy link
Contributor

hirano-vm4 commented Apr 24, 2024

@kyokucho1989

お疲れ様です!遅くにすいません🙏

内容はほぼ個人的には良いと思いました。少し細かい点になりますが1点だけコメントさせてもらいました!これで最後になるかと思います。

私の認識違いもある可能性もあるので、その際はご指摘ください🙆

@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch from b60258b to c93a1cb Compare April 24, 2024 20:11
@kyokucho1989
Copy link
Contributor Author

@hirano-vm4
修正しました!確認のほど、よろしくお願いします

Copy link
Contributor

@hirano-vm4 hirano-vm4 left a comment

Choose a reason for hiding this comment

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

ありがとうございました!私からはApproveします〜🙆引き続きよろしくお願いいたします!

@kyokucho1989
Copy link
Contributor Author

@hirano-vm4
ありがとうございました!

@komagata
メンバーの方のレビューが終了しました。
レビューをお願いします!

@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch from c93a1cb to bd5e434 Compare April 28, 2024 20:22
kyokucho1989 and others added 6 commits May 10, 2024 05:39
必修ではない(進捗計算に使わない)プラクティスのテストについて
変数の中身が変わったため、今までと意味合いが同じになるように修正した
修了プラクティスの表記もデザイン変更に伴い変更されたのでそちらも修正
テストの記載場所もusermodelからdecoratorへ移動
@kyokucho1989 kyokucho1989 force-pushed the bug/mismatch_complete_practice_counts branch from bd5e434 to fa5b1da Compare May 9, 2024 20:40
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 0cc0ca1 into main May 20, 2024
2 checks passed
@komagata komagata deleted the bug/mismatch_complete_practice_counts branch May 20, 2024 05:40
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.

None yet

4 participants