Certificates for community service#791
Conversation
…ctor's name on the document
📝 WalkthroughWalkthroughAdds LaTeX-based PDF certificate generation for community service entries, consolidates sanitization and PDF rendering into a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Controller as CommunityServiceController
participant Policy as CommunityServicePolicy
participant Model as CommunityService
participant Helper as LatexHelper
participant Storage as LaTeX_Storage
participant PDFEngine as Commands::latexToPdf
User->>Browser: Click "Generate Certificate"
Browser->>Controller: POST /community_service/generate_certificate/{id}
Controller->>Policy: authorize(generateCertificate, service)
Policy->>Model: verify service.approved && user is approver
alt allowed
Controller->>Model: load service, requester, approver
Controller->>Controller: determine approvingRole/approver_name/title
Controller->>Helper: generatePDF(view, data)
Helper->>Helper: sanitize data & render .tex
Helper->>Storage: write .tex file
Helper->>PDFEngine: latexToPdf(.tex, outputDir)
PDFEngine-->>Helper: .pdf path
Helper-->>Controller: return file path
Controller-->>Browser: file download response
else denied
Controller-->>Browser: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
app/Models/CommunityService.php (1)
39-39:date_of_servicecorrectly added to$fillable.One small housekeeping note: the PHPDoc block at the top of the class is missing a corresponding
@property string|null $date_of_serviceannotation, which would keep the IDE and static analysis tooling in sync.📝 Suggested docblock addition
* `@property` string $description + * `@property` string|null $date_of_service * `@property` int $semester_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/CommunityService.php` at line 39, Add a PHPDoc property annotation for the new date_of_service attribute in the CommunityService class docblock: update the top-of-class PHPDoc to include "@property string|null $date_of_service" so IDEs and static analyzers recognize the field alongside the existing $fillable entries and other `@property` annotations in CommunityService.app/Utils/LatexHelper.php (3)
36-36: Useself::rather thanLatexHelper::for the internal static call — this also resolves the PHPMD false positive.Calling a private static method via the class name (
LatexHelper::convertCharToSymbol) instead ofself::is what causes PHPMD to report it as "unused". Usingself::is also the idiomatic convention for intra-class static calls in PHP.♻️ Proposed change
- $result[] = LatexHelper::convertCharToSymbol(mb_substr($data, $i, 1)); + $result[] = self::convertCharToSymbol(mb_substr($data, $i, 1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` at line 36, Replace the internal static call to LatexHelper::convertCharToSymbol with a self:: call to avoid the PHPMD "unused" false positive and follow PHP intra-class conventions; locate the call site where LatexHelper::convertCharToSymbol(...) is invoked (inside the same class, where mb_substr($data, $i, 1) is passed) and change it to self::convertCharToSymbol(...), leaving the target private static method convertCharToSymbol unchanged.
30-30: Loose null comparison== nullfor a typed parameter.
"" == nullevaluates totruein PHP, sosanitizeLatex("")returns""via the early-return path rather than the (identical) loop path. The result is the same, but using=== nullis more precise for astring|nullparameter and avoids relying on PHP's loose-comparison rules.♻️ Proposed change
- if ($data == null) { + if ($data === null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` at line 30, Change the loose null comparison in the sanitizeLatex method to a strict check: replace the `if ($data == null)` early-return with a strict `=== null` check because the parameter is typed as `string|null` and `"" == null` can be true; update the conditional in the sanitizeLatex function (and any similar checks in class LatexHelper) to use `=== null` to precisely detect null values.
54-54: Filename collision risk —rand(0, 100000)provides only ~100k distinct values.Under concurrent load, two simultaneous certificate requests can produce the same
md5(rand(0, 100000) . date('c'))value, causing one process to overwrite the other's.tex/Str::uuid()oruniqid('', true)provide collision-free identifiers with no extra cost.♻️ Proposed fix
+ use Illuminate\Support\Str; // ... - $filename = md5(rand(0, 100000) . date('c')); + $filename = Str::uuid()->toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` at line 54, The current $filename generation in LatexHelper uses md5(rand(0, 100000) . date('c')) which risks collisions; replace that with a collision-resistant identifier (e.g., Str::uuid() or uniqid('', true)) when setting $filename in the LatexHelper method that writes/compiles .tex/.pdf files; if using Str::uuid(), import Illuminate\Support\Str at the top of the file and assign $filename to the UUID string, otherwise use uniqid('', true) to ensure unique filenames under concurrent load.app/Policies/CommunityServicePolicy.php (1)
63-66:generateCertificate— missing docblock, loose comparison, and absent return type.Three minor points worth tidying up:
- Loose comparison:
1 == $communityService->approvedworks coincidentally becausetrue == 1istruein PHP, but the field is already cast tobooleanby the model;=== trueis both clearer and more defensive.- Missing return type: every other method in this policy has a declared return type.
- Missing docblock: all other policy methods carry a
@param/ description block.♻️ Proposed tidy-up
+ /** + * Determine whether the user can generate a certificate for the given community service + * (the service must be approved and the user must be its designated approver). + * `@param` User $user + * `@param` CommunityService $communityService + * `@return` bool + */ public function generateCertificate(User $user, CommunityService $communityService) { - return 1 == $communityService->approved && $communityService->approver->id === $user->id; + return $communityService->approved === true && $communityService->approver->id === $user->id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Policies/CommunityServicePolicy.php` around lines 63 - 66, Update the generateCertificate method to match the other policy methods: add a docblock describing parameters (`@param` User $user, `@param` CommunityService $communityService) and purpose, change the approval check from loose equality to a strict boolean check (use $communityService->approved === true), ensure the approver check remains ($communityService->approver->id === $user->id), and declare a bool return type on the method signature (public function generateCertificate(User $user, CommunityService $communityService): bool).app/Models/Role.php (1)
282-288: LGTM — consistent with existing role accessor pattern.The new
secretary()method is a faithful mirror ofdirector(),collegist(), et al. One minor observation: like its siblings, the declared return typeRoledoesn't account for thenullthatfirst()can return when the role doesn't exist in the database. Whilst this is a pre-existing pattern across all similar accessors in this file, it would be worth addressing holistically at some point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Role.php` around lines 282 - 288, The accessor methods like secretary() currently declare a non-null return type Role but call first() which may return null; update secretary() to allow nullable returns (use ?Role or Role|null) and apply the same change to the sibling accessor methods such as director() and collegist() so the signatures match the possible null result from first(); keep the method bodies unchanged.app/Http/Controllers/StudentsCouncil/CommunityServiceController.php (1)
116-147: The downloaded file lacks a meaningful filename.
response()->download($documentPath)will serve the file with the random hash-based filename generated byLatexHelper. It would be rather more polished to provide a human-readable filename as the second argument — e.g. including the requester's name and the word "certificate".✏️ Suggested improvement
- return response()->download($documentPath); + $downloadName = 'community_service_certificate_' . $requester->id . '.pdf'; + return response()->download($documentPath, $downloadName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 116 - 147, The downloaded PDF uses the temp path from LatexHelper::generatePDF so response()->download($documentPath) gives a random filename; update generateCertificate to build a meaningful filename (e.g. combine $requester->name, "certificate" and the date, sanitize/remove spaces/special chars and append the .pdf extension) and pass it as the second argument to response()->download($documentPath, $filename) so the browser prompts with the human-readable name; ensure you derive the extension from $documentPath or force ".pdf" and provide a safe fallback filename if $requester->name is missing.resources/views/latex/community-service.blade.php (1)
86-89: Hardcoded contact email addresses in the footer.Lines 86–87 contain literal email addresses (
titkarsag@eotvos.elte.hu,horvathl@eotvos.elte.hu,elnok@eotvos.elte.hu). Thestatus-cert.blade.phptemplate (line 56) at least dynamically resolves the director's email via\App\Models\User::director()?->email. This is a pre-existing pattern elsewhere, so it is not strictly new to this PR, but worth noting for future alignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/latex/community-service.blade.php` around lines 86 - 89, The footer currently contains hardcoded emails in community-service.blade.php; replace them with dynamic lookups like the pattern used in status-cert.blade.php (e.g. \App\Models\User::director()?->email) so addresses come from the User model or a single source of truth; locate the email string in community-service.blade.php and substitute calls to the appropriate User accessor or a role-based query (e.g. director/secretary) or configuration value so the template renders the current users' emails instead of literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 136-145: The PDF data array accesses
$requester->educationalInformation->neptun without a null check; update the code
that builds the array (the block setting 'requester_neptun') to guard against
missing educational information by checking $requester->educationalInformation
or using $requester->hasEducationalInformation() and provide a safe fallback
(e.g., empty string) for requester_neptun and any other educational fields
before calling the PDF generation in CommunityServiceController (the code
building the array with 'requester_name', 'requester_neptun', etc.).
- Around line 125-132: The code accesses $approvingRole->name without guarding
for a null return from approvingRole(), causing a potential null dereference;
update the logic in the CommunityServiceController method that uses
approvingRole() so it first checks whether $approvingRole is non-null before
comparing to Role::SECRETARY (e.g., if ($approvingRole && $approvingRole->name
=== Role::SECRETARY) { ... }) and only then call User::director() and
Role::director() to swap $approver/$approvingRole; ensure any subsequent use of
$approvingRole assumes a non-null value or is similarly guarded.
In `@app/Utils/LatexHelper.php`:
- Around line 54-69: generatePDF is leaving temporary .tex and .pdf files in the
'latex' storage; remove them after use to avoid disk bloat. After calling
Commands::latexToPdf($pathTex, $outputDir) and after the caller has consumed the
file (i.e., after selecting return value in the generatePDF flow), delete both
$filename . '.tex' and $filename . '.pdf' via
Storage::disk('latex')->delete(...) (or schedule deletion for the debug branch),
or alternatively register a background/shutdown task to unlink
$pathTex/$pathPdf; reference Commands::latexToPdf, $pathTex, $pathPdf and the
debug branch (config('app.debug') && !config('commands.run_in_debug')) when
adding the cleanup logic.
- Around line 50-70: The generatePDF method currently ignores the result of
Commands::latexToPdf and always returns $pathPdf even if compilation failed;
update generatePDF to capture the return value from
Commands::latexToPdf($pathTex, $outputDir), verify it indicates success and/or
that the PDF file at $pathPdf actually exists, and if compilation failed throw
or return a descriptive error (including the captured command output or
exception) or fall back to returning $pathTex in debug mode; ensure you
reference the generatePDF function, the Commands::latexToPdf call, and the
$pathPdf/$pathTex variables when adding the check and error handling.
- Around line 15-38: convertCharToSymbol and sanitizeLatex currently map every
character to \symbol{mb_ord($char)}, but mb_ord returns Unicode code points >255
(e.g., ő → 337) which breaks pdfLaTeX; fix by switching to a Unicode-capable
engine or by replacing the \symbol(...) strategy with proper LaTeX-safe output:
either update the build/CI to use lualatex or xelatex (so UTF-8 characters can
be passed through unchanged) or implement a mapping in convertCharToSymbol that
converts known accented characters to LaTeX accent commands (e.g., ő -> \H{o}, ű
-> \H{u}) and fall back to ascii or numeric-escaped forms for others instead of
\symbol with mb_ord. Ensure changes are applied in convertCharToSymbol and
sanitizeLatex so sanitizeLatex no longer emits invalid \symbol{N} values.
In `@resources/views/latex/community-service.blade.php`:
- Around line 73-74: Replace the hardcoded director name "Dr. Horváth László" in
community-service.blade.php with the same dynamic lookup used elsewhere: call
\App\Models\User::director()?->name (and preserve any title formatting e.g.
"Dr." if needed) so the template displays the current director like
status-cert.blade.php does; update the line that currently contains the literal
name to use that dynamic expression and ensure it still falls back gracefully if
null.
- Line 3: Replace the fragile relative LaTeX resource path defined by the
\resourcepath command with the same absolute Laravel storage helper used in
status-cert.blade.php: remove the \newcommand{\resourcepath}{../public/} line
and update image/include references in community-service.blade.php to use
storage_path('/app/public/<filename>') (e.g.
storage_path('/app/public/epulet.png')) so paths are absolute and consistent
with status-cert.blade.php.
In `@resources/views/student-council/community-service/request.blade.php`:
- Line 14: User::secretary() can return null which will be injected into the
elements array passed to the x-input.select component causing a fatal render
error; update the elements construction so any null/falsey values are filtered
out before merging (e.g., filter the array containing User::secretary() and the
result of User::studentCouncilLeaders()) so x-input.select only receives valid
User objects; reference User::secretary(), User::studentCouncilLeaders(), and
the x-input.select id="approver" elements prop when making the change.
---
Nitpick comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 116-147: The downloaded PDF uses the temp path from
LatexHelper::generatePDF so response()->download($documentPath) gives a random
filename; update generateCertificate to build a meaningful filename (e.g.
combine $requester->name, "certificate" and the date, sanitize/remove
spaces/special chars and append the .pdf extension) and pass it as the second
argument to response()->download($documentPath, $filename) so the browser
prompts with the human-readable name; ensure you derive the extension from
$documentPath or force ".pdf" and provide a safe fallback filename if
$requester->name is missing.
In `@app/Models/CommunityService.php`:
- Line 39: Add a PHPDoc property annotation for the new date_of_service
attribute in the CommunityService class docblock: update the top-of-class PHPDoc
to include "@property string|null $date_of_service" so IDEs and static analyzers
recognize the field alongside the existing $fillable entries and other `@property`
annotations in CommunityService.
In `@app/Models/Role.php`:
- Around line 282-288: The accessor methods like secretary() currently declare a
non-null return type Role but call first() which may return null; update
secretary() to allow nullable returns (use ?Role or Role|null) and apply the
same change to the sibling accessor methods such as director() and collegist()
so the signatures match the possible null result from first(); keep the method
bodies unchanged.
In `@app/Policies/CommunityServicePolicy.php`:
- Around line 63-66: Update the generateCertificate method to match the other
policy methods: add a docblock describing parameters (`@param` User $user, `@param`
CommunityService $communityService) and purpose, change the approval check from
loose equality to a strict boolean check (use $communityService->approved ===
true), ensure the approver check remains ($communityService->approver->id ===
$user->id), and declare a bool return type on the method signature (public
function generateCertificate(User $user, CommunityService $communityService):
bool).
In `@app/Utils/LatexHelper.php`:
- Line 36: Replace the internal static call to LatexHelper::convertCharToSymbol
with a self:: call to avoid the PHPMD "unused" false positive and follow PHP
intra-class conventions; locate the call site where
LatexHelper::convertCharToSymbol(...) is invoked (inside the same class, where
mb_substr($data, $i, 1) is passed) and change it to
self::convertCharToSymbol(...), leaving the target private static method
convertCharToSymbol unchanged.
- Line 30: Change the loose null comparison in the sanitizeLatex method to a
strict check: replace the `if ($data == null)` early-return with a strict `===
null` check because the parameter is typed as `string|null` and `"" == null` can
be true; update the conditional in the sanitizeLatex function (and any similar
checks in class LatexHelper) to use `=== null` to precisely detect null values.
- Line 54: The current $filename generation in LatexHelper uses md5(rand(0,
100000) . date('c')) which risks collisions; replace that with a
collision-resistant identifier (e.g., Str::uuid() or uniqid('', true)) when
setting $filename in the LatexHelper method that writes/compiles .tex/.pdf
files; if using Str::uuid(), import Illuminate\Support\Str at the top of the
file and assign $filename to the UUID string, otherwise use uniqid('', true) to
ensure unique filenames under concurrent load.
In `@resources/views/latex/community-service.blade.php`:
- Around line 86-89: The footer currently contains hardcoded emails in
community-service.blade.php; replace them with dynamic lookups like the pattern
used in status-cert.blade.php (e.g. \App\Models\User::director()?->email) so
addresses come from the User model or a single source of truth; locate the email
string in community-service.blade.php and substitute calls to the appropriate
User accessor or a role-based query (e.g. director/secretary) or configuration
value so the template renders the current users' emails instead of literals.
| [ | ||
| 'requester_name' => $requester->name, | ||
| 'requester_neptun' => $requester->educationalInformation->neptun, | ||
| 'approver_name' => $approver->name, | ||
| 'approver_title' => is_null($approvingRole) ? "" : $approvingRole->translatedName, | ||
| 'service_description' => $communityService->description, | ||
| 'date_of_service' => $communityService->date_of_service, | ||
| 'current_date' => date("Y.m.d.") | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Potential null pointer dereference on educationalInformation.
Line 138 accesses $requester->educationalInformation->neptun without verifying that educationalInformation is not null. If the requester lacks educational information, this will throw a fatal error. The DocumentController::generateStatusCertificate method (line 285–290 in DocumentController.php) guards against this with hasEducationalInformation() — this method would benefit from a similar check.
🛡️ Proposed fix — add a guard before PDF generation
$requester = $communityService->requester;
+ if (!$requester->hasEducationalInformation()) {
+ return back()->with('error', 'A kérvényező tanulmányi adatai hiányoznak.');
+ }
$approver = Auth::user();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around
lines 136 - 145, The PDF data array accesses
$requester->educationalInformation->neptun without a null check; update the code
that builds the array (the block setting 'requester_neptun') to guard against
missing educational information by checking $requester->educationalInformation
or using $requester->hasEducationalInformation() and provide a safe fallback
(e.g., empty string) for requester_neptun and any other educational fields
before calling the PDF generation in CommunityServiceController (the code
building the array with 'requester_name', 'requester_neptun', etc.).
| private static function convertCharToSymbol(string $char): string | ||
| { | ||
| if ($char == ' ') { | ||
| return ' '; | ||
| } | ||
| return "\\symbol{" . mb_ord($char) . "}"; | ||
| } | ||
|
|
||
| /** | ||
| * Converts string to relatively safe LaTeX code by putting every character into a seperate \symbol{...} | ||
| * @param string $data | ||
| * @return string | ||
| */ | ||
| public static function sanitizeLatex(string|null $data): string | ||
| { | ||
| if ($data == null) { | ||
| return ""; | ||
| } | ||
| $len = mb_strlen($data); | ||
| $result = []; | ||
| for ($i = 0; $i < $len; $i++) { | ||
| $result[] = LatexHelper::convertCharToSymbol(mb_substr($data, $i, 1)); | ||
| } | ||
| return implode($result); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the LatexHelper.php file
fd 'LatexHelper.php' --type fRepository: EotvosCollegium/mars
Length of output: 89
🏁 Script executed:
# Search for LaTeX engine configuration in the codebase
rg -n 'pdflatex|lualatex|xelatex' --type=phpRepository: EotvosCollegium/mars
Length of output: 302
🏁 Script executed:
# Search for the Commands class and latexToPdf method
rg -n 'latexToPdf|class Commands' --type=phpRepository: EotvosCollegium/mars
Length of output: 272
🏁 Script executed:
# Look for certificate template files
fd 'community-service|certificate' --type f | grep -E '\.(blade\.php|php)$'Repository: EotvosCollegium/mars
Length of output: 210
🏁 Script executed:
# Search for \symbol usage in templates and views
rg -n '\\\\symbol' --type=phpRepository: EotvosCollegium/mars
Length of output: 142
🏁 Script executed:
# Read the full LatexHelper.php file
cat -n app/Utils/LatexHelper.phpRepository: EotvosCollegium/mars
Length of output: 2569
🏁 Script executed:
# Read the Commands class with focus on latexToPdf
cat -n app/Console/Commands.phpRepository: EotvosCollegium/mars
Length of output: 1327
🏁 Script executed:
# Read the certificate template
cat -n resources/views/latex/community-service.blade.phpRepository: EotvosCollegium/mars
Length of output: 4685
🌐 Web query:
\symbol{} pdflatex valid range 0-255 character code
💡 Result:
In pdfLaTeX (pdfTeX), \symbol{<number>} accesses a glyph by 8‑bit font position, so the valid character code range is 0–255; if you try a larger value (e.g., a Unicode code point like U+20AE = 8366) you’ll get the classic error “character number must be between 0 and 255.” (tug.ctan.org)
If you need code points beyond 255, use a Unicode engine (XeLaTeX/LuaLaTeX), where you can input Unicode scalar values (e.g. \symbol{"3042} / \char"3042). (tex.stackexchange.com)
Citations:
- 1: https://tug.ctan.org/info/latex2e-help-texinfo/latex2e.html
- 2: https://tex.stackexchange.com/questions/615187/unicode-in-pdflatex-says-it-only-supports-up-to-255-code-points-when-im-trying?utm_source=openai
🌐 Web query:
Hungarian character ő unicode code point
💡 Result:
The Hungarian letter ő (Latin small letter o with double acute) has Unicode code point U+0151 (decimal 337). [1][2]
🌐 Web query:
pdflatex \symbol font encoding T1 valid codes
💡 Result:
In pdfLaTeX (8‑bit fonts), \symbol{<n>} (like \char) accepts a character code in the range 0–255; larger values are invalid (“character number must be between 0 and 255”). [1]
With font encoding T1 active (\usepackage[T1]{fontenc}), those numbers index the 256 slots of the Cork/T1 encoding (i.e., the glyph position in the currently selected T1-encoded font). [2]
Key points for “valid codes” in T1:
- Syntactically valid: any integer 0–255 (decimal), also expressible in octal/hex TeX notation. [1]
- Semantically valid (produces the glyph you expect): only the codes that your specific T1 font actually contains for that slot;
T1is a mix and is not meant to be typed as raw codes. [3] - The authoritative mapping from LaTeX accent/letter commands to T1 slots is in
t1enc.def(e.g., it maps\'ato slot 225 in T1). [4]
If what you want is “which number gives which character in T1”, use the Cork/T1 encoding table (and/or inspect t1enc.def), because \symbol just addresses slots—it doesn’t define the table by itself. [2][4]
Sources: [1] Google Groups thread showing TeX’s 0–255 limit for \char-style primitives https://groups.google.com/g/linux.debian.user/c/VymnFqV6B3Y, [2] Cork/T1 is a 256-character encoding https://en.wikipedia.org/wiki/Cork_encoding, [3] TeX FAQ note that T1 code points are a non-keyboard mix and fontenc generates them from LaTeX commands https://davidcarlisle.github.io/uk-tex-faq/FAQ-t1enc.html, [4] Example from t1enc.def mapping \'a → 225 (T1 slot) https://tex.stackexchange.com/questions/276145/what-does-fontenc-do-and-when-is-it-applied-based-on-an-example-of-creating-my-package
\symbol{N} with pdfLatex fails fatally for Hungarian characters — mb_ord() returns Unicode code points exceeding the 0–255 limit.
\symbol{N} in pdfLatex addresses font glyphs by 8-bit slot number (0–255). The mb_ord() function, however, returns a Unicode scalar value. For the Hungarian character ő, this is U+0151 (decimal 337), which exceeds 255 and triggers a fatal compilation error: "character number must be between 0 and 255."
The template calls LatexHelper::sanitizeLatex() on all user input—names, descriptions, and dates—and the certificate template is configured for Hungarian language (\usepackage[magyar]{babel}). Any user input containing ő, ű, or other accented characters with code points exceeding 255 will cause compilation failure in production.
The sustainable remedy is to migrate from pdflatex to a Unicode-aware engine such as lualatex or xelatex, which handle UTF-8 encoded Unicode natively. Alternatively, implement proper LaTeX accent commands (e.g., ő → \H{o}, ű → \H{u}) in place of the current \symbol{} approach.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 15-21: Avoid unused private methods such as 'convertCharToSymbol'. (undefined)
(UnusedPrivateMethod)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Utils/LatexHelper.php` around lines 15 - 38, convertCharToSymbol and
sanitizeLatex currently map every character to \symbol{mb_ord($char)}, but
mb_ord returns Unicode code points >255 (e.g., ő → 337) which breaks pdfLaTeX;
fix by switching to a Unicode-capable engine or by replacing the \symbol(...)
strategy with proper LaTeX-safe output: either update the build/CI to use
lualatex or xelatex (so UTF-8 characters can be passed through unchanged) or
implement a mapping in convertCharToSymbol that converts known accented
characters to LaTeX accent commands (e.g., ő -> \H{o}, ű -> \H{u}) and fall back
to ascii or numeric-escaped forms for others instead of \symbol with mb_ord.
Ensure changes are applied in convertCharToSymbol and sanitizeLatex so
sanitizeLatex no longer emits invalid \symbol{N} values.
| public static function generatePDF($path, $data) | ||
| { | ||
| $renderedLatex = view($path)->with($data)->render(); | ||
|
|
||
| $filename = md5(rand(0, 100000) . date('c')); | ||
| Storage::disk('latex')->put($filename . '.tex', $renderedLatex); | ||
|
|
||
| $outputDir = Storage::disk('latex')->path('/'); | ||
|
|
||
| $pathTex = Storage::disk('latex')->path($filename . ".tex"); | ||
| $pathPdf = Storage::disk('latex')->path($filename . ".pdf"); | ||
|
|
||
| // TODO: figure out result | ||
| Commands::latexToPdf($pathTex, $outputDir); | ||
|
|
||
| if (config('app.debug') && !config('commands.run_in_debug')) { | ||
| return $pathTex; | ||
| } else { | ||
| return $pathPdf; | ||
| } | ||
| } |
There was a problem hiding this comment.
latexToPdf return value is silently discarded — compilation failures are invisible, and a non-existent PDF path is returned.
Line 63 calls Commands::latexToPdf(...) but never assigns or checks the result. The TODO: figure out result comment confirms this is known but unresolved. If pdflatex fails to compile (syntax error, missing package, or the \symbol overflow described above), the .pdf file will not be created, yet $pathPdf is returned unconditionally on line 68. The caller will then attempt to serve a non-existent file, resulting in a 500 error or a silent broken download.
🛡️ Proposed error-handling skeleton
- // TODO: figure out result
- Commands::latexToPdf($pathTex, $outputDir);
-
- if (config('app.debug') && !config('commands.run_in_debug')) {
- return $pathTex;
- } else {
- return $pathPdf;
- }
+ Commands::latexToPdf($pathTex, $outputDir);
+
+ if (config('app.debug') && !config('commands.run_in_debug')) {
+ return $pathTex;
+ }
+
+ if (!file_exists($pathPdf)) {
+ throw new \RuntimeException('LaTeX compilation failed; PDF was not produced. Check the .tex file at: ' . $pathTex);
+ }
+
+ return $pathPdf;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Utils/LatexHelper.php` around lines 50 - 70, The generatePDF method
currently ignores the result of Commands::latexToPdf and always returns $pathPdf
even if compilation failed; update generatePDF to capture the return value from
Commands::latexToPdf($pathTex, $outputDir), verify it indicates success and/or
that the PDF file at $pathPdf actually exists, and if compilation failed throw
or return a descriptive error (including the captured command output or
exception) or fall back to returning $pathTex in debug mode; ensure you
reference the generatePDF function, the Commands::latexToPdf call, and the
$pathPdf/$pathTex variables when adding the check and error handling.
| $filename = md5(rand(0, 100000) . date('c')); | ||
| Storage::disk('latex')->put($filename . '.tex', $renderedLatex); | ||
|
|
||
| $outputDir = Storage::disk('latex')->path('/'); | ||
|
|
||
| $pathTex = Storage::disk('latex')->path($filename . ".tex"); | ||
| $pathPdf = Storage::disk('latex')->path($filename . ".pdf"); | ||
|
|
||
| // TODO: figure out result | ||
| Commands::latexToPdf($pathTex, $outputDir); | ||
|
|
||
| if (config('app.debug') && !config('commands.run_in_debug')) { | ||
| return $pathTex; | ||
| } else { | ||
| return $pathPdf; | ||
| } |
There was a problem hiding this comment.
Temporary .tex and .pdf files are never cleaned up — disk space will accumulate indefinitely.
Every call to generatePDF creates two files in the latex storage disk and leaves them there permanently. On a production server with regular certificate generation activity, this will gradually exhaust available disk space with no automated remedy.
Consider deleting the files after the caller has consumed them (e.g., after the HTTP response is sent), or scheduling a periodic clean-up of old files in the storage directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Utils/LatexHelper.php` around lines 54 - 69, generatePDF is leaving
temporary .tex and .pdf files in the 'latex' storage; remove them after use to
avoid disk bloat. After calling Commands::latexToPdf($pathTex, $outputDir) and
after the caller has consumed the file (i.e., after selecting return value in
the generatePDF flow), delete both $filename . '.tex' and $filename . '.pdf' via
Storage::disk('latex')->delete(...) (or schedule deletion for the debug branch),
or alternatively register a background/shutdown task to unlink
$pathTex/$pathPdf; reference Commands::latexToPdf, $pathTex, $pathPdf and the
debug branch (config('app.debug') && !config('commands.run_in_debug')) when
adding the cleanup logic.
| @@ -0,0 +1,132 @@ | |||
| \documentclass[11pt,a4paper]{article} | |||
|
|
|||
| \newcommand{\resourcepath}{../public/} | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the blade template files mentioned in the review
find . -name "community-service.blade.php" -o -name "status-cert.blade.php" 2>/dev/null | head -20Repository: EotvosCollegium/mars
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Search for LatexHelper to understand working directory configuration
find . -name "*LatexHelper*" -o -name "*latex*helper*" 2>/dev/null | grep -i helper | head -20Repository: EotvosCollegium/mars
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Get a broader view of the project structure
ls -la | head -20Repository: EotvosCollegium/mars
Length of output: 1276
🏁 Script executed:
cat -n ./resources/views/latex/community-service.blade.phpRepository: EotvosCollegium/mars
Length of output: 4685
🏁 Script executed:
cat -n ./resources/views/latex/status-cert.blade.phpRepository: EotvosCollegium/mars
Length of output: 4278
🏁 Script executed:
cat -n ./app/Utils/LatexHelper.phpRepository: EotvosCollegium/mars
Length of output: 2569
🏁 Script executed:
find . -name "epulet.png" -o -name "szabadon_szolgal_a_szellem.png" 2>/dev/nullRepository: EotvosCollegium/mars
Length of output: 95
🏁 Script executed:
rg "epulet.png|szabadon_szolgal_a_szellem.png" -g "*.php" -g "*.blade.php" --max-count 10Repository: EotvosCollegium/mars
Length of output: 581
Resource path must use absolute Laravel helpers, consistent with status-cert.blade.php.
The relative resource path ../public/ is fragile, depending on LaTeX compilation working directory configuration. Moreover, it produces double slashes in resolved paths (e.g. ../public//epulet.png).
Whilst status-cert.blade.php (line 24) correctly uses an absolute path with storage_path('/app/public/epulet.png'), community-service.blade.php relies on a relative path. For consistency and robustness, adopt the same approach using storage_path():
Suggested fix
-\newcommand{\resourcepath}{../public/}
...
-\node[inner sep=0pt] (picture) at (0,2){\includegraphics[width=5cm]{\resourcepath/epulet.png}};
+\node[inner sep=0pt] (picture) at (0,2){\includegraphics[width=5cm]{ {{ storage_path('/app/public/epulet.png') }} }};
...
-\node[inner sep=0pt] (from) at (0,-0.4){\includegraphics[width=5cm]{\resourcepath/szabadon_szolgal_a_szellem.png}};
+\node[inner sep=0pt] (from) at (0,-0.4){\includegraphics[width=5cm]{ {{ storage_path('/app/public/szabadon_szolgal_a_szellem.png') }} }};Remove the \resourcepath command entirely once resolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/latex/community-service.blade.php` at line 3, Replace the
fragile relative LaTeX resource path defined by the \resourcepath command with
the same absolute Laravel storage helper used in status-cert.blade.php: remove
the \newcommand{\resourcepath}{../public/} line and update image/include
references in community-service.blade.php to use
storage_path('/app/public/<filename>') (e.g.
storage_path('/app/public/epulet.png')) so paths are absolute and consistent
with status-cert.blade.php.
| Dr. Horváth László\\ | ||
| igazgató\\ |
There was a problem hiding this comment.
Hardcoded director name will become stale.
The footer contains Dr. Horváth László as a literal string, whereas the status-cert.blade.php template (line 45, 79, 98) dynamically resolves the director's name via \App\Models\User::director()?->name. Should the director change, this template will silently display the wrong name.
One would respectfully recommend using the same dynamic approach here:
🐛 Proposed fix
- Dr. Horváth László\\
+ {{ \App\Utils\LatexHelper::sanitizeLatex(\App\Models\User::director()?->name) }}\\🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/latex/community-service.blade.php` around lines 73 - 74,
Replace the hardcoded director name "Dr. Horváth László" in
community-service.blade.php with the same dynamic lookup used elsewhere: call
\App\Models\User::director()?->name (and preserve any title formatting e.g.
"Dr." if needed) so the template displays the current director like
status-cert.blade.php does; update the line that currently contains the literal
name to use that dynamic expression and ensure it still falls back gracefully if
null.
| <x-input.button s=2 class="right" text="Mentés" /> | ||
| <x-input.text m=12 id="description" required text="Rendezvény, feladatkör, tevékenység" /> | ||
| <x-input.text m=6 id="date_of_service" text="Tevékenység dátuma" /> | ||
| <x-input.select m=4 id="approver" :elements="array_merge([\App\Models\User::secretary()], \App\Models\User::studentCouncilLeaders()->toArray())" text="jóváhagyó"/> |
There was a problem hiding this comment.
User::secretary() can return null, which will be injected into the elements array and likely cause a fatal render error.
User::secretary() is declared as ?User and returns null when no secretary is currently assigned. Passing that null as the first element of the array to x-input.select means the component will attempt to access properties on null, causing an unhandled error for any user who visits the form while the role is vacant.
🛡️ Proposed fix — filter out null before merging
-<x-input.select m=4 id="approver" :elements="array_merge([\App\Models\User::secretary()], \App\Models\User::studentCouncilLeaders()->toArray())" text="jóváhagyó"/>
+<x-input.select m=4 id="approver" :elements="array_merge(array_filter([\App\Models\User::secretary()]), \App\Models\User::studentCouncilLeaders()->toArray())" text="jóváhagyó"/>array_filter with no callback removes null (and other falsy values), so a vacant secretary slot is simply omitted from the list rather than causing an error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <x-input.select m=4 id="approver" :elements="array_merge([\App\Models\User::secretary()], \App\Models\User::studentCouncilLeaders()->toArray())" text="jóváhagyó"/> | |
| <x-input.select m=4 id="approver" :elements="array_merge(array_filter([\App\Models\User::secretary()]), \App\Models\User::studentCouncilLeaders()->toArray())" text="jóváhagyó"/> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/student-council/community-service/request.blade.php` at line
14, User::secretary() can return null which will be injected into the elements
array passed to the x-input.select component causing a fatal render error;
update the elements construction so any null/falsey values are filtered out
before merging (e.g., filter the array containing User::secretary() and the
result of User::studentCouncilLeaders()) so x-input.select only receives valid
User objects; reference User::secretary(), User::studentCouncilLeaders(), and
the x-input.select id="approver" elements prop when making the change.
|
Anitával átbeszéltétek a funkcionális követelményeket? Én ugyan nem tárgyaltam vele végig részletesen a kérdéseket, de valami olyasmi rémlik, hogy olyan módon kéri Anita a különböző dolgokat, hogy az adott esemény adatait ki kelljen az illetőnek töltenie, különböző fieldekben a rendezvény kitöltendő adatai. Illetve emlékeim szerint előadások tartására is ezt szeretnék, hogyha használva lenne, ami nem hiszem, hogy közösségi tevékenységnek számít. |
|
@horcsinbalint Igen, küldött pár mintát és annak ez megfelel:) de megkérdezhetem tőle még egyszer. A közösségi tevékenységből áttehetjük akár külön menüpontba; én inkább csak azért vontam össze, mert így tudom azt az infrastruktúrát használni és nem kell a kereket újrafeltalálni. P.S. bocsi, hogy csak most; nem küldött értesítést és nem vettem észre 😅 |
…nto certs-for-service
|
Now, I've moved the secretariat's part under the Documents page, so that it's clearly separated from community services. See the updated intro (it's terribly littered by AI comments 😅). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/StudentsCouncil/CommunityServiceController.php (1)
53-66:⚠️ Potential issue | 🔴 CriticalDo not trust the approver from the form.
approveris only checked for existence, so a crafted POST can nominate any user ID — including the requester themselves. Because bothapprove()andgenerateCertificate()only verifyapprover_id === user->id, that lets a user self-approve and then download their own certificate. Please validate the approver against a server-side allow-list of valid secretariat/student-council approvers and reject self-selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 53 - 66, The form currently trusts the submitted 'approver' and creates a CommunityService via CommunityService::create using $request->approver; instead, before creation validate that the chosen approver is in your server-side allow-list of authorized approvers (e.g. users with the student-council/secretariat role) and reject the request if not, and also reject if $request->approver == user()->id to prevent self-selection; update the controller code around the request->validate and CommunityService::create blocks to lookup the approver via the Users/Role query (or an AllowList service), enforce membership in that allowed set, and only then set 'approver_id' for the new CommunityService (also ensure approve() and generateCertificate() continue to rely on approver_id === user()->id and will thus be safe once approver_id is server-validated).
♻️ Duplicate comments (3)
app/Http/Controllers/StudentsCouncil/CommunityServiceController.php (2)
123-132:⚠️ Potential issue | 🔴 CriticalGuard
approvingRole()before dereferencing it.
approvingRole()explicitly returnsnull, and that can still happen later if an already-approved approver loses the relevant role. Line 125 will then fatal before the director fallback is reached.Possible fix
- if (Role::SECRETARY == $approvingRole->name) { + if ($approvingRole && $approvingRole->name === Role::SECRETARY) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 123 - 132, approvingRole() can return null and the code dereferences $approvingRole->name causing a fatal; guard the call in CommunityServiceController by checking for null before accessing ->name (e.g., if ($approvingRole !== null && Role::SECRETARY == $approvingRole->name) { ... }) so the director fallback still executes when $approvingRole is null, and ensure you update $approver and $approvingRole to User::director() and Role::director() only inside that guarded block.
121-139:⚠️ Potential issue | 🟠 MajorFail gracefully when the requester has no educational record.
Line 138 assumes
educationalInformationis present, but that relation is nullable elsewhere andDocumentController::generateStatusCertificate()already guards it. At present this path returns a 500 instead of a user-facing error.Possible fix
$requester = $communityService->requester; + if (!$requester->hasEducationalInformation()) { + return back()->with('error', 'A kérvényező tanulmányi adatai hiányoznak az igazolás generálásához.'); + } $approver = Auth::user();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 121 - 139, The code in CommunityServiceController reads $requester->educationalInformation->neptun inside the generatePDF call which can be null and causes a 500; update the method to check that $requester->educationalInformation exists before using ->neptun (e.g. if null, return a user-facing error/redirect or throw a handled exception), and pass a safe value to \App\Utils\LatexHelper::generatePDF only when present; reference the $requester variable, the educationalInformation relation, and the generatePDF call to locate and fix the check.resources/views/student-council/community-service/request.blade.php (1)
1-5:⚠️ Potential issue | 🟠 MajorDefault
$isForSecretariatand filter out an absent secretary.This partial is still included from
resources/views/secretariat/evaluation-form/community_service.blade.phpwithout that variable, so the new branch now raises an undefined-variable error there. Also, when no secretary is assigned,[User::secretary()]still injectsnullinto the select payload.Possible fix
`@php` + $isForSecretariat = $isForSecretariat ?? false; $approvers = $isForSecretariat - ? [\App\Models\User::secretary()] + ? array_values(array_filter([\App\Models\User::secretary()])) : \App\Models\User::studentCouncilLeaders()->toArray(); `@endphp`Also applies to: 10-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/student-council/community-service/request.blade.php` around lines 1 - 5, The partial uses $isForSecretariat and builds $approvers which can be undefined or contain nulls; ensure $isForSecretariat has a default (e.g. false) before the ternary and when building $approvers filter out any null entries from the result of User::secretary() or User::studentCouncilLeaders() so the select payload never contains null; update the Blade snippet that sets $isForSecretariat and $approvers (the variables in the shown `@php` block) to provide a default for $isForSecretariat and apply a null-filter to the chosen array/result.
🧹 Nitpick comments (2)
app/Http/Controllers/Secretariat/DocumentController.php (1)
26-32: Avoid loading certificate history on every documents-page hit.
resources/views/secretariat/document/index.blade.phponly renders the community-service table for secretaries, but this query now runs for every/documentsrequest. When you do need it, please eager-loadcommunityServices.requesterandcommunityServices.approveras well, otherwise the reused table partial fans out into N+1 queries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Secretariat/DocumentController.php` around lines 26 - 32, The Semester query in printingBladeData() is being executed on every /documents request; change it to run only when the current user will render the community-service table (e.g., check the user's secretary role/permission before running the query) and eager-load the nested relations to avoid N+1s by using with(...) for communityServices.requester and communityServices.approver (or by eager-loading communityServices with those relations inside the Semester query that currently uses withWhereHas('communityServices', ...)); keep the existing approver/requester filter logic but move it behind the role check so the query only executes when needed.app/Utils/LatexHelper.php (1)
55-55: Consider using a cryptographically stronger random source for filename generation.Whilst collision is improbable given that
date('c')provides per-second uniqueness,rand(0, 100000)offers rather limited entropy. Under concurrent load within the same second, a collision is theoretically possible.♻️ Proposed improvement using Laravel's Str::random()
+use Illuminate\Support\Str; + // ... - $filename = md5(rand(0, 100000) . date('c')); + $filename = Str::random(40);Alternatively, retain the timestamp for debugging visibility:
- $filename = md5(rand(0, 100000) . date('c')); + $filename = date('Ymd_His') . '_' . Str::random(16);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` at line 55, Replace the weak rand(0, 100000) entropy used to build $filename in LatexHelper.php with a cryptographically secure random source (e.g. use Laravel's Illuminate\Support\Str::random(...) or PHP's random_bytes/bin2hex) and keep the timestamp if you want human-readable uniqueness; update the $filename construction in the method that sets $filename to combine date('c') with a secure random string and add the appropriate import (Str) or error handling for random_bytes as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Role.php`:
- Around line 282-288: The methods (secretary(), collegist(), studentsCouncil(),
director(), sysAdmin()) declare a non-nullable Role return but use
self::where(...)->first() which can return null; replace each body to delegate
to self::get(CONSTANT) (e.g., return self::get(self::SECRETARY)) so the lookup
throws/handles missing roles and satisfies the non-nullable Role contract;
update each respective method to call self::get with its constant
(self::COLLEGIST, self::STUDENTS_COUNCIL, self::DIRECTOR, self::SYS_ADMIN)
instead of where(...)->first().
---
Outside diff comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 53-66: The form currently trusts the submitted 'approver' and
creates a CommunityService via CommunityService::create using
$request->approver; instead, before creation validate that the chosen approver
is in your server-side allow-list of authorized approvers (e.g. users with the
student-council/secretariat role) and reject the request if not, and also reject
if $request->approver == user()->id to prevent self-selection; update the
controller code around the request->validate and CommunityService::create blocks
to lookup the approver via the Users/Role query (or an AllowList service),
enforce membership in that allowed set, and only then set 'approver_id' for the
new CommunityService (also ensure approve() and generateCertificate() continue
to rely on approver_id === user()->id and will thus be safe once approver_id is
server-validated).
---
Duplicate comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 123-132: approvingRole() can return null and the code dereferences
$approvingRole->name causing a fatal; guard the call in
CommunityServiceController by checking for null before accessing ->name (e.g.,
if ($approvingRole !== null && Role::SECRETARY == $approvingRole->name) { ... })
so the director fallback still executes when $approvingRole is null, and ensure
you update $approver and $approvingRole to User::director() and Role::director()
only inside that guarded block.
- Around line 121-139: The code in CommunityServiceController reads
$requester->educationalInformation->neptun inside the generatePDF call which can
be null and causes a 500; update the method to check that
$requester->educationalInformation exists before using ->neptun (e.g. if null,
return a user-facing error/redirect or throw a handled exception), and pass a
safe value to \App\Utils\LatexHelper::generatePDF only when present; reference
the $requester variable, the educationalInformation relation, and the
generatePDF call to locate and fix the check.
In `@resources/views/student-council/community-service/request.blade.php`:
- Around line 1-5: The partial uses $isForSecretariat and builds $approvers
which can be undefined or contain nulls; ensure $isForSecretariat has a default
(e.g. false) before the ternary and when building $approvers filter out any null
entries from the result of User::secretary() or User::studentCouncilLeaders() so
the select payload never contains null; update the Blade snippet that sets
$isForSecretariat and $approvers (the variables in the shown `@php` block) to
provide a default for $isForSecretariat and apply a null-filter to the chosen
array/result.
---
Nitpick comments:
In `@app/Http/Controllers/Secretariat/DocumentController.php`:
- Around line 26-32: The Semester query in printingBladeData() is being executed
on every /documents request; change it to run only when the current user will
render the community-service table (e.g., check the user's secretary
role/permission before running the query) and eager-load the nested relations to
avoid N+1s by using with(...) for communityServices.requester and
communityServices.approver (or by eager-loading communityServices with those
relations inside the Semester query that currently uses
withWhereHas('communityServices', ...)); keep the existing approver/requester
filter logic but move it behind the role check so the query only executes when
needed.
In `@app/Utils/LatexHelper.php`:
- Line 55: Replace the weak rand(0, 100000) entropy used to build $filename in
LatexHelper.php with a cryptographically secure random source (e.g. use
Laravel's Illuminate\Support\Str::random(...) or PHP's random_bytes/bin2hex) and
keep the timestamp if you want human-readable uniqueness; update the $filename
construction in the method that sets $filename to combine date('c') with a
secure random string and add the appropriate import (Str) or error handling for
random_bytes as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fef1f0a-9689-4bd0-9c2a-0c19a20da01f
📒 Files selected for processing (15)
.gitignoreapp/Http/Controllers/Secretariat/DocumentController.phpapp/Http/Controllers/StudentsCouncil/CommunityServiceController.phpapp/Models/CommunityService.phpapp/Models/Role.phpapp/Policies/CommunityServicePolicy.phpapp/Utils/LatexHelper.phpconfig/database.phpdatabase/migrations/2026_02_13_184249_community_service_add_date_of_service.phpresources/views/latex/community-service.blade.phpresources/views/secretariat/document/index.blade.phpresources/views/student-council/community-service/app.blade.phpresources/views/student-council/community-service/request.blade.phpresources/views/student-council/community-service/table.blade.phproutes/web.php
🚧 Files skipped from review as they are similar to previous changes (4)
- app/Models/CommunityService.php
- app/Policies/CommunityServicePolicy.php
- resources/views/secretariat/document/index.blade.php
- resources/views/latex/community-service.blade.php
| /** | ||
| * Returns the role for the (director's) secretary. | ||
| */ | ||
| public static function secretary(): Role | ||
| { | ||
| return self::where('name', self::SECRETARY)->first(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find the Role model
find . -type f -name "Role.php" | head -20Repository: EotvosCollegium/mars
Length of output: 85
🏁 Script executed:
# Read the Role.php file to understand the full context around the secretary() method
cat -n app/Models/Role.php | head -300Repository: EotvosCollegium/mars
Length of output: 12527
🏁 Script executed:
# Let's also check if there's a get() method in the Role model and understand its return type
rg -A 10 "public static function get\(" app/Models/Role.phpRepository: EotvosCollegium/mars
Length of output: 465
🏁 Script executed:
# Search for usages of the secretary() method to see how it's called
rg "secretary\(\)" . --type php -B 2 -A 2Repository: EotvosCollegium/mars
Length of output: 3843
Return-type contract must match implementation.
The method declares Role (non-nullable) but first() returns Role|null. This creates a runtime risk: if the secretary role is missing from the database, the method returns null, violating its type contract and potentially causing a TypeError in calling code. Delegate to self::get(self::SECRETARY), which properly handles null cases by throwing an exception and maintains the non-null contract.
Note: The methods collegist(), studentsCouncil(), director(), and sysAdmin() suffer the same issue.
Suggested fix
public static function secretary(): Role
{
- return self::where('name', self::SECRETARY)->first();
+ return self::get(self::SECRETARY);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/Role.php` around lines 282 - 288, The methods (secretary(),
collegist(), studentsCouncil(), director(), sysAdmin()) declare a non-nullable
Role return but use self::where(...)->first() which can return null; replace
each body to delegate to self::get(CONSTANT) (e.g., return
self::get(self::SECRETARY)) so the lookup throws/handles missing roles and
satisfies the non-nullable Role contract; update each respective method to call
self::get with its constant (self::COLLEGIST, self::STUDENTS_COUNCIL,
self::DIRECTOR, self::SYS_ADMIN) instead of where(...)->first().
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/StudentsCouncil/CommunityServiceController.php (1)
53-66:⚠️ Potential issue | 🟠 MajorValidate
approveragainst the allowed recipients on the server.Line 54 only checks
users.id, so a crafted POST can target any account even though the form limits choices inresources/views/student-council/community-service/request.blade.php:2-4. Please whitelist the secretary and Student Council leaders here as well.Suggested fix
+use Illuminate\Validation\Rule; ... + $allowedApproverIds = collect([User::secretary()?->id]) + ->filter() + ->merge(collect(User::studentCouncilLeaders())->pluck('id')) + ->all(); + $request->validate([ - 'approver' => 'required|exists:users,id', + 'approver' => ['required', 'exists:users,id', Rule::in($allowedApproverIds)], 'description' => 'required|string', 'date_of_service' => 'nullable|string', ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 53 - 66, The current validation only ensures approver exists in users but not that they are an allowed recipient; update the validation logic around request->validate (the approver rule) to whitelist only the secretary and Student Council leaders by checking the approver user's role/position before creating the CommunityService (or add a custom validation Rule::exists with a where clause or a post-validate check); if the approver user is not in the allowed roles (e.g., secretary or student council leader) return a validation error and do not proceed to CommunityService::create (keep Semester::current() and other fields unchanged).
♻️ Duplicate comments (5)
resources/views/student-council/community-service/request.blade.php (1)
2-4:⚠️ Potential issue | 🟠 MajorStill unresolved: filter out a missing secretary before building
$approvers.
User::secretary()is nullable, so the secretariat branch can still pass[null]intox-input.selectwhen the post is vacant. Please drop null entries before assigning the options array.Suggested fix
- $approvers = $isForSecretariat - ? [\App\Models\User::secretary()] + $approvers = $isForSecretariat + ? array_values(array_filter([\App\Models\User::secretary()])) : \App\Models\User::studentCouncilLeaders()->toArray();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/student-council/community-service/request.blade.php` around lines 2 - 4, The $approvers assignment can include a null from \App\Models\User::secretary(), so when building $approvers (the ternary using $isForSecretariat) filter out null entries before passing to the select component; change the secretariat branch to produce an array without nulls (e.g., wrap the result in an array filter) so x-input.select never receives [null].app/Http/Controllers/StudentsCouncil/CommunityServiceController.php (2)
123-124:⚠️ Potential issue | 🔴 CriticalGuard the nullable
approvingRole()result before reading->name.
approvingRole()explicitly returnsnullon lines 107-108, but Line 124 dereferences it unconditionally. Please add a null check here rather than assuming the data is always pristine.Suggested fix
- $isForSecretariat = Role::SECRETARY == $approvingRole->name; + $isForSecretariat = !is_null($approvingRole) && $approvingRole->name === Role::SECRETARY;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 123 - 124, The code dereferences the possibly-null result of approvingRole() when reading ->name; update the CommunityServiceController logic to guard against a null approvingRole by checking the return of self::approvingRole($approver) before accessing ->name (e.g., assign to $approvingRole, if null set $isForSecretariat = false or handle appropriately), and then compare $approvingRole->name to Role::SECRETARY only when $approvingRole is not null.
138-140:⚠️ Potential issue | 🟠 MajorGuard
educationalInformationbefore reading the Neptun code.Line 139 assumes every requester has an attached educational-information record. If that relation is missing, certificate generation dies with a 500 instead of returning a user-facing error.
Minimal guard
$requester = $communityService->requester; + if (is_null($requester->educationalInformation)) { + return back()->with('error', 'A kérvényező tanulmányi adatai hiányoznak.'); + } $approver = Auth::user();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around lines 138 - 140, The code in CommunityServiceController is accessing $requester->educationalInformation->neptun without checking the relation; update the requester_neptun assignment to guard the relation (e.g., check $requester->educationalInformation exists) and either provide a safe fallback (empty string/null) or return a user-facing error before certificate generation; ensure this change is applied where 'requester_neptun' is set so missing educationalInformation does not produce a 500.app/Utils/LatexHelper.php (2)
15-23:⚠️ Potential issue | 🟠 Major
\symbol{mb_ord(...)}is still not Unicode-safe for pdfLaTeX.Lines 18-22 only special-case lowercase
ő/űand the en dash. UppercaseŐ/Ű, curly quotes, and any other character above 255 still become\symbol{336}/\symbol{8221}, which pdfLaTeX will reject. Please either move to a Unicode-capable engine or map those characters to proper LaTeX escapes instead of feeding rawmb_ord()values into\symbol.In pdfLaTeX, what numeric range is valid for \symbol{<number>} / \char, and what is the recommended way to emit UTF-8 characters such as Ő, ő, Ű, or curly quotes from application input?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` around lines 15 - 23, convertCharToSymbol currently emits \symbol{mb_ord(...)} for any non-space/en-dash/ő/ű which fails for Unicode codepoints >255 and misses uppercase Ő/Ű and curly quotes; update convertCharToSymbol to either (a) switch the project to a Unicode-capable LaTeX engine (XeLaTeX/LuaLaTeX) and emit characters directly as UTF-8, or (b) implement a mapping table inside convertCharToSymbol that translates known Unicode characters (e.g., Ő, Ű, curly quotes, en/em dashes, etc.) to proper LaTeX escapes (e.g., \H{O}, \H{u}, ``/'' or the appropriate commands) and only fall back to \symbol{...} for codepoints <=255; remove use of raw mb_ord for >255 codepoints and ensure convertCharToSymbol handles both lowercase and uppercase variants.
65-71:⚠️ Potential issue | 🟠 MajorHonour the debug switch and fail fast when compilation produces no PDF.
Line 66 still runs
Commands::latexToPdf()even when the method later decides to return the.texfile, andapp/Console/Commands.php:27-33only returns stdout. As written, this can both ignorecommands.run_in_debugand return a dead$pathPdfafter a LaTeX error.Suggested fix
- // TODO: figure out result - Commands::latexToPdf($pathTex, $outputDir); - if (config('app.debug') && !config('commands.run_in_debug')) { return $pathTex; - } else { - return $pathPdf; } + + $latexOutput = Commands::latexToPdf($pathTex, $outputDir); + if (!file_exists($pathPdf)) { + throw new \RuntimeException('LaTeX compilation failed: ' . $latexOutput); + } + + return $pathPdf;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Utils/LatexHelper.php` around lines 65 - 71, The code currently calls Commands::latexToPdf($pathTex, $outputDir) unconditionally and then sometimes returns $pathTex for debug, which can ignore the debug switch and return a non-existent $pathPdf after a LaTeX error; change LatexHelper to first check config('app.debug') && !config('commands.run_in_debug') and return $pathTex immediately without calling Commands::latexToPdf, otherwise call Commands::latexToPdf and capture its result (stdout/return value) and/or verify that $pathPdf exists and is non-empty, and if compilation failed throw/return a fast failure (exception or falsy) rather than returning a dead $pathPdf; reference Commands::latexToPdf, $pathTex, $pathPdf and app/Console/Commands.php behavior when implementing these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 101-107: The approvingRole lookup in approvingRole(User $user)
reads the approver's current Student Council role live (via
$user->roles()->where(...)->first()->pivot->object), which can change later;
instead persist a snapshot of the approver's title/object at creation/approval
time and read that stored value. Add a persistent field on the CommunityService
(or related) model (e.g., approving_title or approving_role_object and/or
approving_role_id) and a migration to persist the approver snapshot during the
certificate creation/approval flow (where approvals are recorded), update the
code paths that create/approve certificates to save the approver’s role/object
there, and finally change approvingRole to return the stored snapshot field(s)
rather than querying $user->roles() live; keep
Role::SECRETARY/Role::STUDENT_COUNCIL checks only when determining what to
store.
In `@resources/views/student-council/community-service/request.blade.php`:
- Around line 17-20: The unordered list in the Blade view (the <ul
class="browser-default"> block) still contains placeholder list items ("..." and
the inline red note "(ezt beszéljük majd meg pontosan)"); replace those
placeholder <li> entries with the final secretariat case items (Hungarian copy)
and remove the internal red note so the rendered page clearly lists which cases
belong to the secretariat; update the list content inside the same <ul
class="browser-default"> element in request.blade.php and ensure the items are
properly localized/escaped for production.
---
Outside diff comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 53-66: The current validation only ensures approver exists in
users but not that they are an allowed recipient; update the validation logic
around request->validate (the approver rule) to whitelist only the secretary and
Student Council leaders by checking the approver user's role/position before
creating the CommunityService (or add a custom validation Rule::exists with a
where clause or a post-validate check); if the approver user is not in the
allowed roles (e.g., secretary or student council leader) return a validation
error and do not proceed to CommunityService::create (keep Semester::current()
and other fields unchanged).
---
Duplicate comments:
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php`:
- Around line 123-124: The code dereferences the possibly-null result of
approvingRole() when reading ->name; update the CommunityServiceController logic
to guard against a null approvingRole by checking the return of
self::approvingRole($approver) before accessing ->name (e.g., assign to
$approvingRole, if null set $isForSecretariat = false or handle appropriately),
and then compare $approvingRole->name to Role::SECRETARY only when
$approvingRole is not null.
- Around line 138-140: The code in CommunityServiceController is accessing
$requester->educationalInformation->neptun without checking the relation; update
the requester_neptun assignment to guard the relation (e.g., check
$requester->educationalInformation exists) and either provide a safe fallback
(empty string/null) or return a user-facing error before certificate generation;
ensure this change is applied where 'requester_neptun' is set so missing
educationalInformation does not produce a 500.
In `@app/Utils/LatexHelper.php`:
- Around line 15-23: convertCharToSymbol currently emits \symbol{mb_ord(...)}
for any non-space/en-dash/ő/ű which fails for Unicode codepoints >255 and misses
uppercase Ő/Ű and curly quotes; update convertCharToSymbol to either (a) switch
the project to a Unicode-capable LaTeX engine (XeLaTeX/LuaLaTeX) and emit
characters directly as UTF-8, or (b) implement a mapping table inside
convertCharToSymbol that translates known Unicode characters (e.g., Ő, Ű, curly
quotes, en/em dashes, etc.) to proper LaTeX escapes (e.g., \H{O}, \H{u}, ``/''
or the appropriate commands) and only fall back to \symbol{...} for codepoints
<=255; remove use of raw mb_ord for >255 codepoints and ensure
convertCharToSymbol handles both lowercase and uppercase variants.
- Around line 65-71: The code currently calls Commands::latexToPdf($pathTex,
$outputDir) unconditionally and then sometimes returns $pathTex for debug, which
can ignore the debug switch and return a non-existent $pathPdf after a LaTeX
error; change LatexHelper to first check config('app.debug') &&
!config('commands.run_in_debug') and return $pathTex immediately without calling
Commands::latexToPdf, otherwise call Commands::latexToPdf and capture its result
(stdout/return value) and/or verify that $pathPdf exists and is non-empty, and
if compilation failed throw/return a fast failure (exception or falsy) rather
than returning a dead $pathPdf; reference Commands::latexToPdf, $pathTex,
$pathPdf and app/Console/Commands.php behavior when implementing these checks.
In `@resources/views/student-council/community-service/request.blade.php`:
- Around line 2-4: The $approvers assignment can include a null from
\App\Models\User::secretary(), so when building $approvers (the ternary using
$isForSecretariat) filter out null entries before passing to the select
component; change the secretariat branch to produce an array without nulls
(e.g., wrap the result in an array filter) so x-input.select never receives
[null].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e59f178f-9ab7-4262-b035-990e76852502
📒 Files selected for processing (4)
app/Http/Controllers/StudentsCouncil/CommunityServiceController.phpapp/Utils/LatexHelper.phpresources/views/latex/community-service.blade.phpresources/views/student-council/community-service/request.blade.php
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/latex/community-service.blade.php
| private static function approvingRole(User $user): Role|RoleObject|null | ||
| { | ||
| if ($user->hasRole(Role::SECRETARY)) { | ||
| return Role::secretary(); | ||
| } elseif ($user->hasRole(Role::STUDENT_COUNCIL)) { | ||
| return $user->roles()->where('role_id', Role::studentsCouncil()->id)->first()->pivot->object; | ||
| } else { |
There was a problem hiding this comment.
Persist the approving title instead of looking it up live.
Line 106 takes the approver's current first Student Council role. That makes the certificate title arbitrary for multi-role users, and it will drift if the approver's role changes after approval. For an official certificate, this wants a stored role/object snapshot from creation or approval time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/StudentsCouncil/CommunityServiceController.php` around
lines 101 - 107, The approvingRole lookup in approvingRole(User $user) reads the
approver's current Student Council role live (via
$user->roles()->where(...)->first()->pivot->object), which can change later;
instead persist a snapshot of the approver's title/object at creation/approval
time and read that stored value. Add a persistent field on the CommunityService
(or related) model (e.g., approving_title or approving_role_object and/or
approving_role_id) and a migration to persist the approver snapshot during the
certificate creation/approval flow (where approvals are recorded), update the
code paths that create/approve certificates to save the approver’s role/object
there, and finally change approvingRole to return the stored snapshot field(s)
rather than querying $user->roles() live; keep
Role::SECRETARY/Role::STUDENT_COUNCIL checks only when determining what to
store.
| <ul class="browser-default"> | ||
| <li>... <span style="color: red;">(ezt beszéljük majd meg pontosan)</span></li> | ||
| <li>...</li> | ||
| <li>...</li> |
There was a problem hiding this comment.
Please replace the placeholder secretariat list before release.
Lines 18-20 still render literal ... entries and an internal red note, so the new flow does not actually tell applicants which cases belong to the secretariat.
If useful, I can draft the final Hungarian copy in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/student-council/community-service/request.blade.php` around
lines 17 - 20, The unordered list in the Blade view (the <ul
class="browser-default"> block) still contains placeholder list items ("..." and
the inline red note "(ezt beszéljük majd meg pontosan)"); replace those
placeholder <li> entries with the final secretariat case items (Hungarian copy)
and remove the internal red note so the rendered page clearly lists which cases
belong to the secretariat; update the list content inside the same <ul
class="browser-default"> element in request.blade.php and ensure the items are
properly localized/escaped for production.
2 participants
Closes #782.
UPDATE: the new screenshots and examples:
mintaigazolas.pdf