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.

Sign up for GitHub

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

Jump to bottom

プラクティスの進捗を % でなく、完了プラクティスの個数 / プラクティス全体の個数 として表示する #3191

Merged
merged 6 commits into from
Sep 6, 2021
Merged

プラクティスの進捗を % でなく、完了プラクティスの個数 / プラクティス全体の個数 として表示する #3191

merged 6 commits into from
Sep 6, 2021

Conversation

rjtt17
Copy link
Contributor

@rjtt17 rjtt17 commented Aug 25, 2021 •
edited
Loading

issue: 3150
概要：プラクティスの進捗について、 % でなく、完了プラクティスの個数 / プラクティス全体の個数 として表示するように変更しました

変更前

image

変更後

image

@rjtt17 rjtt17 self-assigned this Aug 26, 2021
@rjtt17 rjtt17 force-pushed the feature/change-practices-progress branch from d1cd8f3 to 49870b8 Compare August 29, 2021 02:38
@rjtt17 rjtt17 requested a review from konaga-k August 30, 2021 04:38
@rjtt17
Copy link
Contributor Author

rjtt17 commented Aug 30, 2021

@konaga-k お時間ある際にレビューお願いいたします🙏

konaga-k
konaga-k reviewed Aug 30, 2021
View reviewed changes
Copy link
Contributor

@konaga-k konaga-k left a comment

Choose a reason for hiding this comment

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

@rjtt17

  • 動作確認はOKです
  • NITSの箇所は修正お願いします
  • MAYの箇所はこのPRで直すべきか迷うので対応おまかせします。対応していいか分からなかったら、お手数ですが駒形さんに確認してください

app/views/api/users/_list_user.json.jbuilder Outdated Show resolved Hide resolved
app/decorators/user_decorator.rb Outdated
Comment on lines 53 to 57
def cached_completed_fraction
Rails.cache.fetch "/model/user/#{id}/completed_fraction" do
completed_fraction
end
end
Copy link
Contributor

@konaga-k konaga-k Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

[感想] 進捗度ってキャッシュを使ったほうが良いんですね
文脈はわからないですが、過去に専用のPRを出して対応しているくらいなので必要な処理なんだなあという理解です

Copy link
Contributor Author

Choose a reason for hiding this comment

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

キャシュ使った方がいいか分からなかったのと使い方が怪しかったので、こちらのメソッドを削除しました〜

Copy link
Contributor

@konaga-k konaga-k Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

自分もキャッシュを使うべきかはわからなくて申し訳ないんですが、パーセントの進捗度はキャッシュしていたことを考えると、分数の進捗度もキャッシュしたほうが良い可能性はありますね
後に @komagata さんにレビューしてもらうと思うんですが、パーセントの進捗度キャッシュを実装していたみたいなので、どういう背景でキャッシュしていたのか含めて、分数でもキャッシュが必要か確認してみるといいと思います
（レビュータイミングでコメントしてもらえることを期待して駒形さんにメンションを付けてみる）

test/models/user_test.rb Outdated Show resolved Hide resolved
@konaga-k
Copy link
Contributor

konaga-k commented Aug 30, 2021 •
edited
Loading

あとdescriptionに貼ってある変更前/変更後の画像なのですが、差分が明確になるように同じ進捗率で揃えてもらいたいです🙏
（60%と30/50のような感じ）

@rjtt17
Copy link
Contributor Author

rjtt17 commented Aug 30, 2021

レビューありがとうございました🙇‍♂️ 修正していきます！

@rjtt17 rjtt17 force-pushed the feature/change-practices-progress branch from 49870b8 to 86aef64 Compare August 31, 2021 12:36
@rjtt17
Copy link
Contributor Author

rjtt17 commented Sep 1, 2021 •
edited
Loading

@konaga-k 修正しましたので、お時間ある際に再度レビューお願いいたします🙏

