卒業証書のPDFファイルをアップロードできる機能を追加 #8190

hagiya0121 · 2024-11-13T09:12:55Z

Issue

卒業証書のpdfファイル（pdf の URL）をユーザーに持たせたい #8160

概要

管理者のユーザー登録情報変更画面で卒業証書のPDFファイルをアップロード及び削除できる機能を追加しました。

変更確認方法

feature/add-diploma-pdf-uploadをローカルに取り込む
1. git fetch origin pull/8190/head:feature/add-diploma-pdf-upload
2. git checkout feature/add-diploma-pdf-upload
foreman start -f Procfile.dev でローカルサーバーを立ち上げ
komagata(管理者)でログイン
ユーザー登録情報変更画面にアクセス
卒業証書PDF項目でbootcamp/test/fixtures/files/users/diplomas/diploma.pdf
をアップロードして、ページ下部の更新するボタンを押す
ユーザー登録情報変更画面にアクセス
卒業証書PDF項目にdiploma.pdfと表示され、PDFを確認を押してPDFが表示されることを確認する
卒業証書PDF項目で削除ボタンを押して、ページ下部の更新するボタンを押す
ユーザー登録情報変更画面にアクセス
卒業証書PDF項目でdiploma.pdfという表示が消えていることを確認

Screenshot

変更前

変更後

hagiya0121 · 2024-11-14T01:43:07Z

@machida
お疲れ様です。デザインをお願いしたいです🙏

machida · 2024-11-14T07:30:06Z

@hagiya0121 了解です！！

machida · 2024-11-18T10:15:19Z

@hagiya0121 お待たせしました！！デザイン入れましたのでご確認お願いします。OKでしたらレビューに進めてくださいー

hagiya0121 · 2024-11-23T10:25:46Z

@ham-cap
お疲れ様です。
こちらのPRのレビューをお願いしたいです🙏
ご都合が悪いときは遠慮なくおっしゃってください🙇

ham-cap · 2024-11-28T04:47:05Z

@hagiya0121
返信が大変遅くなってしまい申し訳ありません🙇‍♂️
先日チーム開発のプラクティスを修了してしまったところなので、別の方にご依頼いただけますでしょうか🙏
恐縮ですがよろしくお願いいたします🙏

hagiya0121 · 2024-11-28T07:37:39Z

@ham-cap
分かりました、ご連絡いただきありがとうございます🙇

hagiya0121 · 2024-11-28T07:39:30Z

@mousu-a
お疲れ様です。
こちらのPRのレビューをお願いしたいです🙏
ご都合が悪いときは遠慮なくおっしゃってください🙇

mousu-a · 2024-11-28T13:01:20Z

@hagiya0121
レビュー依頼ありがとうございます！
ぜひ引き受けさせていただきます〜😄
一週間ほどかかりそうなスケジュール感です🙇‍♂️

mousu-a

@hagiya0121
実装お疲れ様です🍵
レビューお待たせいたしました🙏
いくつかコメントしているのでご確認いただければと思います〜

以下 "変更確認方法"について気になったところのフィードバックになります！
5番のpdfのパスについてですが、bootcamp/test/fixtures/files/users/diplomas/diploma.pdf　の方がいいかもです。

あと、「アップロードして更新する」とありますが、「アップロードして、ページ下部の"更新する"ボタンを押す」　とかの方が間違いがないと思います。8番も同様です！
一瞬「画面の更新かな？」と思ってしまいました😅

mousu-a · 2024-12-02T01:11:14Z

app/javascript/fileinput.js

