日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装 #8061

su-su-su-su · 2024-09-10T14:04:04Z

元々メンターが日報にコメントをだけ入れて確認済みにしないまま画面を離れようとしたらアラートを出す。
というイシューだったのですが、ブラウザの仕様によりbeforeunloadイベントではカスタムメッセージを表示することができないことがわかりました。
そのことを町田さんに相談させていただいて、確認済になっていないとき、Bではなく、Aをクリックしたら、
「日報を確認済みにしていませんがよろしいですか？」
とアラートを出るようにしたいということで進めました。

Issue

【メンター向け機能】日報にコメントをだけ入れて確認済みにしなかったらアラートを出す #6928

概要

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装しました。

変更確認方法

feature/alert-for-unconfirmed-daily-report-commentをローカルに取り込む
foreman start -f Procfile.devでローカル環境を立ち上げる
machidaでログイン
未チェックの日報をクリック
コメントを書いてコメントするをクリック
「日報を確認済みにしていませんがよろしいですか？」とアラートが出ることを確認

Screenshot

変更前

変更後

念のため、以下の２つも確認しました。
メンター以外がコメントしてもアラートが出ないことも確認しました。

メンターが日報が以外にコメントしてもアラートが出ないことを確認しました。

su-su-su-su · 2024-09-10T14:22:15Z

@Judeeeee
お疲れ様です。
可能でしたらお手隙の際に、こちらのPRのレビューをお願いできますでしょうか？
ご都合が合わない場合はおっしゃってください🙇‍♂️
どうぞよろしくお願いいたします!

Judeeeee · 2024-09-11T12:09:49Z

@su-su-su-su
お疲れ様です〜！承知しました🙆‍♀️
できるだけ早くレビューするつもりですが、1週間目処で考えていただけると幸いです🙏

su-su-su-su · 2024-09-12T02:13:48Z

@Judeeeee
お疲れ様です！ありがとうございます🙇‍♂️
全然急ぎでないので、お手隙の際にレビュー頂ければと思います。
よろしくお願い致します🙏

Judeeeee

@su-su-su-su
お疲れ様です！お待たせいたしました💦
ブラウザでの挙動はOKです〜
コメントの

念のため、以下の２つも確認しました。

についても問題ありませんでした🙆‍♀️

一点だけお願いです🙏
su-su-su-suさんが確認したことを客観的に示すためにも、テストを追加いただけると助かります🙇
私も過去にほぼ同じようなissueを担当しており、その時のPRが参考になるかもです〜！
https://github.com/fjordllc/bootcamp/pull/7953/files

お手数ですがよろしくお願いいたします🙏

su-su-su-su · 2024-09-23T15:53:04Z

@Judeeeee
お疲れ様です！
レビューありがとうございます！

su-su-su-suさんが確認したことを客観的に示すためにも、テストを追加いただけると助かります🙇
私も過去にほぼ同じようなissueを担当しており、その時のPRが参考になるかもです〜！

アドバイスありがとうございます🙇‍♂️ Judeeeeeさんがされていた部分的に追加できるテストがなかったので、新規でテストを追加しました。

手隙の際に、再度ご確認頂けますでしょうか？
どうぞよろしくお願い致します🙏

Judeeeee · 2024-09-24T11:19:37Z

@su-su-su-su
修正ありがとうございます🙏
承知いたしました！
レビュー終わり次第また追って連絡させていただきますので、引き続きよろしくお願いいたします〜！

Judeeeee

@su-su-su-su

テスト追加対応ありがとうございました🙏

私の環境だと、テストを実行した際に落ちてしまうのですがsu-su-su-suさんの環境では問題なくパスしているでしょうか？🤔
何度実行しても以下のエラーが出てしまっている状況です😢

