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

[動画機能] 動画追加・一覧の追加 #7535

Open
wants to merge 68 commits into
base: main
Choose a base branch
from

Conversation

a-terumoto-gs
Copy link
Contributor

@a-terumoto-gs a-terumoto-gs commented Mar 13, 2024

Issue

概要

動画の追加機能、一覧表示機能を新機能として実装しました。

変更確認方法

  1. feature/add-video-addition-and-list-display-functionをローカルに取り込む
  2. bin/setuprails db:migraterails db:seedを実行
  3. foreman start -f Procfile.devでアプリを起動し、ログインする
  4. 左側のサイドバーの「Docs・動画」をクリックして /pagesページが表示されることを確認
  5. 動画一覧ページの確認項目をチェックする
    • 動画タブをクリックすると/moviesページが表示されること
    • 動画が2つあり、各々にサムネイル・タイトル・追加したユーザー名・追加日時が表示されていること
    • mp4動画のほうにはタグが表示されていること
    • 右上に「+動画を追加」ボタンがあり、リンク先が正しいこと
      • movies/newに遷移する
  6. 新規追加ページの確認項目をチェックする
    • 関連プラクティスが選択できる
    • 右上に「<動画一覧」ボタンがあり、リンク先が正しいこと
      • moviesに遷移する
    • 関連プラクティスが選択できる
    • タイトルが入力できる
    • 説明欄ではMarkdownが使用できる
    • 動画データがmp4形式、mov形式で添付できる
    • タグが追加できる
    • 動画の追加を押すと動画詳細ページに遷移する
  7. 個別ページの確認項目をチェックする
    • 上部に「+動画を追加」「<動画一覧」ボタンがあり、リンク先がそれぞれ正しいこと
    • 動画の上部に、Docsと同じような形でプラクティス名、タイトル、公開日、ユーザーアイコン、コメント数、Watchボタン、Bookmarkボタン、タグ欄が表示されていること
    • 動画が再生できること
    • 動画の下に説明欄があること
    • リアクションボタン、コメント欄の表示があり、機能すること
    • 一覧に戻るボタンがあること
    • mp4動画とmov動画で動画の表示サイズがそろっていること
  8. ユーザー削除時の表示を確認する
    一般ユーザーで動画を追加した後ログアウトし、管理者権限を持つユーザーでログインしなおし、先ほど動画を追加したユーザーを削除した際、ユーザー欄がghost表示になることを確認する

現時点でのタグの仕様についてですが、以下は現時点での正しい仕様です

  • 一覧ページでのタグをクリックするとそのまま一覧ページを読み込みなおす
  • 個別ページでのタグをクリックするとRouting Errorになる

一覧画面における動画のサムネイルについてですが、今回のissueではサンプル画像をはめる形の仕様で進めることになりましたので、すべて同じ画像が表示されます

Screenshot

既存のデザインからの変更点

左のタブが「Docs」→「Docs・動画」に
「Docs」と「動画」のタブが追加
上部のページタイトルが「Docs」→「Docs・動画」に
動画ページの追加に合わせて「Docs」の表示が出る位置、「+Docs作成」の位置も変更になっております
image

無題

新規ページ

一覧ページ
image

新規作成ページ
image

動画個別ページ
image

ユーザー削除時の表示
image

image

@a-terumoto-gs a-terumoto-gs force-pushed the feature/add-video-addition-and-list-display-function branch 2 times, most recently from 420c907 to 340bc80 Compare March 22, 2024 11:04
@a-terumoto-gs a-terumoto-gs force-pushed the feature/add-video-addition-and-list-display-function branch 3 times, most recently from fe39469 to 18548b3 Compare April 8, 2024 08:01
@a-terumoto-gs
Copy link
Contributor Author

@machida
おつかれさまです!
こちらの機能ですが、実装がおおむね終了したのでデザインを依頼したいです!
新しく追加した3画面のデザインをお願いしたいです。

全体としてDocsのデザインに寄せたつもりで実装していますが、だいぶ表示が崩れてしまっております。

  • 一覧画面
    image
    動画一つ一つの表示はポートフォリオ一覧ページを参考にして実装しております。

  • 新規作成画面
    Image from Gyazo

  • 詳細画面
    Image from Gyazo

お手数をおかけしますがよろしくお願いいたしますm(__)m

@a-terumoto-gs
Copy link
Contributor Author