+  const removeDiplomaFlag = document.getElementById('remove-diploma-flag')
+
+  removeDiplomaButton.addEventListener('click', () => {
+    diplomaFileLink && (diplomaFileLink.style.display = 'none')


すみません、ここの説明ほしいです🙏
なぜこのコードが必要なのかがわからない感じです。
diplomaFileLink && (diplomaFileLink.style.display = 'none')

diplomaFileLink && (diplomaFileLink.style.display = 'none')の動作としてはファイルアップロード後(更新するを押した後)のリンクを非表示にしています。見た目はリンクっぽくないんですけど画像の赤枠全体がリンクです。

これがないとファイルアップロード後に削除を押しても画像のようにリンクが残ったままになってしまいます。

diplomaFileLinkが存在する場合のみ右辺が発火するわけですね。
勉強になります🙏

その場合下記のような形ではどうでしょうか？

if (diplomaFileLink) diplomaFileLink.style.display = 'none'

町田さんがデザインを入れた際に変更してくれたみたいです🤔18c14ab2e02b8526752

mousu-a · 2024-12-02T01:17:49Z

app/views/users/_form.html.slim

+        - if @user.diploma_file.attached?
+          = link_to @user.diploma_file.filename, url_for(@user.diploma_file), class: 'a-text-input', id: 'diploma-file-link'
+        - display = @user.diploma_file.attached? ? 'display: none' : 'display: block'
+        = f.file_field :diploma_file, class: 'a-text-input', id: 'diploma-upload-field', style: display


display　→ display_styleでもいいかもです。

そちらの名前の方がわかりやすいですね。変更しました！
fd1d8db

mousu-a · 2024-12-02T01:21:09Z

app/views/users/_form.html.slim

+        - display = @user.diploma_file.attached? ? 'display: none' : 'display: block'
+        = f.file_field :diploma_file, class: 'a-text-input', id: 'diploma-upload-field', style: display
+        button type='button' id='remove-diploma-button' class='a-button is-md is-secondary' 削除
+


button_tagが使えそうです！

変更しました
3aeb25a

ファイルがアップロードされていない状態で削除ボタンがあるのはおかしい気がします🤔

mousu-a · 2024-12-03T00:55:57Z

config/locales/ja.yml

@@ -111,6 +111,7 @@ ja:
        other_editor: その他のエディタ
        hide_mentor_profile: プロフィール非公開
        invoice_payment: 「請求書払い」
+        diploma_file: 卒業証書PDF


提案です！
"卒業証書(PDF)"とかでもいいかもです。

そちらの方がいいと思います。変更しました
b2ff53d

mousu-a · 2024-12-03T01:03:00Z

app/javascript/fileinput.js

@@ -85,3 +85,42 @@ document.addEventListener('DOMContentLoaded', () => {
  }
  initializeFileInput(document)
 })
+


今回実装する部分を関数にして↑の　DOMContentLoadedに入れてはどうでしょうか？

あと画面の読み込み時に実行していますが、該当する画面でない場合のreturnがなさそうです。

変更しました
6b5d653

mousu-a · 2024-12-03T01:04:05Z

app/views/users/_form.html.slim

@@ -104,6 +104,28 @@
            .form-item-block__item
              = render 'users/form/graduate', f: f

+      .form-item
+        // 卒業証書


このコメントは必要ないかもです🙆

削除しました
fd1d8db

mousu-a · 2024-12-03T01:19:08Z

app/javascript/fileinput.js

@@ -85,3 +85,42 @@ document.addEventListener('DOMContentLoaded', () => {
  }
  initializeFileInput(document)
 })
