提出物未チェックの場合、学習ステータスを修了にできないように変更

[reckyy]

Issue

• プラクティスを終了していないのに、学習ステータスの変更で他の人の回答が見れてしまう。 #7959

概要

表題の通りです。
プラクティスを修了していなくても、他の方の回答が見れてしまうため修正しました。

変更確認方法

1. bugfix/show-answers-incomplete-practiceをローカルに取り込む
2. hajimeで http://localhost:3000/practices/315059988 にアクセス。
3. 学習ステータスを「修了」に変更しようとすると、変更できない旨のalertが出ることを確認。
4. 念のため、他の人の提出物（どれでも構いません）をクリックし、他の提出物が見れないことを確認。

Screenshot

変更前

提出物がない状態でも、修了に変更できてしまう。

変更後

[reckyy]

@mousu-a
お疲れ様です！
レビューをお願いしたくご連絡いたしました。急ぎではありません。
もしご都合厳しければ、教えていただけますと幸いです。
よろしくお願いいたします。

[mousu-a]

@reckyy
お疲れ様です！
レビュー依頼ありがとうございます😄
ぜひ受けさせていただきます〜

明後日くらいに返せればと思います〜🙏

[mousu-a]

@reckyy
お疲れ様です！
すみません、ちょっと遅れてしまいましたがレビューお待たせいたしました。
コード良い感じだと思います👍
気になるところがいくつかあり、その点コメントさせていただきましたのでご確認ください🙏

１つ目　似たようなバグ？

今回のプルリクは「提出物が確認済みになっていない場合、プラクティスのステータスを修了に変更できないようにする」ということですが、似たようなバグを発見しました。

提出物を提出していない状態でもプラクティスページ下部の「終了条件」にある修了ボタンが押せてしまい、このようなモーダルが出てきてしまいます。

質問

こちらはこのissue(プルリク)の対象外ということでよろしかったでしょうか？（ストーリーとしては似ているのかなと思ってお聞きしました）

---

2つ目　コミットの粒度に関して

こちらのコミットに関して、コミットの粒度が大きいように感じました🤔
Whatはわかりやすいですが、Whyがわかりづらかった感じです。

レビュアーはレビュイーとは違って(そのプルリクの)専門家ではないので、(そのプルリクの)初心者でもわかるような説明がされているとレビュアー側としてはとても助かるかもです👀（適宜コミットして変更を分割するなど）

---

すみません、コードは問題ないのですが一応質問のご返答を頂いてからApproveさせていただきますー！

[reckyy]

このステータスを変更している理由は、このテストの目的から変更先のステータスが「修了」でなくてもいいと判断したからです。
もし修了のままだと、バリデーションを追加した関係で提出物をチェックする行程を追加しないといけなくコストが高くなるからです。

[reckyy]

validation追加の関係で、テストがvalidationに引っかかるものがいくつかあったため対応策として作成しました。

[reckyy]

@mousu-a
レビューありがとうございます〜！
以下回答です。

1. 似たようなバグ

ありがとうございます！こちらは全く把握していませんでした。確かに同じことが起こっていますね。。
komagataさんにこのPR内で修正していいか確認します。

2. コミットの粒度

(そのプルリクの)初心者でもわかるような説明がされているとレビュアー側としてはとても助かるかもです👀

おっしゃる通り、コミットの内容は説明不足なものがあったので説明を追加しました。
#7994 (comment)
#7994 (comment)

ただしコミットの粒度に関して。今回このコミットでは

• バリデーション追加
• バリデーション追加に伴うテストの変更

を行なっていますが、これを分割するということですかね。自分としては関連しているものは一つにまとめるべきという考えですが、いかがでしょうか。👀
コミットした時点で、動くものをコミットする、という認識です。（もしバリデーションのみをコミットしてしまうと、テストが動かない）

@komagata
お疲れ様です。
@mousu-a さんから、同様のバグがあることを教えていただきました。
こちらもPR内で修正してしまって良いでしょうか。

[mousu-a]

@reckyy

説明コメント大変助かります！
そしてお早いお返事ありがとうございますー！

すみません、「動く状態でコミットする」という視点はすっかり忘れていました。
ただそれとして説明が欲しい、というのもやっぱり感じます。
なので今reckyyさんにご対応頂いたようにコメントなどでご説明いただけるとレビュアーとしても助かりますー😄

