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

Q&Aを削除する際に表示する確認ダイアログの文言を変更 #4981

Merged
merged 3 commits into from
Jun 24, 2022

Conversation

ai-24
Copy link
Contributor

@ai-24 ai-24 commented Jun 9, 2022

Issue

概要

Q&Aを削除する際に表示する確認ダイアログの文言を変更しました。

変更確認方法

  1. ブランチfeature/change-confirm-prompt-to-delete-questionをローカルに取り込みます
  2. bin/rails sでローカル環境を立ち上げます
  3. kimuraでログインします
  4. http://localhost:3000/questions/new から質問を作成します
  5. 作成した質問の詳細画面に遷移し「削除する」をクリックします
  6. 確認ダイアログが「自己解決した場合は削除せずに回答を書き込んでください。本当に削除しますか?」と表示される事を確認してください

変更前

スクリーンショット 2022-06-09 12 21 52

変更後

スクリーンショット 2022-06-09 12 16 29

@ai-24 ai-24 self-assigned this Jun 9, 2022
@ai-24 ai-24 marked this pull request as ready for review June 9, 2022 03:35
@ai-24 ai-24 requested a review from Saki-htr June 9, 2022 03:36
@ai-24
Copy link
Contributor Author

ai-24 commented Jun 9, 2022

@Saki-htr
お疲れ様です。
お忙しいところ恐れ入りますが、レビューお願いできますでしょうか?🙇‍♀️

Copy link
Contributor

@Saki-htr Saki-htr left a comment

Choose a reason for hiding this comment

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

@ai-24 さん
お待たせしてすみません
動作確認したところOKでした🙆‍♀️

dismiss_confirmを使うと、指定したダイアログが表示された時にキャンセルの操作が行えるのですね。勉強になりました!

私の方からはApproveさせていただきます☺️

@ai-24
Copy link
Contributor Author

ai-24 commented Jun 14, 2022

@Saki-htr
お忙しい中レビューありがとうございました!🙇‍♀️

@komagata
チームメンバーのレビューが通りましたので、レビューよろしくお願いいたします!

@ai-24 ai-24 requested a review from komagata June 14, 2022 01:53
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.

(間違えました)

@@ -394,4 +394,11 @@ class QuestionsTest < ApplicationSystemTestCase
end
assert_link 'Linuxのファイル操作の基礎を覚える'
end

test 'show confirm dialog before delete' do
Copy link
Member

Choose a reason for hiding this comment

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

これってassertがひとつもないような感じなのですが、それだとチェックになってないかもです〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほどです。
私もassertが0なのは気になったのですが、確認ダイアログが空文字だったり、文言が異なっていると下記のようにエラーでテストが落ちるので、dismiss_confirmでテスト出来ていると考えたのですが、assertを使ったテストの方が良いでしょうか。

スクリーンショット 2022-06-16 11 51 57

スクリーンショット 2022-06-16 11 52 55

@komagata komagata self-requested a review June 15, 2022 14:53
@ai-24
Copy link
Contributor Author

ai-24 commented Jun 16, 2022

@komagata
レビューありがとうございます🙇‍♀️
1点質問させていただきました!
ご確認よろしくお願いいたします🙏

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.

テストは基本的にassertで何かを確かめて、それがFailなのかSuccessなのかを確かめる(テストする)ものになっています。

エラーというのはテスト自体が壊れてるという意味なのでもちろん避けたいのですが、assertしていないと何もテストしていないことになってしまいます。

また、なんのテストなのかという主題もわかりやすくなります。

@ai-24 ai-24 force-pushed the feature/change-confirm-prompt-to-delete-question branch from ce95f6d to 102f977 Compare June 22, 2022 03:26
@ai-24
Copy link
Contributor Author

ai-24 commented Jun 22, 2022

@komagata
お疲れ様です。

エラーというのはテスト自体が壊れてるという意味

今までテストの失敗とエラーの違いがあまりよく理解できていなかったと思います。
学びになりました🙇‍♀️ご教示いただきありがとうございます!

テストにassertionを追加いたしましたので、再度ご確認いただけますでしょうか。
よろしくお願いいたします。

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 d1e1886 into main Jun 24, 2022
@komagata komagata deleted the feature/change-confirm-prompt-to-delete-question branch June 24, 2022 00:21
@github-actions github-actions bot mentioned this pull request Jun 24, 2022
22 tasks
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