@machida
上記で依頼した新規作成画面についてですが、別issueでの編集機能追加のために入力欄をformとして切り出して使うようにしたところ、新規作成画面でもデザインがほぼ崩れないことが判明したので、手間を減らすためにこちらのissueにもそちら適応しております。
といったわけで確認は必要かと思いますが大きな調整は不要になったと思います!
後出しで申し訳ありませんが確認よろしくお願いいたします。

Image from Gyazo

@machida
Copy link
Member

machida commented Apr 9, 2024

@a-terumoto-gs 了解ですー では、formを確認させていただきますー
それ以外のページのデザインも進めます!

@a-terumoto-gs a-terumoto-gs force-pushed the feature/add-video-addition-and-list-display-function branch from df10637 to 4f8f9e1 Compare April 9, 2024 08:05
@a-terumoto-gs
Copy link
Contributor Author

@machida
コメントありがとうございます!
個別ページのデザインも同様にできましたのでheaderとして切り出して追加でコミットしております!
該当ファイル↓
_movie_header.html.slim
image

@machida
Copy link
Member

machida commented Apr 9, 2024

@a-terumoto-gs デザイン入れてpush しましたー

@a-terumoto-gs
Copy link
Contributor Author

a-terumoto-gs commented Apr 10, 2024

レビュー用のファイル差分内訳

修正が入っているファイルが多いので内訳を書いておきます

  • 本issueで新たに追加したもの
    • 設定追加とコントローラ、モデル
      • app/assets/javascripts/applicat
      • app/controllers/movies_controller.rb
      • app/models/movie.rb
      • app/models/practice.rb
      • app/models/user.rb
    • 画面表示関係のファイル
      • app/views/movies/_form.html.slim
      • app/views/movies/_movie_header.html.slim
      • app/views/movies/_movie_list.html.slim
      • app/views/movies/index.html.slim
      • app/views/movies/new.html.slim
      • app/views/movies/show.html.slim
      • app/views/pages/_doc_movie_header.html.slim
      • app/views/pages/_tabs.html.slim
      • config/locales/ja.yml
    • タグのコンポーネント用:app/controllers/api/movies_contoller.rb
    • マイグレーションファイルやセットアップ関係のファイル:
      • db/fixtures/acts_as_taggable_on/taggings.yml
      • db/migrate/20240321092012_create_movies.rb
      • db/seeds.rb
      • lib/bootcamp/setup.rb
      • test/fixtures/active_storage/attachments.yml
      • test/fixtures/active_storage/blobs.yml
      • db/fixtures/files/movies/movie1.mp4
      • db/fixtures/files/movies/movie2.mov
      • db/fixtures/files/movies/thumbnail1.jpg
      • db/fixtures/files/movies/thumbnail2.jpg
      • db/fixtures/movies.yml
    • ルーティング:
      • config/routes.rb
      • config/routes/api.rb
    • テストとテスト用データ:
      • test/system/movies_test.rb
      • test/fixtures/files/movies/movie1.mp4
      • test/fixtures/files/movies/movie2.mov
      • test/fixtures/movies.yml
  • 既存のものの表示を変更したもの
    • Docsページの上部の文言:app/views/works/index.html.slim
    • サイドバーの文言:app/views/application/_global_nav.slim
  • デザイン変更と実装の変更があるもの(動画のものと一部共通化)
    • app/views/pages/index.html.slim
    • app/views/pages/new.html.slim
    • app/views/pages/show.html.slim
  • 町田さんのデザインの変更のみ
    • app/javascript/components/Tags/TagEditButton.jsx
    • app/javascript/components/Tags/TagEditModal.jsx
    • app/javascript/stylesheets/application/blocks/page/_page-header-actions.sass
    • app/javascript/stylesheets/shared/blocks/card/_card-body.sass
    • app/javascript/stylesheets/shared/blocks/card/_thumbnail-card.sass
    • app/views/pages/_doc_header.html.slim
    • app/views/pages/_page.html.slim
    • app/views/works/_work.html.slim
  • デザイン変更に伴う修正が加わったテスト
    • test/system/notification/pages_test.rb
    • test/system/pages_test.rb

@a-terumoto-gs a-terumoto-gs force-pushed the feature/add-video-addition-and-list-display-function branch from 2773de8 to 43e1048 Compare April 10, 2024 10:42
@a-terumoto-gs a-terumoto-gs marked this pull request as ready for review April 10, 2024 11:05
@a-terumoto-gs
Copy link
Contributor Author

