-
Notifications
You must be signed in to change notification settings - Fork 3
[fix] 保健所提出書類のテンプレートを総務局現行様式に修正 #1996
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
base: gm3/develop
Are you sure you want to change the base?
[fix] 保健所提出書類のテンプレートを総務局現行様式に修正 #1996
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
| <h3><%= fp.name %></h3> | ||
| <div class="detail-area"> | ||
| <% if cpo %> | ||
| <%= cpo.tent.present? ? simple_format(cpo.tent) : '調理行程が未登録です。' %><br> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「調理行程」は誤字です。正しくは「調理工程」です。
| <%= cpo.tent.present? ? simple_format(cpo.tent) : '調理行程が未登録です。' %><br> | |
| <%= cpo.tent.present? ? simple_format(cpo.tent) : '調理工程が未登録です。' %><br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これは直して
|
|
||
| send_data pdf.to_pdf, | ||
| filename: "#{output_file_name}.pdf", | ||
| # disposition: "inline", # ダウンロードせず表示する |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントアウトされたコード(# disposition: "inline", # ダウンロードせず表示する)が残っています。必要ない場合は削除すべきです。
| <td> | ||
| <% pd = purchase_list.purchase_date %> | ||
| <% formatted_pd = if pd.respond_to?(:strftime) | ||
| pd.strftime("%-m/%-d") | ||
| elsif pd.present? | ||
| begin | ||
| Date.parse(pd.to_s).strftime("%-m/%-d") | ||
| rescue | ||
| pd.to_s | ||
| end | ||
| else | ||
| "" | ||
| end %> | ||
| <%= formatted_pd %> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
仕入れ日の表示ロジックが複雑すぎて、メンテナンスが難しいです。また、Date.parse の rescue ブロックで例外をキャッチしていますが、エラーハンドリングが不十分です。この複雑な日付フォーマットロジックはヘルパーメソッドに切り出すことを推奨します。
| <span>営業許可施設で調理された既製食品を現地で盛り付けるものは、「既製食品」欄に◯を記入</span> | ||
| </div> | ||
| <div class="page-break"></div> | ||
| <div class="page-break" style="page-break-after: always;"></div> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
インラインスタイル(style="page-break-after: always;")が使用されていますが、これはCSSファイルで定義されている .page-break クラスと重複しています。CSSクラスのみを使用するべきです。
| <div class="page-break" style="page-break-after: always;"></div> | |
| <div class="page-break"></div> |
| <%= fes_date.date %><br> | ||
| </div> | ||
| <% end %> | ||
| <% else %> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
空の else ブロックが存在しています。このブロックは削除するか、または販売品が未登録の場合のメッセージを表示すべきです。
| <% else %> | |
| <% else %> | |
| <p>販売品が未登録です。</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
71行目の <% if group.food_products.any? %>かな?
これならendで終わってもよさそう
| <td class="Belonging"><%= group.name %></td> | ||
| <td class="Name"><%= group.user.name %></td> | ||
| <td class="Category"> | ||
| <% student_id = (group.user.user_detail&.student_id || 0).to_i %> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safe navigation operator(&.)を使用しているにもかかわらず、nil の場合のデフォルト値として 0 を設定しています。|| 0 は safe navigation operator で nil が返された場合にも適用されますが、より明示的に &.student_id.to_i または dig(:user_detail, :student_id)&.to_i || 0 のようにすべきです。
| def print_pdf_with_header_footer(template_name, style_name, output_file_name, type) | ||
| respond_to do |format| | ||
| format.pdf do | ||
| html = render_to_string template: "print_pdf/#{template_name}" | ||
| pdf_options = { | ||
| page_size: 'A4', | ||
| encoding: 'UTF-8', | ||
| margin_top: '25mm', | ||
| margin_bottom: '25mm', | ||
| margin_left: '10mm', | ||
| margin_right: '10mm', | ||
| header_spacing: 5, | ||
| footer_spacing: 5, | ||
| header_left: '技大祭実行委員会 総務局', | ||
| header_line: false, | ||
| footer_center: '[page] / [topage]', | ||
| footer_right: "#{Time.now.getlocal('+09:00').strftime('%Y/%-m/%-d %-H:%M')} Group Manager", | ||
| footer_line: false, | ||
| footer_font_size: 10, | ||
| header_font_size: 10, | ||
| outline: true | ||
| } | ||
|
|
||
| pdf_options[:orientation] = 'Landscape' if type == 'Landscape' | ||
| pdf = PDFKit.new(html, pdf_options) | ||
|
|
||
| pdf.stylesheets << Rails.root.join("app/views/print_pdf/#{style_name}.css").to_s | ||
|
|
||
| send_data pdf.to_pdf, | ||
| filename: "#{output_file_name}.pdf", | ||
| # disposition: "inline", # ダウンロードせず表示する | ||
| type: 'application/pdf' | ||
| end | ||
| end | ||
| end |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
新しいメソッド print_pdf_with_header_footer が追加されていますが、既存の print_pdf メソッドとコードが大部分重複しています。DRY原則に違反しており、メンテナンスが困難です。共通部分を抽出して、ヘッダー・フッターのオプションをパラメータとして渡すようにリファクタリングすべきです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これそうだねー
with_header_footerに切り替えるなら既存のprint_pdfを削除しちゃってもいいと思うんだけどprint_pdfを使う時ってある?
| <% student_id = (group.user.user_detail&.student_id || 0).to_i %> | ||
| <% if student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> | ||
| </td> | ||
| </tr> | ||
| <% if group.sub_rep %> | ||
| <tr> | ||
| <td class="No">2</td> | ||
| <td class="Belonging"><%= group.name %></td> | ||
| <td class="Name"><%= group.sub_rep.name %></td> | ||
| <td class="Category"> | ||
| <% sub_rep_student_id = (group.sub_rep&.student_id || 0).to_i %> | ||
| <% if sub_rep_student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif sub_rep_student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> | ||
| </td> | ||
| <td> | ||
| <% @fes_dates.select { |fes_date| fes_date.fes_year_id == group.fes_year_id && fes_date.days_num == 1 }.each do |fes_date| %> | ||
| <%= fes_date.date %><br> | ||
| <% end %> | ||
| <% @fes_dates.select { |fes_date| fes_date.fes_year_id == group.fes_year_id && fes_date.days_num == 2 }.each do |fes_date| %> | ||
| <%= fes_date.date %><br> | ||
| </tr> | ||
| <% end %> | ||
| <% group.employees.each_with_index do |employee, index| %> | ||
| <tr> | ||
| <td class="No"><%= group.sub_rep ? index +3 : index +2 %></td> | ||
| <td class="Belonging"><%=group.name %></td> | ||
| <td class="Name"><%=employee.name %></td> | ||
| <td class="Category"> | ||
| <% employee_student_id = (employee&.student_id || 0).to_i %> | ||
| <% if employee_student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif employee_student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
学籍番号による区分判別ロジック(88888888=学外、99999999=教職員)がマジックナンバーとして3箇所に重複しています。この判別ロジックをモデルメソッドまたはヘルパーメソッドに切り出すべきです。コードの重複が多く、メンテナンス性が低下しています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これできそう?マジックナンバーは対応しておきたい。適当にcursorに投げたの載せとく。
user_details.rb
# 学籍番号の特殊値(マジックナンバー)
STUDENT_ID_EXTERNAL = 88888888 # 学外
STUDENT_ID_STAFF = 99999999 # 教職員
※新規ファイル作ってる。別にどこかに書けばいいと思う。
application_helper.rb
# frozen_string_literal: true
module ApplicationHelper
# student_idからユーザータイプ(学外/教職員/学生)を判定して返す
def user_category_label(student_id)
return '学外' if student_id == UserDetail::STUDENT_ID_EXTERNAL
return '教職員' if student_id == UserDetail::STUDENT_ID_STAFF
'学生'
end
end
そしたらerbの中身これだけで良くなるらしい
<%= user_category_label((group.user.user_detail&.student_id || 0).to_i) %>
| <% else %> | ||
| <% end %> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
空の else ブロックが存在しています。団体がない場合のメッセージを表示するか、このブロックを削除すべきです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コントローラー側でgroup.exists()で存在確認をしているからこのerbでgroupあるかどうかのifチェックは不要になる!
のでこのifとめっちゃしたにあるend消してネスト減らそう!!!
|
これ正解が見たいから出力したpdfくれると嬉しい |
|
2020だと白紙で出力されて他の年度にしたらpdf開けなかった。 |
hikahana
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コードレビューと動作確認だけ
正式なファイルになっているかどうかはあとむになげまーす
@Atomic-Bombs
| def print_pdf_with_header_footer(template_name, style_name, output_file_name, type) | ||
| respond_to do |format| | ||
| format.pdf do | ||
| html = render_to_string template: "print_pdf/#{template_name}" | ||
| pdf_options = { | ||
| page_size: 'A4', | ||
| encoding: 'UTF-8', | ||
| margin_top: '25mm', | ||
| margin_bottom: '25mm', | ||
| margin_left: '10mm', | ||
| margin_right: '10mm', | ||
| header_spacing: 5, | ||
| footer_spacing: 5, | ||
| header_left: '技大祭実行委員会 総務局', | ||
| header_line: false, | ||
| footer_center: '[page] / [topage]', | ||
| footer_right: "#{Time.now.getlocal('+09:00').strftime('%Y/%-m/%-d %-H:%M')} Group Manager", | ||
| footer_line: false, | ||
| footer_font_size: 10, | ||
| header_font_size: 10, | ||
| outline: true | ||
| } | ||
|
|
||
| pdf_options[:orientation] = 'Landscape' if type == 'Landscape' | ||
| pdf = PDFKit.new(html, pdf_options) | ||
|
|
||
| pdf.stylesheets << Rails.root.join("app/views/print_pdf/#{style_name}.css").to_s | ||
|
|
||
| send_data pdf.to_pdf, | ||
| filename: "#{output_file_name}.pdf", | ||
| # disposition: "inline", # ダウンロードせず表示する | ||
| type: 'application/pdf' | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これそうだねー
with_header_footerに切り替えるなら既存のprint_pdfを削除しちゃってもいいと思うんだけどprint_pdfを使う時ってある?
| <% else %> | ||
| <% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コントローラー側でgroup.exists()で存在確認をしているからこのerbでgroupあるかどうかのifチェックは不要になる!
のでこのifとめっちゃしたにあるend消してネスト減らそう!!!
| <h3><%= fp.name %></h3> | ||
| <div class="detail-area"> | ||
| <% if cpo %> | ||
| <%= cpo.tent.present? ? simple_format(cpo.tent) : '調理行程が未登録です。' %><br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これは直して
| <%= fes_date.date %><br> | ||
| </div> | ||
| <% end %> | ||
| <% else %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
71行目の <% if group.food_products.any? %>かな?
これならendで終わってもよさそう
| <% student_id = (group.user.user_detail&.student_id || 0).to_i %> | ||
| <% if student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> | ||
| </td> | ||
| </tr> | ||
| <% if group.sub_rep %> | ||
| <tr> | ||
| <td class="No">2</td> | ||
| <td class="Belonging"><%= group.name %></td> | ||
| <td class="Name"><%= group.sub_rep.name %></td> | ||
| <td class="Category"> | ||
| <% sub_rep_student_id = (group.sub_rep&.student_id || 0).to_i %> | ||
| <% if sub_rep_student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif sub_rep_student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> | ||
| </td> | ||
| <td> | ||
| <% @fes_dates.select { |fes_date| fes_date.fes_year_id == group.fes_year_id && fes_date.days_num == 1 }.each do |fes_date| %> | ||
| <%= fes_date.date %><br> | ||
| <% end %> | ||
| <% @fes_dates.select { |fes_date| fes_date.fes_year_id == group.fes_year_id && fes_date.days_num == 2 }.each do |fes_date| %> | ||
| <%= fes_date.date %><br> | ||
| </tr> | ||
| <% end %> | ||
| <% group.employees.each_with_index do |employee, index| %> | ||
| <tr> | ||
| <td class="No"><%= group.sub_rep ? index +3 : index +2 %></td> | ||
| <td class="Belonging"><%=group.name %></td> | ||
| <td class="Name"><%=employee.name %></td> | ||
| <td class="Category"> | ||
| <% employee_student_id = (employee&.student_id || 0).to_i %> | ||
| <% if employee_student_id == 88888888 %> | ||
| 学外 | ||
| <% elsif employee_student_id == 99999999 %> | ||
| 教職員 | ||
| <% else %> | ||
| 学生 | ||
| <% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これできそう?マジックナンバーは対応しておきたい。適当にcursorに投げたの載せとく。
user_details.rb
# 学籍番号の特殊値(マジックナンバー)
STUDENT_ID_EXTERNAL = 88888888 # 学外
STUDENT_ID_STAFF = 99999999 # 教職員
※新規ファイル作ってる。別にどこかに書けばいいと思う。
application_helper.rb
# frozen_string_literal: true
module ApplicationHelper
# student_idからユーザータイプ(学外/教職員/学生)を判定して返す
def user_category_label(student_id)
return '学外' if student_id == UserDetail::STUDENT_ID_EXTERNAL
return '教職員' if student_id == UserDetail::STUDENT_ID_STAFF
'学生'
end
end
そしたらerbの中身これだけで良くなるらしい
<%= user_category_label((group.user.user_detail&.student_id || 0).to_i) %>
| <% else %> | ||
| <% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これもendで良さそう
対応Issue
resolve #1991
概要
実装詳細
画面スクリーンショット等
テスト項目
備考