Fixing tests - #804
Open
viktorcsimma wants to merge 8 commits into
Open
Fixing tests#804viktorcsimma wants to merge 8 commits into
viktorcsimma wants to merge 8 commits into
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdmission finalisation now removes associated application–workshop pivot records per application; Application missing-data computation for required documents runs only when the user has study lines; added English document file-type translations; tests updated to send Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as AdmissionController
participant Application
participant Pivot as ApplicationWorkshop
participant User
Client->>Controller: POST /admin/admissions/finalize
Controller->>Application: query admitted/processed applications
loop per Application
Controller->>Application: mark as admitted and delete (soft or force)
alt user verified / soft-delete branch
Application->>Pivot: applicationWorkshops()->delete()
Application->>User: (soft-)delete user if applicable
else unverified / force-delete branch
Application->>Pivot: applicationWorkshops()->forceDelete()
Application->>User: forceDelete user
end
end
Controller-->>Client: return finalisation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
Contributor
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/Auth/AdmissionController.php (1)
246-256:⚠️ Potential issue | 🟠 MajorThis introduces avoidable data loss and redundant deletion in
finalize().On Line 246, workshop pivot rows are deleted even for soft-deleted applications that are meant to remain for future reference. On Line 256, manual pivot
forceDelete()is redundant once the parent application is force-deleted with cascade FK enabled.🔧 Proposed adjustment
$application->delete(); - $application->applicationWorkshops()->delete(); } else { @@ $application->forceDelete(); $application->user->forceDelete(); - $application->applicationWorkshops()->forceDelete(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Auth/AdmissionController.php` around lines 246 - 256, In AdmissionController::finalize(), avoid removing workshop pivot rows for soft-deletes and drop the redundant manual pivot forceDelete: do not call $application->applicationWorkshops()->delete() in the soft-delete branch (leave pivots intact for later reference), and remove the final $application->applicationWorkshops()->forceDelete() after $application->forceDelete() so the FK cascade handles pivot removal when performing a hard delete; keep existing file deletions and user/application forceDelete logic intact.
🧹 Nitpick comments (1)
app/Models/ApplicationWorkshop.php (1)
9-11: Please tighten the class docblock wording.The model links applications to workshops (
application_id,workshop_id), not users directly, so this phrasing could mislead future maintainers.💡 Suggested doc tweak
- * A pivot class between users and the workshops they apply for. + * A pivot class between applications and the workshops they apply for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ApplicationWorkshop.php` around lines 9 - 11, The class docblock for ApplicationWorkshop is misleadingly phrased as linking "users and the workshops they apply for"; update the docblock to accurately state that this pivot model links applications to workshops via application_id and workshop_id (mention ApplicationWorkshop class and the application_id/workshop_id fields) so maintainers understand its purpose; keep the wording concise and correct the description in the ApplicationWorkshop docblock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/en/document.php`:
- Line 16: The English translation key 'besorolasi_hatarozat' in
resources/lang/en/document.php is empty; update the value to a meaningful
English label (for example "Classification Decision" or another appropriate
phrase) so the UI won't show a blank; locate the 'besorolasi_hatarozat' entry in
the document.php array and replace the empty string with the chosen English text
ensuring the syntax remains a valid PHP array entry.
---
Outside diff comments:
In `@app/Http/Controllers/Auth/AdmissionController.php`:
- Around line 246-256: In AdmissionController::finalize(), avoid removing
workshop pivot rows for soft-deletes and drop the redundant manual pivot
forceDelete: do not call $application->applicationWorkshops()->delete() in the
soft-delete branch (leave pivots intact for later reference), and remove the
final $application->applicationWorkshops()->forceDelete() after
$application->forceDelete() so the FK cascade handles pivot removal when
performing a hard delete; keep existing file deletions and user/application
forceDelete logic intact.
---
Nitpick comments:
In `@app/Models/ApplicationWorkshop.php`:
- Around line 9-11: The class docblock for ApplicationWorkshop is misleadingly
phrased as linking "users and the workshops they apply for"; update the docblock
to accurately state that this pivot model links applications to workshops via
application_id and workshop_id (mention ApplicationWorkshop class and the
application_id/workshop_id fields) so maintainers understand its purpose; keep
the wording concise and correct the description in the ApplicationWorkshop
docblock.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cc630ef-d086-42ce-9e7a-bd525c00d9d3
📒 Files selected for processing (6)
.gitignoreapp/Http/Controllers/Auth/AdmissionController.phpapp/Models/Application.phpapp/Models/ApplicationWorkshop.phpresources/lang/en/document.phptests/Feature/ApplicationTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Http/Controllers/Auth/AdmissionController.php (1)
256-256: Remove redundant pivot deletion after parent force-delete.Line 256 is unnecessary once Line 254 runs, because the FK cascade on
application_workshops.application_idalready deletes dependent rows.Suggested simplification
$application->forceDelete(); $application->user->forceDelete(); - $application->applicationWorkshops()->forceDelete();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Auth/AdmissionController.php` at line 256, The call to $application->applicationWorkshops()->forceDelete() is redundant because the parent model is already force-deleted and the foreign-key cascade on application_workshops.application_id removes those rows; remove the $application->applicationWorkshops()->forceDelete() invocation (the redundant pivot deletion) from the code so only the parent forceDelete remains.
🤖 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/Auth/AdmissionController.php`:
- Line 246: The current code calls
$application->applicationWorkshops()->delete() which removes historical workshop
selections right after soft-deleting the application; instead either remove that
delete so the applicationWorkshops relation is preserved for archived
(soft-deleted) applications, or copy each related ApplicationWorkshop into an
immutable archive (e.g., ApplicationWorkshopArchive) before deleting and then
delete the originals only if you truly want them gone; locate the call to
applicationWorkshops() in AdmissionController (the delete invocation) and
replace it with logic that either skips deletion or iterates the related models
to persist them into an archive model/table and then conditionally delete the
originals.
---
Nitpick comments:
In `@app/Http/Controllers/Auth/AdmissionController.php`:
- Line 256: The call to $application->applicationWorkshops()->forceDelete() is
redundant because the parent model is already force-deleted and the foreign-key
cascade on application_workshops.application_id removes those rows; remove the
$application->applicationWorkshops()->forceDelete() invocation (the redundant
pivot deletion) from the code so only the parent forceDelete remains.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e0e13ae-065c-4fcd-ba06-72a185866a69
📒 Files selected for processing (5)
app/Http/Controllers/Auth/AdmissionController.phpapp/Models/Application.phpapp/Models/ApplicationWorkshop.phpresources/lang/en/document.phptests/Feature/ApplicationTest.php
✅ Files skipped from review due to trivial changes (1)
- app/Models/ApplicationWorkshop.php
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/lang/en/document.php
- app/Models/Application.php
- tests/Feature/ApplicationTest.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Policies/UserPolicy.php (1)
253-300:⚠️ Potential issue | 🟠 MajorGuard nullable
$objectbefore using$object->id.Line 271 and Line 294 dereference
$object->idalthough$objectis now explicitly nullable. A null or wrong object type here will raise a runtime error instead of cleanly denying permission.Proposed hardening patch
public function updatePermission(User $user, User $target, Role $role, Workshop|RoleObject|null $object = null): bool { + if ( + in_array($role->name, [Role::APPLICATION_COMMITTEE_MEMBER, Role::WORKSHOP_ADMINISTRATOR], true) + && !($object instanceof Workshop) + ) { + return false; + } + if ($role->name == Role::TENANT) { return $user->hasRole([Role::STAFF]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Policies/UserPolicy.php` around lines 253 - 300, The updatePermission method dereferences $object->id while $object can be null or a different type; before using $object->id in the APPLICATION_COMMITTEE_MEMBER and WORKSHOP_ADMINISTRATOR branches, first guard that $object is non-null and the expected type (e.g. $object instanceof Workshop or appropriate RoleObject), and only perform the hasManyThrough()->where('id', $object->id)->exists() check when that guard passes; otherwise treat the check as false (fall through to the other role checks or return false) so no runtime null/dereference error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/Policies/UserPolicy.php`:
- Around line 253-300: The updatePermission method dereferences $object->id
while $object can be null or a different type; before using $object->id in the
APPLICATION_COMMITTEE_MEMBER and WORKSHOP_ADMINISTRATOR branches, first guard
that $object is non-null and the expected type (e.g. $object instanceof Workshop
or appropriate RoleObject), and only perform the hasManyThrough()->where('id',
$object->id)->exists() check when that guard passes; otherwise treat the check
as false (fall through to the other role checks or return false) so no runtime
null/dereference error occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03205862-feb5-45b7-a9ae-bc8d4bfbabe0
📒 Files selected for processing (12)
app/Http/Controllers/Auth/AdmissionController.phpapp/Http/Controllers/StudentsCouncil/QuestionController.phpapp/Mail/CaesarProxyTransport.phpapp/Mail/EvaluationFormClosed.phpapp/Models/AnonymousQuestions/AnswerSheet.phpapp/Models/Internet/InternetAccess.phpapp/Models/Role.phpapp/Models/User.phpapp/Models/Workshop.phpapp/Policies/UserPolicy.phpapp/Utils/HasPeriodicEvent.phpapp/Utils/HasRoles.php
✅ Files skipped from review due to trivial changes (7)
- app/Mail/EvaluationFormClosed.php
- app/Utils/HasPeriodicEvent.php
- app/Models/User.php
- app/Http/Controllers/StudentsCouncil/QuestionController.php
- app/Models/Internet/InternetAccess.php
- app/Models/Role.php
- app/Mail/CaesarProxyTransport.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Controllers/Auth/AdmissionController.php
viktorcsimma
marked this pull request as ready for review
April 28, 2026 13:53
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
1 participant
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.
As in the title. Now, all PHPUnit test cases, as well as PHPStan, should run and succeed. (The problems were mostly with application-related things.)