複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

djkazunoko · 2023-07-02T22:40:01Z

Issue

Ruby on Railsのプラクティスのみ、日報が重複して表示される #6477

概要

バグの詳細

#6477 (comment)

変更点

日報のプラクティス選択欄の表示形式を[カテゴリー名]プラクティス名からプラクティス名に変更した
- これにより日報のプラクティス選択欄に同一プラクティスが重複して表示されなくなった
  - [Mac OS X]Terminalの基礎を覚える、[UNIX]Terminalの基礎を覚える→Terminalの基礎を覚える
- 質問のプラクティス選択欄の表示形式も上記に統一した
本番環境のpractices_reportsテーブルに存在する重複レコードを削除するdata-migrateファイルを作成した
practices_reportsテーブルの[practice_id, report_id]にユニーク制約を追加した
- これによりプラクティスの日報一覧(/practices/{ID}/reports)での日報の重複表示がなくなった

変更確認方法

1. バグの再現

以下の手順をmainブランチ上で行う

db/fixtures/categories_practices.ymlに以下を追加

+ categories_practice61:
+   practice: practice2
+   category: category2
+   position: 7

rails db:resetを実行(dbの初期化とseedデータの読み込み)
rails sでサーバーを起動
localhost:3000にアクセスし、kimuraでログイン
日報テスト日報5ですの編集画面(/reports/730242988/edit)を開く
日報のプラクティス選択欄の表示形式が[カテゴリー名]プラクティス名になっていることを確認する
日報のプラクティス選択欄で[Mac OS X]Terminalの基礎を覚えると[UNIX]Terminalの基礎を覚えるを選択し、内容変更をクリックして保存する
プラクティスTerminalの基礎を覚えるの日報一覧(/practices/198065840/reports)にアクセスし、編集した日報が重複表示されていることを確認する

※この時点でdb/fixtures/categories_practices.ymlの変更は破棄してok

2. 変更確認

日報

bug/fix-duplicate-practice-reportsをローカルに取り込む
rails dbconsoleでdbに接続し、SELECT * FROM practices_reports;を実行して重複レコードを確認する
rails data:migrate:up VERSION=20230701031415を実行(data-migrate)
rails dbconsoleでdbに接続し、SELECT * FROM practices_reports;を実行して重複レコードが削除されていることを確認する
rails db:migrateを実行
rails sでサーバーを起動
localhost:3000にアクセスし、kimuraでログイン
日報テスト日報5ですの編集画面(/reports/730242988/edit)を開く
日報のプラクティス選択欄の表示形式がプラクティス名になっており、選択肢の重複がないことを確認する
プラクティスTerminalの基礎を覚えるの日報一覧(/practices/198065840/reports)にアクセスし、日報の重複表示がないことを確認する

※手順3のdata-migrate実行時にdb/data_schema.rbに生じる差分は無視していい(rubocop実行後に元に戻るため)

質問

質問作成画面(/questions/new)を開き、プラクティス選択欄の表示形式がプラクティス名になっており、選択肢の重複がないことを確認する
質問の表示画面(/questions/{ID})で内容修正を選択して編集ブロックを表示させ、プラクティス選択欄の表示形式がプラクティス名になっており、選択肢の重複がないことを確認する

Screenshot

開発環境のデータを表示しています。

変更前

/reports/730242988/edit

/practices/198065840/reports

/questions/new

/questions/{ID}(編集時)

変更後

/reports/730242988/edit

/practices/198065840/reports

/questions/new

/questions/{ID}(編集時)

参考

djkazunoko · 2023-07-07T22:43:01Z

db/data/20230701031415_delete_duplicate_rows_in_practices_reports.rb

+      DELETE FROM practices_reports
+      WHERE ctid NOT IN (
+        SELECT min(ctid)
+        FROM practices_reports
+        GROUP BY practice_id, report_id
+      );


[practice_id, report_id]の重複レコードを削除して1行だけ残すSQL
ctidの値が最小のレコードを残している
practices_reportsテーブルにはidカラムがないため、PostgreSQLのシステム列であるctidで代用
PostgreSQL: Documentation: 15: 5.5. System Columns