以下返答になります！

今回このコミットでは
バリデーション追加
バリデーション追加に伴うテストの変更
を行なっていますが、これを分割するということですかね。

そうです。自分としては

• バリデーションを追加
• バリデーションにより通らなくなったテストが通るように修正
• completeである必要がないため修正
  • completeのままだと、バリデーションを追加した関係で提出物をチェックする行程を追加しないといけなくなりコストが高くなってしまう
• validation追加に伴う修正
  • 提出物のCheckが済んでいないため、API::Practices::Learning::CompletionMessageController#updateが実行できていなかった。

（文はある程度簡略化しています、参考程度に🙏）このくらいやっても良いのかなと思いました。いかがでしょうか。

---

関連しているものは一つにまとめるべきという考え

こちらに関しては、すでにプルリクとして１つのテーマにまとめているので問題はなさそうに思いますがいかがでしょうか👀

---

コミットした時点で、動くものをコミットする、という認識です。

「プログラムが動かない」状態でコミットするのは微妙としても、「テストが通らない」状態でコミットするのは問題ないという認識です。
それこそ次のコミットで「こういう理由で通らなくなったテストを通るようにした」という風にしても良いのかなと考えています。

お返事お待ちしております〜！

[reckyy]

@mousu-a
ご返信ありがとうございます〜！
以下返答です。
ご返信お待ちしております！

このくらいやっても良いのかなと思いました。いかがでしょうか。

自分はまとめたいな〜と思った理由を後述しています。

こちらに関しては、すでにプルリクとして１つのテーマにまとめているので問題はなさそうに思いますがいかがでしょうか👀

今回は修正箇所が軽微でしたが、もし修正箇所が膨大になった場合、読み込むファイルが多くなります。その場合、コミット単位でレビューを行ったり、変更の流れを追ったりする方もいると思います。（自分はそのタイプです）
そうした時に、コミットが関連性のある変更でまとまっている方が、全体の流れを掴みやすくなると感じています。

「テストが通らない」状態でコミットするのは問題ないという認識です。

この点については、少し強めに言わせていただくと、自分は問題があるという認識です👀
「原因はバリデーションを追加したことだから、どうテストを修正すれば良いのかはわかってる！」という状態であっても、テストが通らないままコミットすることで、潜在的なバグを抱える可能性があると考えています。
テストが通らないということは、その時点でコードが安全かどうか確信できないため、コミットするべきではないというのが自分の考えです。（とはいえ、時にはテストの修正をコミットし忘れることもありますが🥲 基本的には、テストと同時にコミットするべきだと思っています）

[mousu-a]

@reckyy

お早いお返事ありがとうございます！

今回は修正箇所が軽微でしたが、もし修正箇所が膨大になった場合、読み込むファイルが多くなります。その場合、コミット単位でレビューを行ったり、変更の流れを追ったりする方もいると思います。（自分はそのタイプです）
そうした時に、コミットが関連性のある変更でまとまっている方が、全体の流れを掴みやすくなると感じています。

なるほどです。この視点はありませんでした👀

テストが通らないままコミットすることで、潜在的なバグを抱える可能性があると考えています。
テストが通らないということは、その時点でコードが安全かどうか確信できないため、コミットするべきではないというのが自分の考えです。（とはいえ、時にはテストの修正をコミットし忘れることもありますが🥲 基本的には、テストと同時にコミットするべきだと思っています）

なるほどです。

ありがとうございます！
納得できましたのでこちらからはApproveとさせていただきます〜！🙆

[reckyy]

@mousu-a
返信が遅くなり申し訳ございません。
ご返信ありがとうございます！

Approveいただきありがとうございます〜。もし追加でバグの修正が生じた場合、再度レビュー依頼させていただきます。
引き続きよろしくお願いいたします。

[komagata]

@reckyy (CC: @mousu-a )

@mousu-a さんから、https://github.com/fjordllc/bootcamp/pull/7994#pullrequestreview-2237034557があることを教えていただきました。
こちらもPR内で修正してしまって良いでしょうか。

すみません、ちょっと代名詞や主語や目的語を省略されている文章が多くて、具体的になんのことをおっしゃっているのかが理解しづらくなっております。

もう少しご説明いただければありがたいです。

[reckyy]

@komagata
申し訳ないです。コンテキストを省略してしまっていました。

