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

日報が0件の場合、日報ダウンロードボタンを非表示に修正 #4128

Merged
merged 3 commits into from
Feb 8, 2022
Merged

日報が0件の場合、日報ダウンロードボタンを非表示に修正 #4128

merged 3 commits into from
Feb 8, 2022

Conversation

tksmasaki
Copy link
Contributor

@tksmasaki tksmasaki commented Feb 3, 2022 •
edited
Loading

Issue #4113

変更前

image

変更後

image

ローカル環境での確認方法

  • feature/not-display-report-dl-btn-when-no-reports ブランチをローカル環境で起動する
  • 日報ダウンロードボタンの表示を確認
    • ユーザー hajime でログイン
    • ダッシュボード > 自分の日報 を確認
  • 日報ダウンロードボタンの非表示を確認
    • ユーザー kananashi でログイン
    • ダッシュボード > 自分の日報 を確認

@tksmasaki tksmasaki self-assigned this Feb 3, 2022
@tksmasaki tksmasaki marked this pull request as ready for review February 3, 2022 07:33
@tksmasaki
Copy link
Contributor Author

@eatplaynap
お手すきの際に、レビューをお願いします🙏

@tksmasaki tksmasaki requested a review from eatplaynap February 3, 2022 07:39
eatplaynap
eatplaynap reviewed Feb 4, 2022
View reviewed changes
Copy link
Contributor

@eatplaynap eatplaynap left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tksmasaki
実装お疲れ様です！機能は問題なさそうでしたが、2点だけ気になったのでご確認ください🙏
確認手順が書いてあってすごくレビューしやすかったので、自分も取り入れようと思いました〜！

  • テストの名前(詳細はコメント)
  • PRに貼るスクショは↓みたいにタブから日報が0件であることが分かるほうが分かりやすいかも？
    image

test/system/current_user/reports_test.rb Outdated
@@ -7,4 +7,16 @@ class CurrentUser::ReportsTest < ApplicationSystemTestCase
visit_with_auth '/current_user/reports', 'hatsuno'
assert_equal '自分の日報 | FJORD BOOT CAMP（フィヨルドブートキャンプ）', title
end

test 'show reports download btn when reports is present' do
Copy link
Contributor

Choose a reason for hiding this comment

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

テスト追加してるのさすがです！
ここは文法的にはwhen reports is presentよりwhen a report is presentが適切かな？と思いました💪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに変な文章になってますね... ご指摘ありがとうございます！
改めて考えてみたのですが、テスト対象のユーザーは複数の日報があったので when reports exist ではどうでしょうか？ 違和感がありましたら、教えていただきたいです🙇‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

テスト対象のユーザーは複数の日報があったので when reports exist

確かにこちらのほうがテスト対象を正確に表現していて良いと思います！

test/system/current_user/reports_test.rb Outdated Show resolved Hide resolved
@tksmasaki
Copy link
Contributor Author

@eatplaynap

PRに貼るスクショは↓みたいにタブから日報が0件であることが分かるほうが分かりやすいかも？

こちら参考にさせていただき、修正しました！

@eatplaynap
Copy link
Contributor

eatplaynap commented Feb 4, 2022 •
edited
Loading

@tksmasaki
修正ありがとうございます〜！PRわかりやすくなったと思います〜！
テスト名の修正はまだPushされてない感じですかね？
Pushしたらまた確認しますのでメンションお願いします😄

@tksmasaki tksmasaki force-pushed the feature/not-display-report-dl-btn-when-no-reports branch from 2cdd34a to 7a67f9c Compare February 4, 2022 05:52
@tksmasaki
Copy link
Contributor Author

@eatplaynap
テスト名を修正したものをプッシュしました。
お手すきの際に、確認よろしくお願いします🙇‍♂️

@eatplaynap eatplaynap self-requested a review February 5, 2022 02:12
eatplaynap
eatplaynap approved these changes Feb 5, 2022
View reviewed changes
Copy link
Contributor

@eatplaynap eatplaynap left a comment

Choose a reason for hiding this comment

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

@tksmasaki
修正ありがとうございます〜
LGTMです😄

@tksmasaki tksmasaki requested a review from komagata February 5, 2022 05:58
@tksmasaki
Copy link
Contributor Author

@komagata
お手すきの際に、レビューよろしくお願いします🙏

komagata
komagata reviewed Feb 5, 2022
View reviewed changes
test/system/current_user/reports_test.rb Outdated
@@ -7,4 +7,16 @@ class CurrentUser::ReportsTest < ApplicationSystemTestCase
visit_with_auth '/current_user/reports', 'hatsuno'
assert_equal '自分の日報 | FJORD BOOT CAMP（フィヨルドブートキャンプ）', title
end

test 'show reports download btn when reports exist' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test 'show reports download btn when reports exist' do
test 'show reports download button when reports exist' do

特に理由がなければ略さない方がいいかも？

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
特に理由なく略していました... 気をつけます🙇‍♂️
修正したものをプッシュしたので、再度確認よろしくお願いします！

@tksmasaki tksmasaki force-pushed the feature/not-display-report-dl-btn-when-no-reports branch from 7a67f9c to 781e25c Compare February 5, 2022 08:40
komagata
komagata approved these changes Feb 8, 2022
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 dcb93b7 into main Feb 8, 2022
@komagata komagata deleted the feature/not-display-report-dl-btn-when-no-reports branch February 8, 2022 12:16
@github-actions github-actions bot mentioned this pull request Feb 8, 2022
Merged
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eatplaynap eatplaynap eatplaynap approved these changes

@komagata komagata komagata approved these changes

Assignees

@tksmasaki tksmasaki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tksmasaki @eatplaynap @komagata