fix: service以下の状態名変更#1052
Open
Hueter57 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughこのプルリクエストは、アプリケーション申請のステータス体系を刷新し、ステータス遷移の権限制御ロジックを再定義する変更と、ファイル処理で使用される MIME タイプ許可リストを公開化する変更を含みます。 Changesステータス体系と遷移制御の更新
MIME タイプ許可リストの公開化
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Jomon/internal/service/application.go Lines 263 to 292 in 94756a0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/application.go (1)
276-282: 💤 Low value可読性向上のため括弧の追加を検討してください。
Goの演算子優先順位（
&&>||）により論理的には正しいですが、複数の条件が連なっているため一目で理解しにくいです。♻️ 括弧を追加した例
func (s *Service) isAbleAccountManagerUpdateStatus(currentStatus, newStatus Status) bool { - return newStatus == Rejected && currentStatus == PendingReview || - newStatus == PendingReview && currentStatus == ChangeRequested || - newStatus == Approved && currentStatus == PendingReview || - newStatus == PendingReview && currentStatus == Approved || - newStatus == ChangeRequested && currentStatus == PendingReview + return (newStatus == Rejected && currentStatus == PendingReview) || + (newStatus == PendingReview && currentStatus == ChangeRequested) || + (newStatus == Approved && currentStatus == PendingReview) || + (newStatus == PendingReview && currentStatus == Approved) || + (newStatus == ChangeRequested && currentStatus == PendingReview) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/application.go` around lines 276 - 282, The boolean expression in isAbleAccountManagerUpdateStatus is correct but hard to read due to mixed && and ||; wrap each binary comparison pair in parentheses to clarify grouping (e.g., (newStatus == Rejected && currentStatus == PendingReview), (newStatus == PendingReview && currentStatus == ChangeRequested), etc.), keeping the same logic and using the existing Status constants Rejected, PendingReview, ChangeRequested, Approved so the intent is explicit and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/file.go`:
- Around line 101-102: The WriteFile flow is doing a strict equals check against
ApprovedMimeTypes using the raw Content-Type header, which fails when parameters
(e.g. "image/jpeg; charset=binary") are present; modify WriteFile to call
mime.ParseMediaType on the incoming mimetype (from echo.HeaderContentType),
extract the mediaType, normalize it to lowercase, and then check
ApprovedMimeTypes[mediaType]; ensure you handle ParseMediaType errors (treat as
unsupported) and keep the ApprovedMimeTypes lookup using the normalized media
type.
---
Nitpick comments:
In `@internal/service/application.go`:
- Around line 276-282: The boolean expression in
isAbleAccountManagerUpdateStatus is correct but hard to read due to mixed && and
||; wrap each binary comparison pair in parentheses to clarify grouping (e.g.,
(newStatus == Rejected && currentStatus == PendingReview), (newStatus ==
PendingReview && currentStatus == ChangeRequested), etc.), keeping the same
logic and using the existing Status constants Rejected, PendingReview,
ChangeRequested, Approved so the intent is explicit and easier to maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dec30a9-c637-4c91-a912-8472f0c45243
📒 Files selected for processing (3)
internal/service/application.gointernal/service/application_status.gointernal/service/file.go
Comment on lines
+101
to
102
| if !ApprovedMimeTypes[mimetype] { | ||
| return nil, NewBadInputError("unsupported mime type") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 生ヘッダー受け渡しと ParseMediaType 未使用の確認
rg -n -C2 'Header\.Get\(echo\.HeaderContentType\)'
rg -n -C2 '\bWriteFile\s*\(.*mimetype' --type=go
rg -n -C2 '\bParseMediaType\s*\(' --type=goRepository: traPtitech/Jomon
Length of output: 608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/service/file.go (around 80-130) ==="
nl -ba internal/service/file.go | sed -n '70,140p'
echo
echo "=== Find ApprovedMimeTypes / IsApprovedMimeType ==="
rg -n "ApprovedMimeTypes|IsApprovedMimeType" --type=go .
echo
echo "=== internal/router/file.go (around 40-100) ==="
nl -ba internal/router/file.go | sed -n '40,110p'
echo
echo "=== Ensure ParseMediaType usage absence (more broad) ==="
rg -n "ParseMediaType" --type=go .
echo
echo "=== Any existing mime parsing/normalization helpers ==="
rg -n "Parse.*MediaType|ToLower\\(mediaType\\)|strings\\.ToLower\\(.*mim" --type=go internal || trueRepository: traPtitech/Jomon
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/service/file.go (lines ~80-130) ==="
sed -n '70,140p' internal/service/file.go | cat -n
echo
echo "=== Find ApprovedMimeTypes / IsApprovedMimeType ==="
rg -n "ApprovedMimeTypes|IsApprovedMimeType" --type=go .
echo
echo
echo "=== internal/router/file.go (lines ~40-100) ==="
sed -n '40,110p' internal/router/file.go | cat -n
echo
echo
echo "=== ParseMediaType usage (anywhere) ==="
rg -n "ParseMediaType" --type=go .Repository: traPtitech/Jomon
Length of output: 5033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/service/file.go (lines ~1-70) ==="
sed -n '1,80p' internal/service/file.go | cat -nRepository: traPtitech/Jomon
Length of output: 2855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/service/file.go (lines ~1-80) ==="
sed -n '1,80p' internal/service/file.go | cat -nRepository: traPtitech/Jomon
Length of output: 2855
Content-Typeを生値で完全一致判定しており、パラメータ付きで誤拒否する可能性があります。
internal/router/file.go:60 で echo.HeaderContentType をそのまま WriteFile に渡し、internal/service/file.go（WriteFile）で ApprovedMimeTypes[mimetype] の完全一致で判定しているため、image/jpeg; charset=binary のような値が来ると一致せず unsupported mime type になります。mime.ParseMediaType でパラメータを除いたメディアタイプ部分に正規化（小文字化）してから ApprovedMimeTypes[mediaType] を判定してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/file.go` around lines 101 - 102, The WriteFile flow is doing
a strict equals check against ApprovedMimeTypes using the raw Content-Type
header, which fails when parameters (e.g. "image/jpeg; charset=binary") are
present; modify WriteFile to call mime.ParseMediaType on the incoming mimetype
(from echo.HeaderContentType), extract the mediaType, normalize it to lowercase,
and then check ApprovedMimeTypes[mediaType]; ensure you handle ParseMediaType
errors (treat as unsupported) and keep the ApprovedMimeTypes lookup using the
normalized media type.
Member
|
ありがとうございます 実はrouter以下にも同様のstatusがあって、こいつといい感じに同期をとる必要があります Jomon/internal/router/application.go Lines 18 to 26 in 76c53b0 とりあえず、ここもservice以下の変更と同様にrenameしてあげてください。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#904
servise内の状態名のみ変更
現状行ったのは状態名の変更のみ
状態遷移の修正に関して、現状の遷移図がわからない・現状のプログラムの意図が読むだけではわかりにくかったためまだ行っていないが、状態遷移を行っている場所は見つけた
Summary by CodeRabbit
リリースノート
New Features
Refactor