今回のPRは「提出物が確認されていないにも関わらず、学習ステータスを手動で修了にできてしまう」バグの修正です。

この修正により、修了していないプラクティスの他の方の回答が見れないようにすることを目的としていました。

しかし @mousu-a さんより、提出物を作成していない状態でも、以下の画像にある「修了」をクリックすることで、学習ステータスが修了となり、他の方の提出物が見れてしまうことが判明しました。

プラクティスを終了していないのに、学習ステータスの変更で他の人の回答が見れてしまう。 · Issue #7959 は一つ目のバグの修正ではなく、「他の方の提出物を不正に見ることを防ぐ」ことが目的です。そのため、本PR内で修正しても良いかどうか、念のため確認させていただきました。

もし分かりにくいところがあれば、ご指摘いただけますと幸いです。
よろしくお願いいたします。

[komagata]

@reckyy なるほどです。ご説明ありがとうございます。

プラクティスを終了していないのに、学習ステータスの変更で他の人の回答が見れてしまう。

上記の解釈によるとは思いますが、今回は「修了ボタンで修了にできてしまう」の方の修正もお願いできればありがたいです。

[reckyy]

@komagata (cc: @mousu-a )
お疲れ様です。
ご相談させていただきたいことがあり、メンションさせていただきました。お手隙の際にご確認よろしくお願いいたします。

前回のバグの調査の結果

まず、こちらのバグですが、認識に誤りがありました。

しかし @mousu-a さんより、提出物を作成していない状態でも、以下の画像にある「修了」をクリックすることで、学習ステータスが修了となり、他の方の提出物が見れてしまうことが判明しました。

確かに「修了」をクリックすると学習ステータスは「修了」と表示されます。しかし、このPRで追加したバリデーションが正しく機能しており、他の人の提出物は見れないようになっていました。つまり、見た目上は「修了」となっていますが、内部ではステータスの更新が失敗しています。実際、「修了」をクリックしても他の人の提出物は閲覧できませんでした。

なぜ見た目が変更できてしまうのか

learning.vueのpushCompleteが、以下の修了ボタンのクリックイベントに設定されています。

pushComplete()のコードはこちらです

bootcamp/app/javascript/learning.vue

Lines 65 to 87 in 049eff4

	pushComplete() {
	const params = new FormData()
	params.append('status', 'complete')
	fetch(`/api/practices/${this.practiceId}/learning.json`, {
	method: 'PATCH',
	headers: {
	'X-Requested-With': 'XMLHttpRequest',
	'X-CSRF-Token': CSRF.getToken()
	},
	credentials: 'same-origin',
	redirect: 'manual',
	body: params
	})
	.then(() => {
	this.complete = true
	})
	.catch((error) => {
	console.warn(error)
	})
	}
	}
	}

レスポンスを返す、updateアクションが以下です。

bootcamp/app/controllers/api/practices/learning_controller.rb

Lines 17 to 38 in 049eff4

	def update
	learning = Learning.find_or_initialize_by(
	user_id: current_user.id,
	practice_id: params[:practice_id]
	)
	learning.status = if params[:status].nil?
	:complete
	else
	params[:status].to_sym
	end
	status = learning.new_record? ? :created : :ok
	if learning.save
	Newspaper.publish(:learning_create, { user: learning.user })
	notify_to_chat_for_employment_counseling(learning) if status == :created && learning.practice.title == '就職相談部屋を作る'
	head status
	else
	render json: learning.errors, status: :unprocessable_entity
	end
	end

responseが返ってきた時点でステータスをcompleteにしているため、アクションの成功失敗に関係なく文言が変更されてしまっていました。

相談したいこと

この状況を改善するために、レスポンスに基づいてステータスをcompleteにするかどうかを分岐するコードを作成しました。

.then((response) => {
  if(response.ok){
    this.complete = true
    
  } else {
    response.json().then((data) => {
      alert(data.error)
    })
  }
})

ただし、以下の問題が残っています。

• 修了ボタンのlabelに付与されているfor属性が影響して、クリックすると操作が失敗していても「プラクティス〜を修了しました」のモーダルが表示されてしまいます。

bootcamp/app/javascript/learning.vue

Lines 12 to 17 in 049eff4

	li.card-main-actions__item(v-else)
	label#js-complete.a-button.is-sm.is-warning.is-block(
	@click='pushComplete',
	for='modal-learning_completion')
	i.fa-solid.fa-check
	| 修了

