-
Notifications
You must be signed in to change notification settings - Fork 15
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
Finalise application process #618
Conversation
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe recent changes significantly enhance the functionality of the admission management system. Key improvements include the introduction of the Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Controller as AdmissionController
participant View as Finalization View
participant Model as Application Model
participant Database
Admin->>Controller: Access /admission/finalize
Controller->>Model: Fetch admitted and not admitted applications
Model-->>Controller: Return applications data
Controller->>View: Render finalization view with applications
View->>Admin: Display applications for finalization
Admin->>View: Submit finalization form
View->>Controller: POST finalize data
Controller->>Model: Update application statuses
Model->>Database: Soft delete not admitted applications
Database-->>Model: Confirm updates
Controller-->>View: Return success message
View->>Admin: Display success notification
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
resources/views/auth/application/role_status_update_component.blade.php (1)
1-17
: Consider improving accessibility and semantics.The current HTML structure lacks semantic tags and accessibility attributes. Consider using more semantic HTML elements and adding ARIA roles or labels for better accessibility.
- <div style="display: flex;flex-direction: column;align-items: center;justify-content: center;"> + <section style="display: flex;flex-direction: column;align-items: center;justify-content: center;" role="region" aria-label="Role Status Update">app/Livewire/ApplicationRoleStatusUpdate.php (1)
40-43
: Clarify the purpose of theupdated
property.The
getUpdatedProperty
method determines if thelastUpdated
time is within the last two seconds. Ensure this logic is necessary and correctly implemented.Consider renaming the method to
isRecentlyUpdated
for clarity.- public function getUpdatedProperty() + public function isRecentlyUpdated()
database/migrations/2024_07_23_195958_add_fields_to_applications_table.php
Show resolved
Hide resolved
->with('application') | ||
->whereIn('id', $not_admitted->pluck('user_id')) | ||
//ignore users with any existing role | ||
->whereDoesntHave('roles') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: At the moment, even newly registered applicants get the printer role by default, so I don't think this will end up deleting anybody as-is. (last year's applicants are still present in the users
table too)
$user->roles()->attach(Role::get(Role::PRINTER)->id); |
Probably easier to jump into the database and fix this manually, rather than changing the code here to handle this quirk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, true... But this won't be the case after #394 has been merged.
But even before that we can just delete that role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; we don't need the printer role. No one can print without paying for admins before, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a seeded applicant did not get workshops nor educational information; is this specific to seeded users or a general problem? I think at least workshops should not be a problem.
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now:) one more small change and we can go ahead, I think.
@coderabbitai review |
This PR implements the missing finalise applications feature.
It's on a new page now where the final names and statistics can be seen.
It is also possible to change who has been admitted as resident or extern.