djkazunoko · 2023-07-07T22:54:29Z

db/migrate/20230702035134_add_unique_constraint_to_practices_reports.rb

+class AddUniqueConstraintToPracticesReports < ActiveRecord::Migration[6.1]
+  def change
+    remove_index :practices_reports, [:practice_id, :report_id]
+    add_index :practices_reports, [:practice_id, :report_id], unique: true
+  end
+end


既存のindexにunique: trueだけ追加したい場合、一旦削除して作り直す必要がある(remove_index→add_index)

djkazunoko · 2023-07-11T03:45:56Z

@wata00913
お疲れ様です！
こちらお手隙の際に、レビューをお願いできますでしょうか🙌

wata00913 · 2023-07-11T08:28:47Z

@djkazunoko
お疲れ様です！
レビュー依頼ありがとうございます。１週間ほど時間を要しますが、大丈夫でしょうか？

djkazunoko · 2023-07-11T11:11:51Z

@wata00913
いつでも大丈夫です〜
よろしくお願いします🙇‍♂️

wata00913 · 2023-07-18T00:08:20Z

app/views/api/practices/index.json.jbuilder

+user_practices = @categories.flat_map do |category|
+  category.practices
+end
+
+unique_practices = user_practices.uniq


5行目のendにuniqをつけた方がシンプルに書けると思いました〜

Suggested change

user_practices = @categories.flat_map do |category|

category.practices

end

unique_practices = user_practices.uniq

unique_practices = @categories.flat_map do |category|

category.practices

end.uniq

ご確認いただきありがとうございます！
(&:method)を使ってさらにシンプルにしてみました〜
app/helpers/reports_helper.rbも修正しています。
a1298c1

wata00913

@djkazunoko
レビューお待たせしました🙏

動作確認はOKでした〜
動作手順も分かりやすく、kazunokoさんのセルフレビューを見てDBについて参考になりました！

１点コードについてコメントしましたので、確認をよろしくお願いします🙇‍♂️

wata00913

@djkazunoko
確認しました、OKです！
Approveさせて頂きます！👍

djkazunoko · 2023-07-24T23:40:56Z

@wata00913
レビューしていただきありがとうございました！

djkazunoko · 2023-07-24T23:42:02Z

@komagata
メンバーレビュー完了しましたので、レビューよろしくお願いします。

komagata · 2023-08-01T04:10:56Z

app/views/api/practices/index.json.jbuilder

-    json.category category.name
-  end
+
+unique_practices = @categories.flat_map(&:practices).uniq


viewはなるべく処理をせず見た目に関する部分だけすべきなのでこちらはControllerでやった方が良さそうです。

@komagata
修正しました c9cf0f2

komagata · 2023-08-01T04:13:36Z

app/helpers/reports_helper.rb

@@ -11,11 +11,7 @@ def practice_options(categories)

  def practice_options_within_course


このメソッドのテストが欲しいかなと思います。

@komagata
修正しました 80fd1e1
プラクティスの並び順、複数カテゴリーに所属する同一プラクティスが存在する場合に重複表示されないことを確認しています。

komagata · 2023-08-22T23:43:37Z

@djkazunoko こちらいかがでしょうか

djkazunoko · 2023-08-22T23:56:42Z

@komagata
返信遅れてすいません、現在対応中です。

表示形式を [カテゴリー名]プラクティス名からプラクティス名に変更質問作成画面にも反映されてる

日報のプラクティス選択の表示形式と統一した

djkazunoko · 2023-09-03T22:24:44Z

@komagata
修正しましたので、ご確認お願いします。

komagata

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

djkazunoko self-assigned this Jul 2, 2023

djkazunoko force-pushed the bug/fix-duplicate-practice-reports branch from a2b2b40 to 521200b Compare July 7, 2023 02:59

djkazunoko commented Jul 7, 2023

View reviewed changes