2024-08-26.16.39.07.mov

このモーダルをpushComplete()が成功した場合にのみ表示させる方法が思いつきません。
アドバイスをいただけると幸いです。

[mousu-a]

@reckyy
調査ありがとうございます！！

モーダルの表示をpushComplete()の結果に紐づけたいわけですよね。
completeがtrueだったら（pushComplete()が成功したら）forに値('modal-learning_completion')をセットしたい、という認識です。

GPTと一緒に考えてみました。
まだ解決出来ていませんが途中経過を貼ります。

以下のようにやればどうかと考えました。

1. data()の中で変数を初期化
2. pushComplete()が成功したらその変数に値('modal-learning_completion')をセット
3. for属性にその変数を展開する。

コードです。必要な部分のみ抜粋しています。
bootcamp/app/javascript/learning.vue

    li.card-main-actions__item(v-else)
      label#js-complete.a-button.is-sm.is-warning.is-block(
        @click='pushComplete',
        // forの値に変数をセット
        :for='modalFor')
        i.fa-solid.fa-check
        | 修了

~~~

  data() {
    return {
    // ... 他のプロパティ ...
    // forにセットする変数を初期化
      modalFor: null,
    }

~~~

 pushComplete() {
    // ... fetch処理 ...
        .then((response) => {
          // アクションが成功したら値をセット
          if (response.ok) {
            this.complete = true
            this.modalFor = 'modal-learning_completion'
          } else {
            response.json().then((data) => {
              alert(data.error)
            })
          }
        })
        .catch((error) => {
          console.warn(error)
        })
    }

ですがこれだとモーダルが表示されなくなってしまうんですよね〜...なんでだろう。
その辺りもうちょっと深掘りしてみます。

[reckyy]

@mousu-a
お疲れ様です。
こちらこそ、代替案をいただきありがとうございます！！

その方法だと、確かにpushComplete()が正常に終了した後for属性に値がセットされます。
ただその後表示されるlabelは、v-if='complete'で分岐される「修了しています」の文言です。
なので、for属性がセットされたlabelはそもそも表示されません。
そのため、モーダルが表示されることはあり得なくなります。
（自分もこの方法試したので🥲）

[komagata]

@reckyy

この状況を改善するために、レスポンスに基づいてステータスをcompleteにするかどうかを分岐するコードを作成しました。

修了ボタンのlabelに付与されているfor属性が影響して、クリックすると操作が失敗していても「プラクティス〜を修了しました」のモーダルが表示されてしまいます。

局所的に修正するだけで直すことが前提となっていますが、広範囲に修正する（learning.vue自体を全くの別物に変えてしまっても問題ありません）ことも考慮に入れた上であれば解決できるかもしれません。

本当にギブアップという感じであれば再度おっしゃっていただければと思います。

[reckyy]

@mousu-a
お疲れ様です。
修正完了しましたので、再度レビューをお願いできればと思います。
よろしくお願いいたします。

@komagata (cc: @mousu-a )
お疲れ様です。
ご返信ありがとうございます！

無事にモーダルを正しく表示できたため、ご報告させていただきます。
また、最下部で一点質問させていただいていますので、お手隙の際にご回答いただけますと幸いです。

とった修正方法

今回表示を正しく切り替えたかったモーダルが以下になります。

モーダルのコードはこちらになります。

bootcamp/app/views/shared/_modal_learning_completion.html.slim

Lines 1 to 19 in 441af9a

	- id = 'modal-learning_completion'
	= render '/shared/modal', id: id
	#modal-learning_completion-data(data-practice-id="#{practice.id}" data-should-display-message-automatically="#{should_display_message_automatically}")
	.congrats-card-body
	h2.congrats-card-body__title
	| プラクティス「#{practice.title}」を修了しました🎉
	.congrats-card-body__image-container
	- if practice.completion_image.attached?
	= image_tag @practice.completion_image, class: 'congrats-card-body__image', alt: "#{practice.title}修了おめでとうカード"
	- else
	= image_tag('completion/completion_default.png', class: 'congrats-card-body__image', alt: "#{practice.title}修了おめでとうカード")
	.card-footer
	.card-main-actions
	ul.card-main-actions__items
	li.card-main-actions__item
	= link_to '喜びをXにポストする！', tweet_url, class: 'a-button is-sm is-primary is-block', target: '_blank', rel: 'noopener'
	li.card-main-actions__item.is-sub
	label.card-main-actions__muted-action.is-closer(for="#{id}")
	| 閉じる

さらに、_modal.html.slimの部分は以下です。

bootcamp/app/views/shared/_modal.html.slim

Lines 1 to 14 in 441af9a

	- if defined?(auto_show) && auto_show
	input.a-toggle-checkbox(id="#{id}" type="checkbox" checked)
	- else
	input.a-toggle-checkbox(id="#{id}" type="checkbox")
	.modal
	label.modal__overlay(for="#{id}")
	.modal-content
	- if defined?(modal_title)
	.modal-header
	h2.modal-title
	= modal_title
	label.modal-header__close(for="#{id}")
	.modal-body
	= yield

モーダルの表示切り替えはこの上4行のコードで行われていました。learning.vueのpushComplete()が成功した場合に、inputタグのchecked属性をtrueに設定する処理を追加しました。これにより、学習ステータスが正しく更新された際にのみモーダルが表示されるようにしました。

質問

局所的に修正するだけで直すことが前提となっていますが、広範囲に修正する（learning.vue自体を全くの別物に変えてしまっても問題ありません）ことも考慮に入れた上であれば解決できるかもしれません。

基本的に影響範囲は少ない方が良いという考えから、局所的な修正を優先していたかもしれません。
広範囲に修正しても良いと判断する基準はあるのでしょうか？
また、もし今後何らかのIssueで修正範囲が大きくなる場合は、リーダー（komagataさん）に確認した上で広範囲の修正を検討するべきでしょうか？

[mousu-a]

コード良いと思います！予定にないissueの対応までありがとうございました🙇‍♂️
こちらからはApproveとさせていただきます〜！

[reckyy]

@komagata
お疲れ様です。
チームメンバーのレビューが終わりましたので、お手隙の際に以下の2点ご対応いただけますと幸いです。

• レビュー
• 提出物未チェックの場合、学習ステータスを修了にできないように変更 #7994 (comment) の確認

よろしくお願いいたします。

[komagata]

@reckyy

広範囲に修正しても良いと判断する基準はあるのでしょうか？

基本的に「自分の関係ある部分・そうでない部分」と考えるのでなく、サイト・コード全体として自然な形（他のプログラマーが見ても迷わない）になるように修正する必要があります。

今回の場合、learning.vueが想定している仕様自体が現状と合わないのであればlearning.vueを丸ごと変えてしまっても問題ないですし、無理なく変更できるのであれば局所的な修正でも構いません。

ギブアップするほど無理があるのであればleaning.vue自体を全部変えてしまっても問題ないという意味になります。

[komagata]

2回使うのでなければproductという変数を作る意味はないかもです。（1行で良いと思います。）

[komagata]

@reckyy こちらいかがでしょうか？

[reckyy]

@komagata
おっしゃる通りです。
変数削除する修正をしたあと、返信させていただこうと思ってました。返信が遅れて申し訳ないです。 🙇

加えて長期間作業できておらず、ご迷惑をおかけしている点も重ねて申し訳ないです。
こちらのPRは一週間以内に作業再開するようにします！

[komagata]

他の箇所でもincludeとテストやsetupのメソッドは1行開けているので開けた方がいいかなと思います。

[reckyy]

@komagata
お疲れ様です。
レビューありがとうございます！返信が遅くなり申し訳ございません。

一点ご相談させていただきたいです。

相談

• learning.vueを非Vue化した #8131

上記PRで、今回修正を行なったapp/javascript/learning.vueを非Vue化しています。
なので、自分が今回Vueで追加実装した部分はマージできないため、上記PRのmerge後、非Vue化されたファイルに追加実装を行いたいと考えているのですが、いかがでしょうか。

[komagata]

@reckyy

上記PRのmerge後、非Vue化されたファイルに追加実装を行いたいと考えているのですが、いかがでしょうか。

そちらのPRは今見たらすでにマージされているようですので、作業をお願いします。

[reckyy]

@komagata
ご返信ありがとうございます！

そちらのPRは今見たらすでにマージされているようですので、作業をお願いします。

承知しました！

[komagata]

@reckyy conflictの修正をお願いします。

[reckyy]

@komagata
修正しました！