a-terumoto-gs commented Apr 10, 2024

@kyokucho1989
お疲れ様です!
お忙しいところ恐縮ですが、お手すきの際にこちらのissueのレビューをお願いしてもよろしいでしょうか?
急ぎではありません。

新機能なので差分が多くなっているので、内訳をコメントでまとめています。
不明な点があれば遠慮なくお伝えください!
よろしくお願いいたしますm(__)m

@kyokucho1989
Copy link
Contributor

@a-terumoto-gs
了解しました!一週間ほどお待ちくださいー

@kyokucho1989
Copy link
Contributor

@a-terumoto-gs
ざっと確認してますー
質問です。

  • 動画の追加の権限
    これは誰でもできる仕様ですか?

  • Movieモデルの関連付けについて
    MovieモデルはUserモデルやPracticeモデルとは独立した関係の方がよいかと思います。ユーザーやプラクティスが削除されてしまったときの処理が難しくなりそうです。
    どうでしょうか?

@a-terumoto-gs
Copy link
Contributor Author

a-terumoto-gs commented Apr 11, 2024

@kyokucho1989
コメントありがとうございます

  • 動画の追加の権限
    これは誰でもできる仕様ですか?

そうです。追加者に制限はないです。

  • Movieモデルの関連付けについて
    MovieモデルはUserモデルやPracticeモデルとは独立した関係の方がよいかと思います。ユーザーやプラクティスが削除されてしまったときの処理が難しくなりそうです。
    どうでしょうか?

その点深く考えられていませんでしたが、ユーザーに関しては確かにそうかもしれません。
ユーザーについてはご指摘の通り、独立させる仕様に変更しようかと思います。

プラクティスについては、個人的には関連付いていてよいのではないか、と考えます。
プラクティスを選択する場合はプラクティスに関連付いた動画をあげるときになると思うため、そのプラクティスの削除と同時に動画が消えても支障がないかなと思います。
プラクティスに関係ない動画を追加する際には(LT会の動画など)、プラクティス選択をせずに追加できるため、プラクティス削除で動画も消えることはないので支障はないかと思います。

この点に関しては駒形さんにも確認をしていただこうと思いますm(__)m

@komagata
モデル間の関連付けについて質問です。
MovieモデルとUserモデルやPracticeモデルの関連付けについて上記のように考えていますが、いかがでしょうか?

@a-terumoto-gs a-terumoto-gs force-pushed the feature/add-video-addition-and-list-display-function branch from 43e1048 to 641b15a Compare April 17, 2024 08:02
Copy link
Contributor

@kyokucho1989 kyokucho1989 left a comment

Choose a reason for hiding this comment

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

@a-terumoto-gs

レビューしました。コメントします。
動画を添付できないエラーが発生しています。
確認をお願いしますー

Comment on lines 18 to 19
validates :title, presence: true
validates :description, presence: true
Copy link
Contributor

Choose a reason for hiding this comment

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

文字数の長さに制限を加えた方がいいと思います。デザインが崩れるのを予防できそうです。

Copy link
Contributor Author

@a-terumoto-gs a-terumoto-gs Apr 19, 2024

Choose a reason for hiding this comment

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

titleには、他の制限を加えている箇所を参考に、上限を255文字として制限を加えました。
descriptionは説明欄なのでどれくらいの上限を付ければ十分かわからなかったこと、他の似たような箇所でも制限を付けている箇所がなかったことも踏まえ、そのままにしています。

Copy link
Contributor

Choose a reason for hiding this comment

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

user ではなくtitle にお願いします…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コードを追加する行が一行ずれていたことに気づいていませんでした…ご指摘ありがとうございます
titleの行に修正しましたm(__)m

app/views/movies/index.html.slim Show resolved Hide resolved
app/controllers/movies_controller.rb Outdated Show resolved Hide resolved
if @movie.save
redirect_to @movie, notice: '動画を追加しました。'

movie_path = save_blob_to_tempfile(@movie.movie_data.blob)
Copy link
Contributor

Choose a reason for hiding this comment

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

この結果、
movie_path = "tmp/.mp4.mp420240418-3064-xe6xyp" というpathが生成されました。
.を含んでしまっています。
動画をアップロードできないエラーの原因になっているかもしれません。

Copy link
Contributor Author

@a-terumoto-gs a-terumoto-gs Apr 19, 2024