@rjtt17 rjtt17 marked this pull request as ready for review September 1, 2021 12:24
@rjtt17 rjtt17 requested a review from konaga-k September 1, 2021 12:24
konaga-k
konaga-k suggested changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@konaga-k konaga-k left a comment

Choose a reason for hiding this comment

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

@rjtt17
コメント返しました〜
SHOULDは対応お願いします
MAYは引き続き対応するかお任せします（既存のテストの変更になるため）

test/models/user_test.rb Outdated Show resolved Hide resolved
app/decorators/user_decorator.rb Outdated
Comment on lines 53 to 57
def cached_completed_fraction
Rails.cache.fetch "/model/user/#{id}/completed_fraction" do
completed_fraction
end
end
Copy link
Contributor

@konaga-k konaga-k Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

自分もキャッシュを使うべきかはわからなくて申し訳ないんですが、パーセントの進捗度はキャッシュしていたことを考えると、分数の進捗度もキャッシュしたほうが良い可能性はありますね
後に @komagata さんにレビューしてもらうと思うんですが、パーセントの進捗度キャッシュを実装していたみたいなので、どういう背景でキャッシュしていたのか含めて、分数でもキャッシュが必要か確認してみるといいと思います
（レビュータイミングでコメントしてもらえることを期待して駒形さんにメンションを付けてみる）

@rjtt17 rjtt17 force-pushed the feature/change-practices-progress branch from 86aef64 to 9272fd0 Compare September 3, 2021 00:43
@rjtt17
Copy link
Contributor Author

rjtt17 commented Sep 3, 2021

@konaga-k
コメントいただいた部分について、修正致しました！
お手すきの際に再度ご確認お願いします🙇‍♂️

■修正内容の詳細は以下です。

[SHOULD] practice52のfixturesは元に戻す
practice52のfixturesは元に戻しました。

[MAY] テストではpractice52ではなくpractice55を採用する。その場合はcompleted_percentage の方も修正する
二つのテストとも practice55を採用し、修正しました。

@rjtt17 rjtt17 requested a review from konaga-k September 3, 2021 13:07
konaga-k
konaga-k approved these changes Sep 4, 2021
View reviewed changes
Copy link
Contributor

@konaga-k konaga-k left a comment

Choose a reason for hiding this comment

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

OKです〜
LGTM

komagata
komagata requested changes Sep 4, 2021
View reviewed changes
app/models/user.rb
@@ -339,15 +339,13 @@ def away?
end

def completed_percentage
Copy link
Member

Choose a reason for hiding this comment

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

パーセントが不要ならこのメソッド自体不要かも？

Copy link
Contributor

@konaga-k konaga-k Sep 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ピンク色のプログレスバーを表示する計算でまだ必要になると思ってたんですがどうでしょう

@rjtt17
実はいらなかったりします？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata
CC: @konaga-k
ご確認ありがとうございました🙇‍♂️
プログレスバーを表示している、app/views/users/practices/_completed_practices_progress.html.slim やapp/javascript/user-practice-progress.vue 等でパーセント使っているので、残しておいたのですが、不要でしょうか？

Copy link
Member

Choose a reason for hiding this comment

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

@rjtt17 そちらを見逃していました。それだったら問題ございません〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata ご確認ありがとうございました🙇‍♂️

komagata
komagata approved these changes Sep 6, 2021
View reviewed changes
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 3b6f9f3 into main Sep 6, 2021
@komagata komagata deleted the feature/change-practices-progress branch September 6, 2021 10:21
@github-actions github-actions bot mentioned this pull request Sep 6, 2021
Merged
14 tasks
@obregonia1 obregonia1 mentioned this pull request Sep 24, 2021
Closed
@kyokucho1989 kyokucho1989 mentioned this pull request Apr 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konaga-k konaga-k konaga-k approved these changes

@komagata komagata komagata approved these changes

Assignees

@rjtt17 rjtt17

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rjtt17 @konaga-k @komagata