djkazunoko force-pushed the bug/fix-duplicate-practice-reports branch from 521200b to 16915d4 Compare July 9, 2023 01:50

djkazunoko marked this pull request as ready for review July 11, 2023 03:44

djkazunoko requested a review from wata00913 July 11, 2023 03:45

wata00913 reviewed Jul 18, 2023

View reviewed changes

djkazunoko force-pushed the bug/fix-duplicate-practice-reports branch 3 times, most recently from 63175da to a1298c1 Compare July 21, 2023 01:59

wata00913 approved these changes Jul 24, 2023

View reviewed changes

djkazunoko requested a review from komagata July 24, 2023 23:41

komagata reviewed Aug 1, 2023

View reviewed changes

djkazunoko added 7 commits August 26, 2023 12:07

日報のプラクティス選択の表示形式を変更して複数カテゴリーに所属するプラクティスの重複表示を無くした

b106b06

表示形式を [カテゴリー名]プラクティス名からプラクティス名に変更質問作成画面にも反映されてる

practices_reportsテーブルの重複行を削除するdata-migrateファイルを作成

5f4c3c9

practices_reportsテーブルの[practice_id, report_id]にユニーク制約を追加

e0e74ff

テストを修正

4b4792d

質問編集時のプラクティス選択の表示形式を変更して複数カテゴリーに所属するプラクティスの重複表示を無くした

0eb7a55

日報のプラクティス選択の表示形式と統一した

メソッドをリファクタリング

a6b892d

viewに書いていた処理をcotrollerに移動

c9cf0f2

djkazunoko force-pushed the bug/fix-duplicate-practice-reports branch from a1298c1 to c9cf0f2 Compare August 26, 2023 03:13

practice_options_within_courseメソッドのテストを作成

80fd1e1

djkazunoko force-pushed the bug/fix-duplicate-practice-reports branch from ba4bbd1 to 80fd1e1 Compare August 27, 2023 05:23

komagata approved these changes Sep 4, 2023

View reviewed changes

komagata merged commit 7f65173 into main Sep 4, 2023

komagata deleted the bug/fix-duplicate-practice-reports branch September 4, 2023 08:32

github-actions bot mentioned this pull request Sep 4, 2023

Release 2023-09-07 22:53:16 #6866

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

djkazunoko commented Jul 2, 2023 •

edited

Loading

djkazunoko Jul 7, 2023

djkazunoko Jul 7, 2023

djkazunoko commented Jul 11, 2023

wata00913 commented Jul 11, 2023

djkazunoko commented Jul 11, 2023

wata00913 Jul 18, 2023

djkazunoko Jul 21, 2023

wata00913 left a comment

wata00913 left a comment

djkazunoko commented Jul 24, 2023

djkazunoko commented Jul 24, 2023

komagata Aug 1, 2023

djkazunoko Aug 27, 2023

komagata Aug 1, 2023

djkazunoko Aug 27, 2023

komagata commented Aug 22, 2023

djkazunoko commented Aug 22, 2023

djkazunoko commented Sep 3, 2023

komagata left a comment

		@@ -11,11 +11,7 @@ def practice_options(categories)

		def practice_options_within_course

複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

複数カテゴリーに所属するプラクティスを紐づけた日報の重複表示を修正 #6698

Conversation

djkazunoko commented Jul 2, 2023 • edited Loading

Issue

概要

バグの詳細

変更点

変更確認方法

1. バグの再現

2. 変更確認

Screenshot

変更前

変更後

参考

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djkazunoko commented Jul 11, 2023

wata00913 commented Jul 11, 2023

djkazunoko commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wata00913 left a comment

Choose a reason for hiding this comment

wata00913 left a comment

Choose a reason for hiding this comment

djkazunoko commented Jul 24, 2023

djkazunoko commented Jul 24, 2023

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

komagata commented Aug 22, 2023

djkazunoko commented Aug 22, 2023

djkazunoko commented Sep 3, 2023

komagata left a comment

Choose a reason for hiding this comment

djkazunoko commented Jul 2, 2023 •

edited

Loading