Error:
CommentsTest#test_show_confirm_dialog_if_report_is_not_confirmed:
Capybara::ModalNotFound: Unable to find modal dialog with 日報を確認済みにしていませんがよろしいですか？
    test/system/comments_test.rb:357:in `block in <class:CommentsTest>'

お手数ですが、ご確認いただけると幸いです🙇

Judeeeee · 2024-09-26T12:30:52Z

test/system/comments_test.rb

+
+  test 'show confirm dialog if report is not confirmed' do
+    visit_with_auth "/reports/#{reports(:report2).id}", 'machida'
+    assert_text '確認OKにする'
+    within('.thread-comment-form__form') do
+      fill_in('new_comment[description]', with: 'comment test')
+    end
+    accept_confirm '日報を確認済みにしていませんがよろしいですか？' do
+      click_button 'コメントする'
+    end
+    assert_text 'comment test'
+  end


テスト追加ありがとうございます🙏

未チェックの日報をクリック

コメントを書いてコメントするをクリック

「日報を確認済みにしていませんがよろしいですか？」とアラートが出ることを確認

を確認するテストだと思うので、ちょっと意図とずれている印象です😓

この記述だと、

フォームにコメントを入力する

アラートが出る

「コメントする」ボタンを押す

ように見えます🤔
お手数ですが、もう一度確認いただけると幸いです🙇

@Judeeeee

ご指摘ありがとうございます。
ご指摘の通りで

フォームにコメントを入力する

アラートが出る

「コメントする」ボタンを押す

となっておりました。
確認不足でした。お手間をおかけしました🙇‍♂️

未チェックの日報をクリック

コメントを書いてコメントするをクリック

「日報を確認済みにしていませんがよろしいですか？」とアラートが出ることを確認

となるように

test 'show confirm dialog if report is not confirmed' do visit_with_auth "/reports/#{reports(:report2).id}", 'machida' assert_text '確認OKにする' within('.thread-comment-form__form') do fill_in('new_comment[description]', with: 'comment test') click_button 'コメントする' end accept_confirm '日報を確認済みにしていませんがよろしいですか？' do end assert_text 'comment test' end

と修正しました。

再度ご確認いただけますと幸いです。
よろしくお願いいたします🙇‍♂️

@su-su-su-su

ご対応いただきありがとうございます🙏
こちらですが、私のaccept_confirmの認識が間違っていたため、su-su-su-suさんの以前の変更が正しいです💦

公式ドキュメントの説明では、

Execute the block, accepting a confirm.
Expects a block whose actions will trigger the display modal to appear.

とあります。
https://www.rubydoc.info/gems/capybara/Capybara%2FSession:accept_confirm

ややこしいのですが、accept_confirmのブロック内に書く処理は「モーダルを出現させる処理」なので、以前の変更のままで十分意図通りだと思います😓

折角ご対応いただいたのに混乱させてしまって申し訳ないです🙇🙇

私の方からはApproveさせていただきますので、修正次第komagataさんにレビュー依頼をお願いいたします🙇

@Judeeeee
ご確認ありがとうございます！
そうなのですね。
私こそ自分の書いたコードに自信がなく、ご指摘を頂いたまま修正してしまい申し訳ございません🙇‍♂️
こちら元のコードに修正いたしました。

su-su-su-su · 2024-09-27T15:26:15Z

@Judeeeee

お疲れ様です。ご確認ありがとうございます🙇‍♂️

私の環境だと、テストを実行した際に落ちてしまうのですがsu-su-su-suさんの環境では問題なくパスしているでしょうか？🤔
何度実行しても以下のエラーが出てしまっている状況です😢

私の方で再度テストをしました。

test/system/comments_test.rb をテストしました。

bin/rails test test/system/comments_test.rb                                                    
Running via Spring preloader in process 15261
Run options: --seed 8377

# Running:

................................
[Minitest::CI] Generating test report in JUnit XML format...


Finished in 105.349211s, 0.3038 runs/s, 0.7024 assertions/s.
32 runs, 74 assertions, 0 failures, 0 errors, 0 skips

test/system/comments_test.rb のhow_confirm_dialog_if_report_is_not_confirmedのみテストをしました。

bin/rails test test/system/comments_test.rb -n /show_confirm_dialog_if_report_is_not_confirmed/

Running via Spring preloader in process 25194
Run options: -n /show_confirm_dialog_if_report_is_not_confirmed/ --seed 45360

# Running:

.
[Minitest::CI] Generating test report in JUnit XML format...


Finished in 21.066426s, 0.0475 runs/s, 0.0949 assertions/s.
1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

となっていて私の方ではテストは通っていたのですが、テストの方法が違っていたりしているのでしょうか？

Judeeeee · 2024-09-30T11:38:43Z

@su-su-su-su
お待たせしました💦
ご確認ありがとうございます🙏

結論、私の方でもテストが通ることを確認しました🙇
私のローカル環境の問題でして、bin/setupを実行し再度テストしたところ問題が解消されました。
お手数をおかけしました🙇

su-su-su-su · 2024-10-01T06:08:39Z

@Judeeeee

私のローカル環境の問題でして、bin/setupを実行し再度テストしたところ問題が解消されました。

ご確認ありがとうございます🙏
自分もそうなったときbin/setupをしてみます。
参考になりました！ありがとうございます🙇‍♂️

su-su-su-su · 2024-10-01T06:09:32Z

@komagata
お疲れ様です。
approveいただきましたのでレビューをお願いできますでしょうか。
よろしくお願いいたします。

komagata · 2024-10-06T18:38:50Z

app/javascript/comments.vue

@@ -294,6 +294,17 @@ export default {
      }
    },
    postComment() {
+      if (this.commentableType === 'Report' && this.isRole('mentor')) {
+        const noConfirmed = !this.checkId


コード全体で確認（confirm）ではなくチェック（check）という用語を使っているのでそれに合わせた方がいいかなと思います。

そうなると!this.checkIdでもかなり意味がそのまま通るのでそのままでも良さそうですし、一旦変数に入れることで意味を明確にするのであれば、

const notChecked = !this.checkId

とかの方がいいかもです。

ご指摘ありがとうございます。
ご提示いただいたconst notChecked = !this.checkIdに修正しました。

komagata · 2024-10-06T18:41:45Z

app/javascript/comments.vue

+          const confirmResult = window.confirm(
+            '日報を確認済みにしていませんがよろしいですか？'
+          )
+          if (!confirmResult) {
+            return
+          }


Suggested change

const confirmResult = window.confirm(

'日報を確認済みにしていませんがよろしいですか？'

)

if (!confirmResult) {

return

}

if (window.confirm('日報を確認済みにしていませんがよろしいですか？')) {

return

}

でいいかも？

ご指摘ありがとうございます。
ご提示いただいたコードを動作確認したところ「日報を確認済みにしていませんがよろしいですか？」とアラートが表示され「OK」を押すとtrueが返されreturnが実行されてしまい、期待した動作になりませんでした。

そのため!window.confirm('日報を確認済みにしていませんがよろしいですか？') として、false が返るように修正しました。

修正後のコードは以下です。

if (this.commentableType === 'Report' && this.isRole('mentor')) { const notChecked = !this.checkId if (notChecked) { if ( !window.confirm('日報を確認済みにしていませんがよろしいですか？') ) { return } } }

su-su-su-su · 2024-10-16T11:15:19Z

@komagata

お疲れ様です。
ご指摘頂いた点を修正いたしましたのでお手隙の際にご確認お願いします。

komagata · 2024-10-30T05:56:48Z

app/javascript/comments.vue

+        if (notChecked) {
+          if (
+            !window.confirm('日報を確認済みにしていませんがよろしいですか？')
+          ) {
+            return
+          }
+        }


不必要なブロックの入れ子は避けるべきなので下記はいかがでしょうか？？

Suggested change

if (notChecked) {

if (

!window.confirm('日報を確認済みにしていませんがよろしいですか？')

) {

return

}

}

if (notChecked && !window.confirm('日報を確認済みにしていませんがよろしいですか？')) {

return

}

@komagata
コメントありがとうございます。
ご提案頂いたコードに修正しました。
不必要なブロックの入れ子にならないように自分でも気づけるように注意したいと思います。
ありがとうございます！

su-su-su-su · 2024-10-31T07:51:11Z

@komagata
お疲れ様です。
ご指摘頂いた点を修正いたしましたのでお手隙の際に再度ご確認お願いします。

komagata

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

su-su-su-su self-assigned this Sep 10, 2024

su-su-su-su requested a review from Judeeeee September 10, 2024 14:22

Judeeeee reviewed Sep 19, 2024

View reviewed changes

su-su-su-su force-pushed the feature/alert-for-unconfirmed-daily-report-comment branch from 77bda5e to b412eac Compare September 23, 2024 15:11

Judeeeee reviewed Sep 26, 2024

View reviewed changes

Judeeeee self-requested a review September 30, 2024 12:02

Judeeeee approved these changes Sep 30, 2024

View reviewed changes

su-su-su-su force-pushed the feature/alert-for-unconfirmed-daily-report-comment branch from b288973 to 7bcd02a Compare October 1, 2024 05:23

komagata reviewed Oct 6, 2024

View reviewed changes

su-su-su-su force-pushed the feature/alert-for-unconfirmed-daily-report-comment branch from 7bcd02a to e87fd1c Compare October 16, 2024 10:57

komagata reviewed Oct 30, 2024

View reviewed changes

su-su-su-su added 7 commits October 31, 2024 00:23

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装

7eff41c

日報が未確認の場合、ダイアログを表示するか確認するテストを追加

280110e

rubocopに対する修正

fabeb51

テストのアラートが出るタイミングをコメントした後になるように修正

0b37e61

テストを元のコードに修正

2075770

confirmをcheckに統一、冗長な処理を簡潔化

c9a5f15

不必要なブロックの入れ子を避ける為に修正

02d7e9d

su-su-su-su force-pushed the feature/alert-for-unconfirmed-daily-report-comment branch from e87fd1c to 02d7e9d Compare October 30, 2024 15:25

komagata reviewed Nov 7, 2024

View reviewed changes

komagata merged commit 73713d6 into main Nov 7, 2024
3 checks passed

komagata deleted the feature/alert-for-unconfirmed-daily-report-comment branch November 7, 2024 18:42

github-actions bot mentioned this pull request Nov 7, 2024

Release 2024-11-10 16:40:08 +0000 #8180

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装 #8061

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装 #8061

su-su-su-su commented Sep 10, 2024 •

edited

Loading

su-su-su-su commented Sep 10, 2024

Judeeeee commented Sep 11, 2024

su-su-su-su commented Sep 12, 2024

Judeeeee left a comment •

edited

Loading

su-su-su-su commented Sep 23, 2024

Judeeeee commented Sep 24, 2024

Judeeeee left a comment

Judeeeee Sep 26, 2024

su-su-su-su Sep 29, 2024

Judeeeee Sep 30, 2024 •

edited

Loading

su-su-su-su Oct 1, 2024

su-su-su-su commented Sep 27, 2024

Judeeeee commented Sep 30, 2024

su-su-su-su commented Oct 1, 2024

su-su-su-su commented Oct 1, 2024

komagata Oct 6, 2024

su-su-su-su Oct 16, 2024

komagata Oct 6, 2024

su-su-su-su Oct 16, 2024

su-su-su-su commented Oct 16, 2024

komagata Oct 30, 2024

su-su-su-su Oct 31, 2024

su-su-su-su commented Oct 31, 2024

komagata left a comment

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装 #8061

日報が確認OKされていない場合、メンターがコメントした時にアラートが出るように実装 #8061

Conversation

su-su-su-su commented Sep 10, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

su-su-su-su commented Sep 10, 2024

Judeeeee commented Sep 11, 2024

su-su-su-su commented Sep 12, 2024

Judeeeee left a comment • edited Loading

Choose a reason for hiding this comment

su-su-su-su commented Sep 23, 2024

Judeeeee commented Sep 24, 2024

Judeeeee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Judeeeee Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

su-su-su-su commented Sep 27, 2024

Judeeeee commented Sep 30, 2024

su-su-su-su commented Oct 1, 2024

su-su-su-su commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

su-su-su-su commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

su-su-su-su commented Oct 31, 2024

komagata left a comment

Choose a reason for hiding this comment

su-su-su-su commented Sep 10, 2024 •

edited

Loading

Judeeeee left a comment •

edited

Loading

Judeeeee Sep 30, 2024 •

edited

Loading