Skip to content

Refine userpolicy and allow partial fillings in application #746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 19, 2025

Conversation

horcsinbalint
Copy link
Member

Description

Now applicants are able to save partially filled forms during the application process and the presence of those values are only checked when they submit their entire application.
Furthermore, applicants are no longer able to edit data once they have submitted their application.
For editing userdata, being able to view it is no longer enough, the UserPolicy has been revised a bit (only the user, secretariat, student council leaders and sysadmins can edit data from now on). Users should not be allowed to edit some of their data (like workshop list, date of birth).

Related Issue

Closes #234

@horcsinbalint horcsinbalint requested a review from a team as a code owner July 15, 2025 21:59
deepsource-autofix bot added a commit that referenced this pull request Jul 15, 2025
This commit fixes the style issues introduced in 47f7ae7 according to the output
from PHP CS Fixer.

Details: #746
Copy link
Contributor

coderabbitai bot commented Jul 15, 2025

📝 Walkthrough

Walkthrough

This update introduces detailed field-level validation and permission checks for user and application data management. It refines the UserPolicy with new granular authorisation methods, enhances validation logic and error reporting, adjusts database schema for nullable fields, and updates user interface components to reflect required fields and permissions. Related tests and translations are updated accordingly.

Changes

File(s) Change Summary
app/Http/Controllers/Auth/ApplicationController.php ensureApplicationExists now creates missing related user data (personal and educational information) alongside applications; success message in confirmStart simplified.
app/Http/Controllers/Secretariat/UserController.php Authorisation checks updated to require 'edit' permission; validation and update logic for user info refactored; tenant date validation expanded; role management authorisation streamlined.
app/Models/Application.php missingData() refactored for granular field-level checks and detailed missing data messages.
app/Models/EducationalInformation.php, tests/Unit/AlfonsoTest.php Changed method visibility: isMasterAdmittee() and autumnSemester() are now public.
app/Policies/UserPolicy.php Added edit, editStaticPersonalInformation, and editStaticEducationalInformation methods for layered permission checks.
app/Utils/ApplicationHandler.php Validation rules updated for questions data; logic for status and accommodation fields refined.
app/Utils/HasRoles.php Added hasRoleOtherThanTenant() to check for roles excluding 'TENANT'.
database/migrations/2025_07_11_184933_make_phone_number_nullable.php Makes phone_number in personal_information nullable.
database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php Makes name in study_lines nullable.
resources/lang/en/user.php, resources/lang/hu/user.php Added/updated translation entries for edit restrictions and field labels.
resources/views/auth/application/app.blade.php Button label updated for clarity.
resources/views/auth/application/files.blade.php File input label and required field indicators updated.
resources/views/auth/application/questions.blade.php Enhanced required field indicators and helper texts; improved textarea labels.
resources/views/components/input/*.blade.php All input components updated to conditionally display a red asterisk for required fields.
resources/views/user/alfonso-language-exams.blade.php Form access now based on edit permission; required field indicators added.
resources/views/user/alfonso.blade.php Clarified ALFONSÓ language/level instructions; input enablement based on permissions.
resources/views/user/educational-information.blade.php Input enablement, required indicators, and helper texts now reflect granular permissions.
resources/views/user/personal-information.blade.php Field enablement and required status dynamically controlled by permissions; improved user guidance.
resources/views/user/study-line-selector.blade.php Field requirements and enablement now conditional on user role and permissions.
resources/views/user/update_tenant_status.blade.php Datepicker now restricts date range to today through six months ahead.
resources/views/utils/user/profile-picture.blade.php Profile picture forms now shown only to users with edit permission; label updated.
tests/Feature/ApplicationTest.php Test assertions updated for granular field-level missing data checks.
tests/Feature/UserTest.php Removed default role and application creation in user setup.
tests/Unit/AlfonsoTest.php autumnSemester() visibility changed to public.
app/Livewire/ParentChildForm.php Added public property $inputAttributes for flexible input attribute customization.
app/Models/StudyLine.php getName() method now returns empty string if name is null.
resources/views/livewire/parent-child-form.blade.php Input element updated to apply dynamic attributes from $inputAttributes.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI
    participant UserController
    participant UserPolicy
    participant ApplicationController
    participant Models

    User->>UI: Attempts to edit user/application data
    UI->>UserController: Submits form data
    UserController->>UserPolicy: Checks 'edit' or related permissions
    alt Permission granted
        UserController->>Models: Validates and updates data
        Models-->>UserController: Returns success/failure
        UserController-->>UI: Displays result
    else Permission denied
        UserController-->>UI: Displays error or disables fields
    end
Loading

Possibly related PRs

  • EotvosCollegium/mars#744: Changes the visibility of the autumnSemester() method in AlfonsoTest, directly related to this update's method visibility change.
  • EotvosCollegium/mars#613: Refactors the missingData() method in Application.php, similar to the granular validation changes in this update.
  • EotvosCollegium/mars#687: Modifies the AlfonsoTest class, which is also updated in this PR.

Suggested labels

enhancement, priority

Suggested reviewers

  • kdmnk
  • horcsinbalint

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af0af2c and 34b71cb.

📒 Files selected for processing (1)
  • app/Http/Controllers/Secretariat/UserController.php (14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/Http/Controllers/Secretariat/UserController.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: phpunit
  • GitHub Check: phpunit
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@horcsinbalint horcsinbalint marked this pull request as draft July 15, 2025 22:04
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (6)
resources/views/components/input/checkbox.blade.php (1)

15-18: Functionality approved, though consider CSS class for styling consistency.

The conditional asterisk rendering works correctly and follows the expected pattern. However, the inline style="color:red;" approach might be worth standardising with a CSS class for better maintainability across the application.

resources/views/auth/application/files.blade.php (1)

20-23: Asterisk indicator properly implemented, but consider internationalization.

The required field indicator works correctly. However, the change from a translation key to hardcoded Hungarian text ("Fájl kiválasztása") may pose internationalization concerns if the application supports multiple languages.

app/Models/EducationalInformation.php (1)

130-130: Consider the implications of exposing this method publicly.

Whilst the visibility change enables external permission checks, it exposes internal business logic that was previously encapsulated. This could lead to inconsistent usage patterns across the codebase.

Consider whether a more structured approach would be preferable, such as:

  • Creating a dedicated service class for permission-related checks
  • Adding a facade method that provides a cleaner interface
  • Using Laravel's authorization system more extensively
app/Http/Controllers/Auth/ApplicationController.php (1)

153-153: Consider using Carbon for date handling consistency.

Whilst date('Y') is functional, using Carbon would provide better consistency with the rest of the Laravel application and make the code more testable.

-                    'year_of_acceptance' => date('Y')
+                    'year_of_acceptance' => now()->year
resources/lang/hu/user.php (1)

23-23: Consider aligning contact options across languages.

The Hungarian version provides more comprehensive contact information (system administrators, student council, and secretariat) compared to the English version which only mentions system administrators. This inconsistency might confuse users.

Consider updating the English version to match the Hungarian comprehensiveness:

-'data_cannot_be_edited_application' => 'To edit your information, contact the [system administrators](mailto::SYSADMIN_EMAIL).',
+'data_cannot_be_edited_application' => 'Due to the admission process, you currently cannot edit your information. To make changes, please contact the [system administrators](mailto::SYSADMIN_EMAIL), the [Student Council](mailto::STUDENT_COUNCIL_EMAIL), or the [Secretariat](mailto::SECRETARIAT_EMAIL).',
resources/views/user/alfonso.blade.php (1)

39-42: Consider consistency in disabled state styling.

The form controls properly implement permission-based disabling and required field indicators. However, ensure that disabled form elements have appropriate visual styling to clearly indicate their non-editable state to users.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0642de0 and 921cf5f.

📒 Files selected for processing (31)
  • app/Http/Controllers/Auth/ApplicationController.php (1 hunks)
  • app/Http/Controllers/Secretariat/UserController.php (14 hunks)
  • app/Models/Application.php (1 hunks)
  • app/Models/EducationalInformation.php (1 hunks)
  • app/Policies/UserPolicy.php (2 hunks)
  • app/Utils/ApplicationHandler.php (2 hunks)
  • app/Utils/HasRoles.php (1 hunks)
  • database/migrations/2025_07_11_184933_make_phone_number_nullable.php (1 hunks)
  • database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (1 hunks)
  • resources/lang/en/user.php (2 hunks)
  • resources/lang/hu/user.php (2 hunks)
  • resources/views/auth/application/app.blade.php (1 hunks)
  • resources/views/auth/application/files.blade.php (1 hunks)
  • resources/views/auth/application/questions.blade.php (6 hunks)
  • resources/views/components/input/checkbox.blade.php (1 hunks)
  • resources/views/components/input/datepicker.blade.php (2 hunks)
  • resources/views/components/input/file.blade.php (1 hunks)
  • resources/views/components/input/select.blade.php (1 hunks)
  • resources/views/components/input/text.blade.php (1 hunks)
  • resources/views/components/input/textarea.blade.php (1 hunks)
  • resources/views/components/input/timepicker.blade.php (1 hunks)
  • resources/views/user/alfonso-language-exams.blade.php (1 hunks)
  • resources/views/user/alfonso.blade.php (1 hunks)
  • resources/views/user/educational-information.blade.php (3 hunks)
  • resources/views/user/personal-information.blade.php (1 hunks)
  • resources/views/user/study-line-selector.blade.php (1 hunks)
  • resources/views/user/update_tenant_status.blade.php (1 hunks)
  • resources/views/utils/user/profile-picture.blade.php (3 hunks)
  • tests/Feature/ApplicationTest.php (6 hunks)
  • tests/Feature/UserTest.php (0 hunks)
  • tests/Unit/AlfonsoTest.php (1 hunks)
💤 Files with no reviewable changes (1)
  • tests/Feature/UserTest.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: horcsinbalint
PR: EotvosCollegium/mars#743
File: app/Http/Controllers/Auth/AdmissionController.php:237-237
Timestamp: 2025-07-08T22:59:53.901Z
Learning: When horcsinbalint defers issues for "human reviewers to check later," he's making a conscious decision to prioritize the current PR over fixing secondary issues, and this should be acknowledged rather than being insistent about immediate fixes.
app/Utils/ApplicationHandler.php (1)
Learnt from: viktorcsimma
PR: EotvosCollegium/mars#542
File: app/Http/Controllers/Secretariat/SemesterEvaluationController.php:161-161
Timestamp: 2024-06-13T19:25:19.821Z
Learning: 'anonymous_questions' is a section name in the 'secretariat.evaluation-form.app' view and does not require handling in the `store` method of `SemesterEvaluationController.php`.
resources/views/auth/application/questions.blade.php (1)
Learnt from: viktorcsimma
PR: EotvosCollegium/mars#542
File: app/Http/Controllers/Secretariat/SemesterEvaluationController.php:161-161
Timestamp: 2024-06-13T19:25:19.821Z
Learning: 'anonymous_questions' is a section name in the 'secretariat.evaluation-form.app' view and does not require handling in the `store` method of `SemesterEvaluationController.php`.
🧬 Code Graph Analysis (4)
database/migrations/2025_07_11_184933_make_phone_number_nullable.php (1)
database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (2)
  • up (11-16)
  • down (21-26)
tests/Unit/AlfonsoTest.php (1)
app/Models/Semester.php (1)
  • Semester (51-373)
app/Utils/HasRoles.php (2)
app/Models/User.php (1)
  • roles (218-222)
app/Models/Role.php (1)
  • Role (34-335)
database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (1)
database/migrations/2025_07_11_184933_make_phone_number_nullable.php (2)
  • up (11-16)
  • down (21-26)
🪛 GitHub Actions: CI
tests/Feature/ApplicationTest.php

[error] 249-262: PHPUnit test failure in test_submit: Expected response status code 302 but received 500 due to exception in UserController.

app/Http/Controllers/Secretariat/UserController.php

[error] 230-230: Error: Call to a member function has() on array causing HTTP 500 response instead of expected 302 in test_submit.

🪛 PHPMD (2.15.0)
app/Http/Controllers/Secretariat/UserController.php

266-266: Avoid unused local variables such as '$data'. (Unused Code Rules)

(UnusedLocalVariable)

🔇 Additional comments (44)
tests/Unit/AlfonsoTest.php (1)

45-45: Excellent visibility change for improved code reuse.

Making this method public allows other test classes to utilise this utility function, promoting consistency across the test suite. The method logic remains sound and unchanged.

app/Utils/HasRoles.php (1)

202-205: Well-implemented utility method for role-based authorization.

The method provides a clear and focused check for users with roles beyond TENANT. The implementation is straightforward and the naming is descriptive. This will be particularly useful for authorization policies.

resources/views/components/input/textarea.blade.php (1)

10-12: Proper implementation of required field visual indicator.

The conditional red asterisk follows standard UX patterns and maintains consistency with other input components. This enhancement will improve user experience by clearly indicating mandatory fields.

resources/views/auth/application/app.blade.php (1)

89-89: Improved button text clarity.

The change from "Ellenőrzés" to "Áttekintés" better describes the button's function, as "overview" is more accurate than "check" for a review and finalisation step. This enhances user understanding of the interface.

resources/views/user/update_tenant_status.blade.php (1)

20-23: Excellent frontend validation alignment with backend constraints.

The date constraints properly prevent users from selecting invalid dates, whilst the null-safe operator ensures robust value binding. This frontend-backend consistency enhances both user experience and data integrity.

resources/views/components/input/datepicker.blade.php (1)

33-42: Excellent enhancement for date validation constraints.

The addition of conditional maxDate and minDate configuration is a splendid improvement that enhances form validation and user experience. The implementation properly checks for attribute presence and converts values to JavaScript Date objects.

This change provides valuable date range validation that will prevent users from selecting invalid dates, which is particularly important for fields like tenant dates as mentioned in the AI summary.

database/migrations/2025_07_11_184933_make_phone_number_nullable.php (2)

11-16: Migration structure is sound and aligns with PR objectives.

The nullable change to phone_number properly supports the partial application filling requirement mentioned in the PR objectives, allowing users to save incomplete forms whilst deferring validation until full submission.


21-26: Down method correctly implements rollback logic.

The reversion to non-nullable state is properly implemented, ensuring the migration can be safely rolled back if needed.

database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (2)

11-16: Migration follows established pattern and supports partial filling.

The nullable change to the name field in study_lines is consistent with the similar migration for phone_number and properly supports the partial application filling feature described in the PR objectives.


21-26: Rollback logic correctly implemented.

The down method properly reverts the nullable change, maintaining migration integrity.

resources/views/auth/application/files.blade.php (1)

24-26: Consistent asterisk implementation for name field.

The asterisk attribute addition maintains consistency with the file input above and follows the established pattern across input components.

resources/views/user/alfonso-language-exams.blade.php (2)

45-45: Excellent improvement to use proper authorization.

The change from checking $user->educationalInformation to using @can('edit', $user) is a significant improvement that properly implements the refined UserPolicy mentioned in the PR objectives, ensuring consistent permission checks across the application.


50-65: Consistent asterisk implementation across form inputs.

The asterisk attributes have been properly added to all required fields (file, language, level, type, date), maintaining consistency with the UI improvements implemented across other input components in this PR.

resources/lang/en/user.php (2)

72-72: LGTM! Clear and helpful user messaging.

The language entries provide clear guidance to users about editing restrictions and appropriate contact channels.


23-23: Email placeholder replacement mechanism is secure

I’ve confirmed that the ::SYSADMIN_EMAIL, ::SECRETARIAT_EMAIL and ::STUDENT_COUNCIL_EMAIL tokens are never sourced from user input but from trusted config entries (config('mail.sys_admin_mail'), etc.). They’re applied in two ways:

  • For configurable texts, the ConfigurableText model’s get accessor runs a simple str_replace on the raw markdown, swapping each placeholder for its config value.
  • For user language strings, the Blade @markdown directive receives the translation (which still contains ::SYSADMIN_EMAIL) and a second argument array of the same config values. This array is passed into the markdown-to-HTML renderer, which produces safe <a href="mailto:…"> links via the Markdown parser. There is no opportunity for untrusted input or HTML injection.

No sanitisation gaps were found, and no further changes are required.

app/Http/Controllers/Auth/ApplicationController.php (1)

147-156: LGTM! Proper data integrity enforcement.

The extension to ensure related user data entities exist is well-implemented and follows the established pattern. The automatic creation of personalInformation and educationalInformation supports the partial filling feature effectively.

resources/views/user/study-line-selector.blade.php (2)

7-11: Excellent implementation of permission-based UI controls.

The conditional requirements and permission checks are properly implemented. The asterisk indicators and disabled states provide clear visual feedback to users about field requirements and editing restrictions.


57-59: Proper authorization for destructive actions.

The @can('edit', $user) directive correctly restricts the delete button to authorised users only, preventing unauthorised data modification.

resources/lang/hu/user.php (1)

76-76: LGTM! Improved translation specificity.

The refinement from "Szak" to "Szak neve" provides better clarity for users about what information is expected in this field.

resources/views/user/alfonso.blade.php (2)

18-34: Excellent implementation of granular permission checks.

The conditional display of informational messages based on user permissions is well-structured. The separation between general edit permissions and static educational information editing provides appropriate access control granularity.


50-52: Proper authorization enforcement on form submission.

The conditional rendering of the save button based on permissions is correct and prevents unauthorised form submissions. This aligns well with the backend authorization checks.

app/Utils/ApplicationHandler.php (3)

22-32: Well-implemented nullable validation rules for partial completion.

The changes to make fields nullable support the PR objective of allowing partial application completion. The validation rules properly handle optional data whilst maintaining data integrity.


43-52: Robust null handling for status field.

The status handling logic correctly manages null values, which supports partial application completion. The explicit null assignment when status is not set is appropriate for the new nullable approach.


40-40: Validation rule for ‘accommodation’ is correct

The Blade checkbox in resources/views/auth/application/questions.blade.php submits a boolean-like value (“on” when checked), which the sometimes|accepted rule rightly validates. The handler’s line

$data['accommodation'] = isset($data['accommodation']) && $data['accommodation'];

and the model’s boolean cast ensure the database field remains in sync. No further changes required.

resources/views/utils/user/profile-picture.blade.php (3)

9-19: Proper authorization implementation for profile picture deletion.

The permission check using @can('edit', $user) correctly restricts profile picture deletion to authorised users. The consistent implementation across both desktop and mobile views is commendable.


24-35: Consistent authorization for profile picture upload.

The upload form is properly wrapped with permission checks, ensuring only authorised users can modify profile pictures. The form structure and validation remain intact whilst adding security controls.


30-31: Improved user guidance with descriptive labels.

The updated file input text "Profilkép választása - nem kötelező" provides clearer guidance to users about the optional nature of profile pictures. This enhances the user experience.

Also applies to: 53-53

resources/views/auth/application/questions.blade.php (4)

12-12: Excellent visual indicators for required fields.

The addition of red asterisks to indicate required fields significantly improves user experience by clearly communicating mandatory information. This visual enhancement aligns well with the form validation approach.

Also applies to: 47-47, 72-72, 91-91


122-142: Improved question input structure with explicit labels.

The conversion from x-input components to explicit labels and textareas provides better semantic structure and accessibility. The consistent styling with font-size: 15px ensures visual consistency.


145-148: Enhanced textarea with proper constraints.

The addition of maxlength="5000" and improved helper text formatting provides better user guidance and data validation. This enhancement supports proper form completion.


51-51: Ensure server-side validation for the “status” radio group

I wasn’t able to find any server-side rule validating the status field in your FormRequest classes or controller validate() calls. Please confirm that your final submission request includes something like:

'status' => 'required|in:resident,passive,alumni,…'

Key locations to check:

  • app/Http/Controllers/Auth/AdmissionController.php (currently only validates a status_filter)
  • app/Http/Controllers/Secretariat/SemesterEvaluationController.php (validates section, not status)
  • Any FormRequest under app/Http/Requests (directory does not appear to exist)
resources/views/user/educational-information.blade.php (5)

3-19: Exemplary implementation of layered permission messaging.

The conditional display of informational messages based on different permission levels (edit vs editStaticEducationalInformation) provides excellent user feedback. The structured approach to permission-based UI is commendable.


28-32: Consistent permission-based field control implementation.

The systematic application of permission checks, required indicators, and validation attributes across input fields demonstrates excellent attention to detail. The conditional requirements based on user roles (collegist, alumni, application presence) are well-implemented.

Also applies to: 36-39, 40-46


47-55: Robust validation for Neptun code field.

The Neptun code field implementation with proper regex pattern validation (^$|^[A-Za-z0-9]{6}$) and length constraints demonstrates good data validation practices. The pattern allows for empty values or exactly 6 alphanumeric characters.


76-78: Proper authorization for checkbox controls.

The permission-based disabling of faculty and workshop checkboxes ensures consistent access control across all form elements. The implementation maintains data integrity whilst providing appropriate user feedback.

Also applies to: 94-96


109-111: Consistent conditional rendering of action elements.

The conditional rendering of the "add study line" button and save button based on user permissions ensures UI consistency and prevents unauthorised actions. This approach aligns well with the backend authorization checks.

Also applies to: 130-133

resources/views/user/personal-information.blade.php (3)

8-24: Well-implemented permission-based messaging

The conditional display of informational messages based on user permissions is properly structured and provides clear guidance to users about their editing capabilities.


25-159: Properly implemented field-level permission controls

The dynamic disabling of form inputs and conditional required attributes based on user permissions is well-executed. The distinction between edit and editStaticPersonalInformation permissions provides appropriate granularity.


162-164: Appropriate save button access control

The conditional rendering of the save button based on edit permission prevents unauthorised form submissions at the UI level.

app/Models/Application.php (1)

336-421: Excellent refactoring of missing data validation

The transformation from broad presence checks to detailed field-level validation significantly improves the user experience by providing specific feedback about missing data. The conditional ALFONSÓ validation based on admission year and exemption status demonstrates thoughtful implementation.

app/Policies/UserPolicy.php (1)

43-93: Well-structured layered authorization methods

The three new authorization methods provide appropriate granularity for controlling user data editing permissions. The hierarchical structure, where more specific methods build upon the base edit method, follows good design principles.

tests/Feature/ApplicationTest.php (1)

185-367: Test updates align with validation changes

The transformation of test assertions from broad categories to individual field checks properly validates the refactored missingData() method. However, the pipeline failure indicates an issue with the educational information update endpoint.

The pipeline failure at lines 249-262 suggests the educational information update is returning HTTP 500 instead of 302. This is likely related to the issue in UserController::updateEducationalInformation() at line 230.

app/Http/Controllers/Secretariat/UserController.php (2)

60-60: Appropriate elevation of authorization requirements

The change from 'view' to 'edit' permissions across multiple methods correctly implements the stricter access control requirements for modifying user data.

Also applies to: 85-85, 105-105, 159-159, 263-263, 286-286, 318-318, 341-341


108-130: Well-designed dynamic validation rules

The conditional building of validation rules based on user permissions is excellently implemented. This ensures that only editable fields are validated whilst maintaining data integrity for restricted fields.

Also applies to: 161-193

@@ -6,6 +6,7 @@
use App\Models\RoleObject;
use App\Models\User;
use App\Models\Workshop;
use App\Http\Controllers\Auth\ApplicationController;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Architectural concern: Controller dependency in policy

Importing and instantiating a controller within a policy violates the separation of concerns principle. The ApplicationController::isActive() check should be refactored to use a service or helper method instead.

Consider extracting the application period logic to a dedicated service:

-use App\Http\Controllers\Auth\ApplicationController;
+use App\Services\ApplicationService;

And in the edit method:

-    app(ApplicationController::class)->isActive() &&
+    app(ApplicationService::class)->isApplicationPeriodActive() &&

Also applies to: 52-55

🤖 Prompt for AI Agents
In app/Policies/UserPolicy.php at line 9 and also lines 52-55, remove the import
and usage of ApplicationController within the policy to avoid controller
dependency. Refactor by extracting the application period logic currently in
ApplicationController::isActive() into a dedicated service or helper class.
Then, update the policy to call this new service method instead of the
controller method, maintaining separation of concerns and proper architecture.

deepsource-autofix bot added a commit that referenced this pull request Jul 15, 2025
This commit fixes the style issues introduced in f492c3e according to the output
from PHP CS Fixer.

Details: #746
deepsource-autofix bot added a commit that referenced this pull request Jul 15, 2025
This commit fixes the style issues introduced in 5271cb1 according to the output
from PHP CS Fixer.

Details: #746
@horcsinbalint horcsinbalint requested a review from Copilot July 15, 2025 22:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements partial filling support for application forms and refines user permissions for data editing. Users can now save partially completed forms during the application process, with validation only occurring upon final submission. The PR also restricts data editing once applications are submitted and updates the UserPolicy to properly control who can edit user data.

  • Enables partial form filling during application process with validation only on submission
  • Restricts user data editing based on application status and user roles
  • Updates UserPolicy to allow editing only for specific roles (secretariat, student council leaders, sysadmins) plus the user themselves under certain conditions

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/Unit/AlfonsoTest.php Changed autumnSemester method visibility from private to public
tests/Feature/UserTest.php Removed automatic role assignment and application creation in test setup
tests/Feature/ApplicationTest.php Updated test assertions to check for specific missing data fields instead of general categories
resources/views/utils/user/profile-picture.blade.php Added authorization checks for profile picture editing
resources/views/user/update_tenant_status.blade.php Added date range validation for tenant until field
resources/views/user/study-line-selector.blade.php Added conditional requirements and edit permissions for study line fields
resources/views/user/personal-information.blade.php Added edit permission checks and conditional field requirements
resources/views/user/educational-information.blade.php Added edit permission checks and conditional field requirements
resources/views/user/alfonso.blade.php Added edit permission checks and conditional requirements for Alfonso language selection
resources/views/user/alfonso-language-exams.blade.php Added edit permission checks for language exam uploads
resources/views/components/input/*.blade.php Added asterisk support for required field indicators
resources/views/auth/application/questions.blade.php Updated form validation and field requirements for application questions
resources/views/auth/application/files.blade.php Added asterisk indicators for required file upload fields
resources/views/auth/application/app.blade.php Changed button text from "Ellenőrzés" to "Áttekintés"
resources/lang/hu/user.php Added new translation keys for data editing restrictions
resources/lang/en/user.php Added new translation keys for data editing restrictions
database/migrations/*.php Made phone number and study line name fields nullable
app/Utils/HasRoles.php Added hasRoleOtherThanTenant method
app/Utils/ApplicationHandler.php Updated validation rules to allow nullable fields during partial filling
app/Policies/UserPolicy.php Added new authorization methods for different levels of user data editing
app/Models/EducationalInformation.php Changed isMasterAdmittee method visibility from private to public
app/Models/Application.php Refactored missingData method to provide more granular validation
app/Http/Controllers/Secretariat/UserController.php Updated authorization checks and validation logic throughout
app/Http/Controllers/Auth/ApplicationController.php Added automatic creation of personal and educational information records

Comment on lines 69 to +70
$old_profile->update(['path' => $path]);
Storage::delete($old_profile->path);
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old profile picture file is deleted after updating the database record with the new path. This creates a race condition where if the update fails, the old file is already deleted. The Storage::delete should be called before the update, or the operations should be wrapped in a transaction.

Suggested change
$old_profile->update(['path' => $path]);
Storage::delete($old_profile->path);
DB::transaction(function () use ($old_profile, $path) {
$old_profile->update(['path' => $path]);
Storage::delete($old_profile->path);
});

Copilot uses AI. Check for mistakes.

"level" => "bachelor",
"start" => Semester::current()->id,
"type" => "bachelor",
"start" => AlfonsoTest::autumnSemester()->id,
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a test helper method from a different test class (AlfonsoTest) creates tight coupling between test classes. Consider moving the autumnSemester method to a shared test utility class or creating the semester data directly in this test.

Copilot uses AI. Check for mistakes.

'end' => $studyLine["end"] ?? null,
]);
}
}

});

return redirect()->back()->with('message', __('general.successful_modification'));
if ($error_with_neptun_code) {
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The neptun code validation logic is complex and spans multiple lines with nested conditions. This could be extracted into a separate method to improve readability and testability.

Copilot uses AI. Check for mistakes.

required
:disabled="user()->cannot('editStaticPersonalInformation', $user)"
asterisk
maxlength="225"
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxlength value "225" appears to be inconsistent with the validation rule "max:225" in the controller, but more importantly, the standard maximum length for email addresses is typically 254 characters according to RFC specifications.

Suggested change
maxlength="225"
maxlength="254"

Copilot uses AI. Check for mistakes.

$validator['high_school'] = [$requiredForCollegist, 'string', 'max:255'];
$validator['year_of_graduation'] = [$requiredForCollegist, 'integer', 'between:1895,' . date('Y')];
$validator['year_of_acceptance'] = [$requiredForCollegist, 'integer', 'between:1895,' . date('Y')];
$validator['neptun'] = [$requiredForCollegist, 'string', 'size:6'];
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The neptun validation requires exactly 6 characters when the user is a collegist, but the HTML pattern in the view allows for empty strings (^$|^[A-Za-z0-9]{6}$). This creates an inconsistency between frontend and backend validation.

Suggested change
$validator['neptun'] = [$requiredForCollegist, 'string', 'size:6'];
$validator['neptun'] = [$requiredForCollegist, 'string', $requiredForCollegist === 'required' ? 'size:6' : 'nullable'];

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
resources/views/user/educational-information.blade.php (1)

47-68: Consider simplifying the complex conditional logic.

The Neptun code and educational email fields contain multiple conditional expressions that could benefit from refactoring for improved readability.

Consider extracting the conditional logic into computed variables:

+@php
+    $isApplicationContext = isset($application);
+    $emailHelper = $isApplicationContext 
+        ? 'lehetőleg @student.elte.hu-s (nem kötelező, a felvételit követően pótolható)'
+        : 'lehetőleg @student.elte.hu-s';
+@endphp
 <x-input.text s=6 id='educational-email' text='user.educational-email' name="email"
                 :value="$user->educationalInformation?->email"
-                :helper="
-                isset($application)
-                ?
-                    'lehetőleg @student.elte.hu-s (nem kötelező, a felvételit követően pótolható)'
-                :
-                    'lehetőleg @student.elte.hu-s'"
+                :helper="$emailHelper"
                 :required="$user->isCollegist()"
                 :disabled="user()->cannot('edit', $user)"
-                :asterisk="!isset($application)"
+                :asterisk="!$isApplicationContext"
                 maxlength="255"
                 />
app/Http/Controllers/Secretariat/UserController.php (1)

205-211: Thoughtful error handling for unique constraint violations.

The approach to handle Neptun code uniqueness violations by saving other data whilst flagging the error is user-friendly and prevents data loss. However, consider using more descriptive variable naming.

Consider using a more descriptive variable name:

-$error_with_neptun_code = false;
+$neptunCodeConflict = false;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4db35c2 and 0f26c99.

📒 Files selected for processing (28)
  • app/Http/Controllers/Auth/ApplicationController.php (1 hunks)
  • app/Http/Controllers/Secretariat/UserController.php (14 hunks)
  • app/Models/Application.php (1 hunks)
  • app/Policies/UserPolicy.php (2 hunks)
  • app/Utils/ApplicationHandler.php (2 hunks)
  • app/Utils/HasRoles.php (1 hunks)
  • database/migrations/2025_07_11_184933_make_phone_number_nullable.php (1 hunks)
  • database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (1 hunks)
  • resources/views/auth/application/app.blade.php (1 hunks)
  • resources/views/auth/application/files.blade.php (1 hunks)
  • resources/views/auth/application/questions.blade.php (6 hunks)
  • resources/views/components/input/checkbox.blade.php (1 hunks)
  • resources/views/components/input/datepicker.blade.php (2 hunks)
  • resources/views/components/input/file.blade.php (1 hunks)
  • resources/views/components/input/select.blade.php (1 hunks)
  • resources/views/components/input/text.blade.php (1 hunks)
  • resources/views/components/input/textarea.blade.php (1 hunks)
  • resources/views/components/input/timepicker.blade.php (1 hunks)
  • resources/views/user/alfonso-language-exams.blade.php (1 hunks)
  • resources/views/user/alfonso.blade.php (1 hunks)
  • resources/views/user/educational-information.blade.php (3 hunks)
  • resources/views/user/personal-information.blade.php (1 hunks)
  • resources/views/user/study-line-selector.blade.php (1 hunks)
  • resources/views/user/update_tenant_status.blade.php (1 hunks)
  • resources/views/utils/user/profile-picture.blade.php (3 hunks)
  • tests/Feature/ApplicationTest.php (6 hunks)
  • tests/Feature/UserTest.php (0 hunks)
  • tests/Unit/AlfonsoTest.php (1 hunks)
💤 Files with no reviewable changes (1)
  • tests/Feature/UserTest.php
✅ Files skipped from review due to trivial changes (1)
  • resources/views/components/input/timepicker.blade.php
🚧 Files skipped from review as they are similar to previous changes (24)
  • resources/views/auth/application/app.blade.php
  • resources/views/components/input/select.blade.php
  • resources/views/components/input/file.blade.php
  • resources/views/components/input/datepicker.blade.php
  • database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php
  • resources/views/user/update_tenant_status.blade.php
  • resources/views/components/input/checkbox.blade.php
  • database/migrations/2025_07_11_184933_make_phone_number_nullable.php
  • resources/views/auth/application/files.blade.php
  • resources/views/components/input/textarea.blade.php
  • resources/views/components/input/text.blade.php
  • app/Http/Controllers/Auth/ApplicationController.php
  • resources/views/user/study-line-selector.blade.php
  • resources/views/utils/user/profile-picture.blade.php
  • resources/views/user/alfonso-language-exams.blade.php
  • app/Utils/HasRoles.php
  • tests/Unit/AlfonsoTest.php
  • resources/views/user/alfonso.blade.php
  • app/Utils/ApplicationHandler.php
  • resources/views/user/personal-information.blade.php
  • resources/views/auth/application/questions.blade.php
  • app/Models/Application.php
  • tests/Feature/ApplicationTest.php
  • app/Policies/UserPolicy.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: horcsinbalint
PR: EotvosCollegium/mars#743
File: app/Http/Controllers/Auth/AdmissionController.php:237-237
Timestamp: 2025-07-08T22:59:53.901Z
Learning: When horcsinbalint defers issues for "human reviewers to check later," he's making a conscious decision to prioritize the current PR over fixing secondary issues, and this should be acknowledged rather than being insistent about immediate fixes.
🧬 Code Graph Analysis (1)
app/Http/Controllers/Secretariat/UserController.php (9)
app/Models/EducationalInformation.php (4)
  • user (83-86)
  • EducationalInformation (65-240)
  • studyLines (91-94)
  • languageExams (100-103)
app/helpers.php (1)
  • user (15-23)
app/Models/RoleUser.php (3)
  • user (62-65)
  • role (70-73)
  • object (54-57)
app/Http/Controllers/StudentsCouncil/EpistolaController.php (1)
  • delete (59-64)
app/Models/User.php (8)
  • isCollegist (753-770)
  • roles (218-222)
  • isTenant (849-852)
  • personalInformation (228-231)
  • internetAccess (370-373)
  • workshops (264-267)
  • faculties (305-308)
  • educationalInformation (237-240)
app/Models/Role.php (1)
  • alumni (293-296)
app/Rules/SameOrUnique.php (1)
  • SameOrUnique (10-63)
app/Models/LanguageExam.php (1)
  • educationalInformation (60-63)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (1)
  • store (162-246)
🔇 Additional comments (22)
resources/views/user/educational-information.blade.php (7)

3-19: Well-structured permission-based messaging system.

The conditional rendering of informational messages based on user permissions is implemented correctly. The distinction between full edit restrictions and partial edit restrictions provides clear user feedback whilst maintaining consistent email contact information across both scenarios.


28-32: Proper implementation of dynamic form attributes.

The high school input field correctly incorporates permission-based disabled states and required field indicators. The use of user()->cannot('editStaticEducationalInformation', $user) aligns with the authorization policy changes.


36-46: Appropriate conditional logic for year fields.

The year of acceptance field properly uses $user->isCollegist(alumni: true) || isset($user->application) for the required attribute, which correctly handles both collegist users and application scenarios.


71-84: Consistent permission implementation for faculty selection.

The faculty checkbox section correctly implements permission-based disabling and includes proper required field indicators. The use of user()->cannot('edit', $user) is consistent with the authorization model.


87-103: Appropriate conditional rendering for workshop selection.

The workshop section correctly uses !isset($application) to hide workshops during application processes and applies editStaticEducationalInformation permission checks appropriately.


105-111: Proper authorization for study line management.

The study line section correctly uses @can('edit', $user) to control access to the add study line button, ensuring users without edit permissions cannot modify this data.


117-133: Consistent permission handling for textarea fields and save button.

The research topics and extra information textareas, along with the save button, correctly implement permission-based disabling and visibility controls. The consistent use of user()->cannot('edit', $user) maintains proper authorization boundaries.

app/Http/Controllers/Secretariat/UserController.php (15)

60-60: Properly elevated authorization requirement.

The change from 'view' to 'edit' authorization for profile picture management is correct and aligns with the principle of least privilege. Users should only modify profile pictures if they have edit permissions.


85-85: Consistent authorization enhancement.

The authorization check for deleting profile pictures correctly uses 'edit' permission, maintaining consistency with the store method and proper security boundaries.


105-130: Excellent implementation of dynamic validation logic.

The conditional validation rule building based on user permissions is well-structured. The use of user()->can('editStaticPersonalInformation', $user) properly restricts validation of sensitive fields whilst allowing other fields to be validated for all users with edit permissions.


142-145: Proper array key existence check.

The use of array_key_exists('tenant_until', $data) correctly handles the conditional update of tenant information, ensuring the field is only processed when present in the validated data.


159-192: Well-structured dynamic validation for educational information.

The educational information validation logic correctly separates static fields (requiring special permissions) from dynamic fields (requiring only edit permissions). The progressive building of validation rules ensures proper authorization whilst maintaining functionality.


213-244: Robust transaction handling for data consistency.

The use of DB transactions for educational information updates is excellent practice, ensuring data integrity across multiple related operations. The proper handling of study lines with null coalescing operators prevents undefined index errors.


222-228: Appropriate conditional data synchronization.

The conditional synchronization of workshop and faculty data using array_key_exists ensures that these relationships are only updated when the corresponding data is present in the request, preventing unintended data removal.


230-242: Proper study line management with null safety.

The study line creation logic correctly handles potential null values using the null coalescing operator, preventing undefined index errors whilst maintaining data integrity.


246-250: User-friendly error handling with appropriate messaging.

The conditional return logic provides clear feedback to users about Neptun code conflicts whilst allowing successful updates to proceed normally. The error message appropriately suggests contacting administrators for resolution.


263-271: Appropriate authorization for Alfonso status updates.

The use of editStaticEducationalInformation permission for Alfonso status updates is correct, as this information is considered static educational data. The simplified validation approach is appropriate for this specific use case.


286-304: Consistent permission enforcement for language exam management.

The language exam upload method correctly uses 'edit' permission and validates file uploads appropriately. The direct use of validated data in the creation method is clean and secure.


318-318: Proper authorization for language exam deletion.

The 'edit' permission requirement for deleting language exams is appropriate and maintains consistency with the upload functionality.


341-349: Appropriate authorization and validation for tenant updates.

The tenant until date update correctly uses 'edit' permission and validates the date range appropriately. The synchronization with internet access dates maintains data consistency.


422-422: Consistent authorization pattern for role management.

The use of $this->authorize() for role addition maintains consistency with Laravel's authorization patterns and properly enforces permission checks.


445-445: Proper authorization for role removal.

The authorization check for role removal correctly uses the same pattern as role addition, ensuring consistent permission enforcement across role management operations.

@horcsinbalint horcsinbalint marked this pull request as ready for review July 15, 2025 22:21
horcsinbalint and others added 5 commits July 19, 2025 19:10
This commit also disables editing for applicants who have already
submitted their application / when the application deadline has passed.
It also disables some educational information that rarely changes for a
user.
This commit fixes the style issues introduced in 5271cb1 according to the output
from PHP CS Fixer.

Details: #746
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/Models/Application.php (1)

356-360: Consistent implementation pattern with potential access concern.

The educational information validation follows the same pattern as personal information. The same concern applies here regarding array versus object property access for the $educationalInformation model instance.

🧹 Nitpick comments (1)
app/Models/StudyLine.php (1)

15-15: Consider updating PHPDoc to reflect nullable name property.

The documentation declares @property string $name, but given the database migration has made this field nullable, it would be more accurate to document it as @property string|null $name for consistency.

- * @property string $name
+ * @property string|null $name
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f26c99 and a0ca394.

📒 Files selected for processing (34)
  • app/Http/Controllers/Auth/ApplicationController.php (2 hunks)
  • app/Http/Controllers/Secretariat/UserController.php (14 hunks)
  • app/Livewire/ParentChildForm.php (1 hunks)
  • app/Models/Application.php (1 hunks)
  • app/Models/EducationalInformation.php (1 hunks)
  • app/Models/StudyLine.php (1 hunks)
  • app/Policies/UserPolicy.php (2 hunks)
  • app/Utils/ApplicationHandler.php (2 hunks)
  • app/Utils/HasRoles.php (1 hunks)
  • database/migrations/2025_07_11_184933_make_phone_number_nullable.php (1 hunks)
  • database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php (1 hunks)
  • resources/lang/en/user.php (2 hunks)
  • resources/lang/hu/user.php (2 hunks)
  • resources/views/auth/application/app.blade.php (1 hunks)
  • resources/views/auth/application/files.blade.php (1 hunks)
  • resources/views/auth/application/questions.blade.php (6 hunks)
  • resources/views/components/input/checkbox.blade.php (1 hunks)
  • resources/views/components/input/datepicker.blade.php (2 hunks)
  • resources/views/components/input/file.blade.php (1 hunks)
  • resources/views/components/input/select.blade.php (1 hunks)
  • resources/views/components/input/text.blade.php (1 hunks)
  • resources/views/components/input/textarea.blade.php (1 hunks)
  • resources/views/components/input/timepicker.blade.php (1 hunks)
  • resources/views/livewire/parent-child-form.blade.php (1 hunks)
  • resources/views/user/alfonso-language-exams.blade.php (1 hunks)
  • resources/views/user/alfonso.blade.php (1 hunks)
  • resources/views/user/educational-information.blade.php (3 hunks)
  • resources/views/user/personal-information.blade.php (1 hunks)
  • resources/views/user/study-line-selector.blade.php (1 hunks)
  • resources/views/user/update_tenant_status.blade.php (1 hunks)
  • resources/views/utils/user/profile-picture.blade.php (3 hunks)
  • tests/Feature/ApplicationTest.php (6 hunks)
  • tests/Feature/UserTest.php (0 hunks)
  • tests/Unit/AlfonsoTest.php (1 hunks)
💤 Files with no reviewable changes (1)
  • tests/Feature/UserTest.php
✅ Files skipped from review due to trivial changes (2)
  • resources/views/user/update_tenant_status.blade.php
  • resources/views/components/input/checkbox.blade.php
🚧 Files skipped from review as they are similar to previous changes (27)
  • resources/views/auth/application/app.blade.php
  • resources/views/components/input/file.blade.php
  • resources/views/components/input/select.blade.php
  • tests/Unit/AlfonsoTest.php
  • resources/views/auth/application/files.blade.php
  • resources/views/components/input/datepicker.blade.php
  • resources/views/components/input/timepicker.blade.php
  • database/migrations/2025_07_11_202220_make_fields_in_study_lines_nullable.php
  • app/Utils/HasRoles.php
  • resources/lang/en/user.php
  • database/migrations/2025_07_11_184933_make_phone_number_nullable.php
  • resources/views/components/input/text.blade.php
  • resources/views/user/alfonso-language-exams.blade.php
  • resources/views/utils/user/profile-picture.blade.php
  • app/Models/EducationalInformation.php
  • resources/views/components/input/textarea.blade.php
  • resources/views/user/alfonso.blade.php
  • resources/views/user/study-line-selector.blade.php
  • app/Http/Controllers/Auth/ApplicationController.php
  • resources/views/auth/application/questions.blade.php
  • resources/lang/hu/user.php
  • app/Policies/UserPolicy.php
  • resources/views/user/personal-information.blade.php
  • resources/views/user/educational-information.blade.php
  • tests/Feature/ApplicationTest.php
  • app/Utils/ApplicationHandler.php
  • app/Http/Controllers/Secretariat/UserController.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: horcsinbalint
PR: EotvosCollegium/mars#743
File: app/Http/Controllers/Auth/AdmissionController.php:237-237
Timestamp: 2025-07-08T22:59:53.901Z
Learning: When horcsinbalint defers issues for "human reviewers to check later," he's making a conscious decision to prioritize the current PR over fixing secondary issues, and this should be acknowledged rather than being insistent about immediate fixes.
🧬 Code Graph Analysis (1)
app/Models/Application.php (3)
app/Models/EducationalInformation.php (3)
  • user (83-86)
  • studyLines (91-94)
  • alfonsoExempted (158-162)
app/helpers.php (1)
  • user (15-23)
app/Models/User.php (3)
  • personalInformation (228-231)
  • educationalInformation (237-240)
  • faculties (305-308)
🔇 Additional comments (9)
app/Models/StudyLine.php (1)

101-101: Excellent defensive programming approach for nullable field handling.

The null coalescing operator ensures the method maintains its string return type contract whilst gracefully handling the newly nullable name database column. This is a sound approach that prevents potential null return values.

app/Models/Application.php (6)

344-349: Excellent implementation of granular user validation.

The explicit validation of user name and email represents a significant improvement over previous broad checks. The implementation correctly validates these fundamental user attributes whilst maintaining proper null safety.


362-364: Proper relationship validation implementation.

The faculty count validation correctly utilises the Eloquent relationship method to verify that at least one faculty has been selected. This represents sound Laravel practice.


367-384: Comprehensive study line validation with proper defensive programming.

The implementation demonstrates excellent defensive programming practices by first checking for the existence of educational information and study lines before iterating. The individual field validation for each study line (name, type, training_code, start) provides the granular validation mentioned in the PR objectives.


385-392: Excellent conditional validation with proper exemption handling.

The ALFONSÓ language validation correctly implements conditional logic that only applies when year_of_acceptance is set and the user is not exempted. The utilisation of the alfonsoExempted() method demonstrates proper separation of concerns and maintainable code architecture.


394-414: Enhanced categorisation improves user experience.

The addition of the "Szakmai és motivációs kérdések:" prefix to these validation messages significantly improves the user experience by clearly categorising the missing data. This aligns perfectly with the PR's objective of providing more detailed validation feedback.


416-418: Proper positioning of file validation requirement.

Moving the file count validation to the conclusion of the method ensures that users receive comprehensive feedback about all missing data types before addressing file upload requirements. This represents sound UX design principles.

app/Livewire/ParentChildForm.php (1)

18-18: Well-implemented attribute customisation property.

I say, this is a rather sensible addition that provides excellent flexibility for input customisation. The default value is quite appropriate, and the documentation is spot on.

resources/views/livewire/parent-child-form.blade.php (1)

21-29: Critical security concern: Unescaped attribute output poses XSS risk.

I must say, whilst the restructuring is rather clever, there's a rather serious security vulnerability here. The template is outputting both attribute names and values without proper escaping, which could lead to XSS attacks if malicious content is provided via $inputAttributes.

Apply this diff to properly escape the attribute output:

-                        <input
-                            class="validate parent-child parent-child-input"
-                            @foreach ($inputAttributes as $attrName => $attrValue)
-                                {{ $attrName }}="{{ $attrValue }}"
-                            @endforeach
-
-                            name="{{ $name }}[]"
-                            wire:model.live="items.{{ $index }}"
-                        >
+                        <input
+                            class="validate parent-child parent-child-input"
+                            @foreach ($inputAttributes as $attrName => $attrValue)
+                                {!! e($attrName) !!}="{!! e($attrValue) !!}"
+                            @endforeach
+
+                            name="{{ $name }}[]"
+                            wire:model.live="items.{{ $index }}"
+                        >

Alternatively, consider using Laravel's Html::attributes() helper if available, or implement server-side validation to ensure only safe attribute names and values are permitted.

Likely an incorrect or invalid review comment.

Comment on lines +350 to 355
foreach (['place_of_birth', 'date_of_birth', 'mothers_name', 'phone_number',
'country', 'county', 'zip_code', 'city', 'street_and_number'] as $personal_info_field) {
if (!isset($personalInformation) || !isset($personalInformation[$personal_info_field])) {
$missingData[] = "Személyes adat: ".strtolower(__('user.'.$personal_info_field));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify array access pattern for personal information fields.

The validation logic appears sound, however, I must raise a concern regarding the array access pattern. The condition !isset($personalInformation[$personal_info_field]) suggests that $personalInformation is being treated as an array, yet the model relationship indicates it should be a PersonalInformation object instance.

Please run this verification script to confirm the expected data structure:


🏁 Script executed:

#!/bin/bash
# Description: Verify how PersonalInformation model properties are accessed in the codebase
# Expected: Should reveal whether object property access or array access is the correct pattern

echo "=== Searching for PersonalInformation property access patterns ==="
rg -A 3 -B 1 'personalInformation\->' --type php

echo "=== Searching for PersonalInformation array access patterns ==="
rg -A 3 -B 1 'personalInformation\[' --type php

echo "=== Checking PersonalInformation model definition ==="
cat app/Models/PersonalInformation.php | head -50

Length of output: 6992


Please replace array-style access with object property access on the PersonalInformation model

To keep consistency with Eloquent models (and the rest of the codebase), PersonalInformation should be treated as an object, not an array. In app/Models/Application.php around lines 350–355, update the check to use property access:

• File: app/Models/Application.php
Lines: ~350–355

Diff:

-        foreach (['place_of_birth', 'date_of_birth', 'mothers_name', 'phone_number',
-                 'country', 'county', 'zip_code', 'city', 'street_and_number'] as $personal_info_field) {
-            if (!isset($personalInformation) || !isset($personalInformation[$personal_info_field])) {
-                $missingData[] = "Személyes adat: ".strtolower(__('user.'.$personal_info_field));
-            }
-        }
+        foreach (['place_of_birth', 'date_of_birth', 'mothers_name', 'phone_number',
+                 'country', 'county', 'zip_code', 'city', 'street_and_number'] as $personal_info_field) {
+            if (! isset($personalInformation)
+                || ! isset($personalInformation->{$personal_info_field})
+            ) {
+                $missingData[] = "Személyes adat: "
+                    .strtolower(__('user.'.$personal_info_field));
+            }
+        }

This ensures you’re checking the model’s attributes via object properties rather than array offsets.

📝 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.

Suggested change
foreach (['place_of_birth', 'date_of_birth', 'mothers_name', 'phone_number',
'country', 'county', 'zip_code', 'city', 'street_and_number'] as $personal_info_field) {
if (!isset($personalInformation) || !isset($personalInformation[$personal_info_field])) {
$missingData[] = "Személyes adat: ".strtolower(__('user.'.$personal_info_field));
}
}
foreach (['place_of_birth', 'date_of_birth', 'mothers_name', 'phone_number',
'country', 'county', 'zip_code', 'city', 'street_and_number'] as $personal_info_field) {
if (! isset($personalInformation)
|| ! isset($personalInformation->{$personal_info_field})
) {
$missingData[] = "Személyes adat: "
.strtolower(__('user.'.$personal_info_field));
}
}
🤖 Prompt for AI Agents
In app/Models/Application.php around lines 350 to 355, the code currently
accesses the PersonalInformation model attributes using array syntax, which is
inconsistent with Eloquent model usage. Update the condition to check properties
using object property access (e.g., $personalInformation->place_of_birth)
instead of array keys. This change aligns with the rest of the codebase and
proper Eloquent model conventions.

This could theoretically allow applicants (but not verified users) to
change their acceptance year. However, we supervise applications
manually and they'd have to bypass the disabled input field on the front
end, so this is not a huge issue.
@horcsinbalint horcsinbalint merged commit 7c1ccd8 into development Jul 19, 2025
4 of 5 checks passed
horcsinbalint pushed a commit that referenced this pull request Jul 19, 2025
This commit fixes the style issues introduced in 5271cb1 according to the output
from PHP CS Fixer.

Details: #746
@horcsinbalint horcsinbalint deleted the partial-forms2 branch July 19, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine userpolicy
2 participants