+
+document.addEventListener('DOMContentLoaded', () => {
+  const removePdfButton = document.getElementById('js-remove-pdf-button')


この辺りの変数名も、先のコメントで触れたように関数で括れば変数名にpdfってつけなくて良さそうです。

変数名の変更とリファクタリングも一緒に行いました。
6b5d653

hagiya0121 · 2024-12-04T01:06:33Z

@mousu-a
お疲れ様です！
指摘された点の修正と動作確認方法も確かにわかりづらかったので修正しました🙏
確認お願いします🙇

mousu-a

@hagiya0121
レビューお待たせいたしました🙏
いくつかコメントしているのでご確認いただければと思います〜

mousu-a · 2024-12-06T03:25:09Z

app/views/users/_form.html.slim

+        - display = @user.diploma_file.attached? ? 'display: none' : 'display: block'
+        = f.file_field :diploma_file, class: 'a-text-input', id: 'diploma-upload-field', style: display
+        button type='button' id='remove-diploma-button' class='a-button is-md is-secondary' 削除
+


ファイルがアップロードされていない状態で削除ボタンがあるのはおかしい気がします🤔

mousu-a · 2024-12-06T03:34:29Z

app/javascript/fileinput.js

@@ -75,6 +75,45 @@ function extractField(elements) {
  }
 }

+function initializePdfUploadField() {


提案です！
initializeDiplomaUploadFieldの方がいいかも？

mousu-a · 2024-12-06T03:39:18Z

app/javascript/fileinput.js

+  const removeButton = document.getElementById('js-remove-pdf-button')
+  const fileLink = document.getElementById('js-pdf-file-link')
+  const removeFlag = document.getElementById('js-remove-pdf-flag')
+  const fileNameDisplay = document.getElementById('js-pdf-name')


提案です！
fileNameDisplay → fileName でも良さそうです🙆

mousu-a · 2024-12-06T03:41:38Z

app/javascript/fileinput.js

+  const fileNameDisplay = document.getElementById('js-pdf-name')
+  const fileInput = uploadField.querySelector('input[type="file"]')
+
+  const updateFileNameDisplay = (name = '') => {


提案です！
updateDisplayedFileName とかではどうでしょうか？

mousu-a · 2024-12-06T03:43:39Z

app/javascript/fileinput.js

+  }
+
+  removeButton.addEventListener('click', () => {
+    if (fileLink) fileLink.style.display = 'none'


ファイルがアップロードされている時のみ削除ボタンが表示されていれば、if文は不要になるかもです

fileLinkはHTMLが生成されない場合があるのでエラーになるようです。

mousu-a · 2024-12-07T01:20:14Z

app/javascript/fileinput.js

+  const fileInput = uploadField.querySelector('input[type="file"]')
+
+  const updateFileNameDisplay = (name = '') => {
+    if (name) {


こんな感じで書けそうです🙆

fileNameDisplay.textContent = name const displayedStatus = name ? 'block' : 'none' fileNameDisplay.style.display = displayedStatus

指摘された点とfileInputのイベントリスナーのロジックもリファクタリングしました。
97b19ed

hagiya0121 · 2024-12-09T07:20:14Z

@mousu-a
お疲れ様です！指摘された点を修正したので確認お願いします🙏

mousu-a

@hagiya0121
変更ありがとうございます！
気になる点少しだけコメントさせていただきましたのでご確認をお願いいたします🙇‍♂️
他は良さそうです👌

mousu-a · 2024-12-10T02:37:16Z

app/javascript/fileinput.js

+  uploadField.style.display = fileLink ? 'none' : 'flex'
+  removeButton.style.display = fileLink ? 'block' : 'none'
+
+  const updateDisplayedFileName = (name = '') => {


細かい点ですが、removeButton.style.display = displayedStatusを関数の中に入れるのならばupdateDisplayedFileNameは命名を変更したほうが良さそうに思いました。

"表示状態をアップデートする"ということでupdateDisplayStatus 、updateDisplayState とかどうでしょうか。StateはReactにもある概念なので相手に意味が伝わりやすいかも？

もっと良いものがあればそちらにしていただいて構いません🙇‍♂️

updateDisplayStateがわかりやすそうなのでそっちに変更しました。

mousu-a · 2024-12-10T02:38:10Z

app/javascript/fileinput.js

+  const fileName = document.getElementById('js-pdf-name')
+  const fileInput = uploadField.querySelector('input[type="file"]')
+
+  uploadField.style.display = fileLink ? 'none' : 'flex'


fileLinkをよくif文に使われているので、if文の条件として分かりやすい名前をつけてあげると可読性があがるかもです。
こんな感じでどうでしょうか。const isFileUploaded = fileLink

必要あるか微妙なんですけどisFileUploadedは名前的に真偽値を期待させるので二重否定して真偽値になるようにしました。

hagiya0121 · 2024-12-10T11:58:18Z

@mousu-a
修正しました！確認お願いします🙇

mousu-a

#8190 (comment)

isFileUploadedは名前的に真偽値を期待させる

すみません、後で気づきました🙏
個人的には、fileLink はそのままで変数名をfileUploaded とかにするのが丸いように思います。
（たしかrubocopの方で二重否定はOUTとされていたので、個人的に二重否定に対して若干否定的な気持ちがあります😅）

ただ修正必須ではないのでApproveとさせていただきます〜。
変更ありがとうございました！

hagiya0121 · 2024-12-11T02:42:50Z

@mousu-a
何度もレビューありがとうございました🙇デフォルトのrubocopだと二重否定ダメなのは知りませんでした💡

hagiya0121 · 2024-12-11T02:43:36Z

@komagata
メンバーのレビューが完了したので確認お願いします🙇

hagiya0121 added 2 commits November 13, 2024 17:58

卒業証書のアップロード機能を追加

496b8ec

テストを追加

8385b8a

hagiya0121 requested a review from machida November 14, 2024 01:42

machida assigned machida and hagiya0121 Nov 15, 2024

machida added 2 commits November 18, 2024 19:10

pdfアップロードのデザインを作成

3d8dc4e

pdf input のsassファイルを分けた

f3bc5cc

machida approved these changes Nov 18, 2024

View reviewed changes

machida removed their assignment Nov 18, 2024

UIの変更に合わせてテストを修正

d16a661

hagiya0121 requested a review from ham-cap November 23, 2024 10:25

hagiya0121 marked this pull request as ready for review November 23, 2024 22:45

hagiya0121 requested review from mousu-a and removed request for ham-cap November 28, 2024 07:38

mousu-a reviewed Dec 3, 2024

View reviewed changes

hagiya0121 added 4 commits December 3, 2024 17:35

変数名の変更と不要なコメントを削除

fd1d8db

卒業証書PDFの表記を「卒業証書(PDF)」に変更

b2ff53d

削除ボタンをbutton_tagで生成するように変更

3aeb25a

javascriptのロジックの関数化とリファクタリング

6b5d653

mousu-a reviewed Dec 7, 2024

View reviewed changes

ファイルがアップロードされてるときのみ削除ボタンを表示

9b55b38

hagiya0121 added 2 commits December 8, 2024 20:37

変数名とメゾッド名を変更

7d82332

ファイル名表示ロジックとイベントハンドラーのロジックを簡潔化

97b19ed

mousu-a reviewed Dec 10, 2024

View reviewed changes

変数名の変更

ba692b1

mousu-a approved these changes Dec 10, 2024

View reviewed changes

卒業証書のPDFファイルをアップロードできる機能を追加 #8190

Are you sure you want to change the base?

卒業証書のPDFファイルをアップロードできる機能を追加 #8190

Conversation

hagiya0121 commented Nov 13, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

hagiya0121 commented Nov 14, 2024

machida commented Nov 14, 2024

machida commented Nov 18, 2024

hagiya0121 commented Nov 23, 2024

ham-cap commented Nov 28, 2024

hagiya0121 commented Nov 28, 2024 • edited Loading

hagiya0121 commented Nov 28, 2024

mousu-a commented Nov 28, 2024 • edited Loading

mousu-a left a comment • edited Loading

Choose a reason for hiding this comment

mousu-a Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 3, 2024 • edited Loading

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

mousu-a Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 commented Dec 4, 2024

mousu-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

mousu-a Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 commented Dec 9, 2024

mousu-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mousu-a Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

hagiya0121 commented Dec 10, 2024

mousu-a left a comment • edited Loading

Choose a reason for hiding this comment

hagiya0121 commented Dec 11, 2024

hagiya0121 commented Dec 11, 2024

hagiya0121 commented Nov 13, 2024 •

edited

Loading

hagiya0121 commented Nov 28, 2024 •

edited

Loading

mousu-a commented Nov 28, 2024 •

edited

Loading

mousu-a left a comment •

edited

Loading

mousu-a Dec 2, 2024 •

edited

Loading

hagiya0121 Dec 3, 2024 •

edited

Loading

mousu-a Dec 3, 2024 •

edited

Loading

mousu-a Dec 2, 2024 •

edited

Loading

mousu-a Dec 2, 2024 •

edited

Loading

mousu-a Dec 3, 2024 •

edited

Loading

mousu-a Dec 3, 2024 •

edited

Loading

mousu-a Dec 3, 2024 •

edited

Loading

hagiya0121 Dec 4, 2024 •

edited

Loading

mousu-a Dec 6, 2024 •

edited

Loading

mousu-a Dec 6, 2024 •

edited

Loading

mousu-a Dec 6, 2024 •

edited

Loading

mousu-a Dec 7, 2024 •

edited

Loading

hagiya0121 Dec 8, 2024 •

edited

Loading

mousu-a Dec 10, 2024 •

edited

Loading

hagiya0121 Dec 10, 2024 •

edited

Loading

mousu-a left a comment •

edited

Loading