Choose a reason for hiding this comment

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

上記のエラーについて検証後、pathの中に.が含まれてしまうようなタイトルの動画で検証しましたが、エラーがでないことを確認しました!
よって変更を加えずにそのままにしております。
検証できていなかったので助かりますm(__)m

@komagata
Copy link
Member

@a-terumoto-gs

モデル間の関連付けについて質問です。
MovieモデルとUserモデルやPracticeモデルの関連付けについて上記のように考えていますが、いかがでしょうか?

movieの持ち主(投稿者)という意味でuserとの関連が欲しいです。

また、関連プラクティスという意味でpracticeとの関連が欲しいです。(0~n個のプラクティスと関連)

@a-terumoto-gs
Copy link
Contributor Author

a-terumoto-gs commented Apr 19, 2024

モデル同士の関連付けについて

UserモデルとPracticeモデル、どちらが消えても関連付けられたMovieモデルは残る仕様にする

実装に当たって、dependetntオプションなしとdependent: nullifyのどちらにするか悩んだが、

  • オプションなし:親オブジェクトが削除されても、子オブジェクトの外部キーが残ったままになる、参照先のない外部キーが残る
  • nullify:親オブジェクトが削除されると。子オブジェクトの外部キーがnullになり、他の親オブジェクトと関連付けられるようになる

それぞれの方法の削除後の仕様を考慮して、参照先のない外部キーが残るのはよくないだろう、新しく作成した親オブジェクトとの関連付けをし直すかも、と考えnullifyで実装することとした。
これに伴い、nullでもエラーが出ないようにMovieテーブルを修正

a-terumoto-gs and others added 24 commits June 28, 2024 19:55
…deliting-function

[動画機能] 動画編集・削除機能の追加
@komagata
Copy link
Member

📝 編集・削除などのブランチがこちらにマージされたらリリースする。

@ai-24
Copy link
Contributor

ai-24 commented Nov 2, 2024

@komagata @machida
お疲れ様です!
今回こちらのPRをmainブランチにマージするにあたって「コンフリクトを解消し、Tab Componentを使用すべき部分に変更を加える」というIssueを割り振っていただきました。
現在私のローカルでこちらのPRの同期を進めております。
一度レビューはapproveされているかと思うのですが、schemaに気になる点がいくつかあったので質問+確認させていただきたく、上記にいくつかコメントさせていただきました。

上記のコメントに至った経緯は以下になります。

  1. こちらのブランチを私のローカルに取り込み、bin/setuprails db:migrateを実行しschemaファイルが更新されたことを確認。
  2. 1つずつ内容を確認したところ、今回のPRで追加・変更されたマイグレーションファイルからは読み取れない変更を発見
  3. リモートのmainブランチでschemaを確認し、今回発生した変更はmainブランチでは見当たらなかったので、このPR内で加えられた変更だと判断
  4. 2347484 でschemファイルに変更が加えられているようだったので、その1つ前のコミットまで戻ってmigrationを実行し直した。(今回ここでrails db:resetを実行しました。本来はやるべきでないと思うのですが、チームに戻ってきてからまだ1つのissueしかしておらず、特に開発環境で入れているデータがなかったので実行する判断になりました)
  5. コミットされているschemaファイル内でコンフリクトが発生していたのでDBの作り直しが出来なかった
  6. コンフリクトが発生する前のコミットまで戻ってrails db:reset
    047449f
  7. 当初出ていたschemaの差分が出ないことを確認。下記差分のみ。
create_table "movies", force: :cascade do |t|
     t.string "title", null: false
     t.text "description"
-    t.bigint "user_id", null: false
+    t.bigint "user_id"
     t.bigint "practice_id"
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
  1. その後 4 のコミットまでにマイグレーションファイルが追加・変更されたコミットが下記のみだったので内容を確認
    d93db24
  2. 特に気になった点(上記コメントの部分)の変更は今回のPRで加えられていないはずと判断

認識が間違っている部分があるかと思います。
ご教示いただけますでしょうか。
よろしくお願いいたします。

@ai-24
Copy link
Contributor

ai-24 commented Nov 2, 2024

@komagata @machida
度々申し訳ありません。
こちらのPRを作成されたa-terumoto-gsさんがもう開発メンバーではないようだったので、お2人に質問させていただきました。
よろしくお願いいたします!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 作業中
Development

Successfully merging this pull request may close these issues.

5 participants