Decouple periodic events from controller names #555
Conversation
|
Warning Rate limit exceeded@kdmnk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 40 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
📝 Walkthrough📝 WalkthroughWalkthroughThis update involves a significant refactoring of the codebase, wherein key controllers now extend Changes
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
app/Utils/PeriodicEventController.php (1)
Line range hint
38-50: Ensure that the constructor correctly initializes theperiodicEventNameand that theperiodicEventmethod accurately retrieves the latest event. Consider adding more robust error handling for cases where the event might not be found.Consider improving error handling in the
periodicEventmethod to gracefully handle cases where no events are found.app/Http/Controllers/Secretariat/SemesterEvaluationController.php (1)
Line range hint
91-161: Thestore()method's extensive validation rules are appropriate for ensuring data integrity. However, the method's complexity could be reduced by breaking it down into smaller methods, each handling a specific part of the form's processing. This would improve readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (1)
25-26: I say, a rather clever implementation in the constructor!The call to the parent constructor with
PeriodicEvent::SEMESTER_EVALUATION_PERIODis spot on. It's a fine way to implement the periodic event handling, I must say.However, if I may be so bold, I'd suggest a slight refinement to the comment. Perhaps something along these lines:
- //use the same period as the Semester Evaluation. + // Use the same period as the Semester Evaluation for consistency.This minor adjustment provides a smidge more context, explaining why we're using this particular period. What do you think?
app/Models/User.php (1)
Line range hint
1-567: Consider adding type hints to method parameters and return types.While reviewing the file, I noticed that many method parameters and return types lack type hints. Adding these would improve code readability and catch potential type-related errors early.
For example, in the
scopeDoesntHaveStatusFormethod, we could add type hints like this:- public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) + public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester): BuilderConsider applying similar type hints throughout the class where appropriate.
app/Http/Controllers/Auth/ApplicationController.php (1)
42-45: Enhance the method's PHPDoc commentsMay I suggest enriching the method's PHPDoc comments by including parameter and return type annotations? This will improve readability and provide better support in IDEs.
Proposed addition:
/** * Update the PeriodicEvent connected to the applications. * * @param Request $request * @return RedirectResponse * @throws AuthorizationException */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/Http/Controllers/Auth/AdmissionController.php (3 hunks)
- app/Http/Controllers/Auth/ApplicationController.php (1 hunks)
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php (4 hunks)
- app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (1 hunks)
- app/Mail/EvaluationFormReminder.php (1 hunks)
- app/Models/User.php (2 hunks)
- tests/Feature/ApplicationTest.php (1 hunks)
🧰 Additional context used
🪛 phpstan
app/Http/Controllers/Auth/ApplicationController.php
48-48: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🔇 Additional comments (16)
app/Mail/EvaluationFormReminder.php (1)
22-24: 🛠️ Refactor suggestionSimplified constructor with potential implications.
I say, the changes to the constructor appear to be a step towards a more straightforward and robust implementation. The removal of the
$countparameter and the simplification of the constructor body suggest a refinement of the class's responsibilities. Making the$deadlineparameter non-nullable is a jolly good move, as it enforces the presence of a deadline and prevents potential null-related issues.However, we must consider the following:
- Existing code that utilizes this class may need to be updated to accommodate the new constructor signature.
- The removal of the
$countparameter might affect functionality elsewhere in the system that previously relied on this information.To ensure a smooth transition, I recommend the following actions:
- Update all instantiations of
EvaluationFormReminderthroughout the codebase to match the new constructor signature.- Review any code that previously utilized the
$countparameter to ensure it's not adversely affected by its removal.- Consider adding a comment explaining the rationale behind these changes for future maintainers.
To verify the impact of these changes, we can run the following script:
This script will help identify areas of the codebase that may need attention due to these changes.
✅ Verification successful
EvaluationFormReminder Constructor Changes Verified
The recent modifications to the
EvaluationFormReminderconstructor have been successfully implemented. The removal of the$countparameter and the adjustment to the$deadlineparameter are reflected accurately in the codebase. No usages of the$countparameter were found, indicating that its removal does not affect existing functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of EvaluationFormReminder and potential issues # Search for instantiations of EvaluationFormReminder echo "Searching for EvaluationFormReminder instantiations:" rg --type php "new EvaluationFormReminder\(" -A 2 # Search for potential usages of a 'count' variable in relation to EvaluationFormReminder echo "\nSearching for potential 'count' usages:" rg --type php "EvaluationFormReminder.*count" -B 2 -A 2Length of output: 640
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (3)
5-6: Splendid additions to the use statements, I must say!The inclusion of
PeriodicEventandPeriodicEventControlleris most appropriate given the class refactoring. Furthermore, the explicit import ofValidatorenhances code clarity. Well done, indeed!Also applies to: 9-9
Line range hint
1-214: A word of caution regarding the broader implications, if I may.While the changes we've reviewed are most certainly on the right track, I feel compelled to bring to your attention a couple of matters that warrant further consideration:
The removal of the
HasPeriodicEventtrait suggests that this functionality is now provided by our parent class. Splendid! However, we ought to ensure that all the methods previously provided by the trait are indeed available through our new inheritance structure.The bulk of our class methods remain unchanged. It might be prudent to review these methods to ensure they align harmoniously with our new inheritance structure. Perhaps some of these could benefit from leveraging the new capabilities we've inherited?
Might I suggest a thorough testing of the class functionality? Additionally, consider if any of our unchanged methods could be refactored to make better use of our new periodic event handling capabilities.
Let's run a quick check to identify potential refactoring opportunities:
#!/bin/bash # Description: Identify methods that might benefit from refactoring # Test: Search for methods that might use periodic event functionality rg --type php -n '(function\s+\w+\s*\([^)]*\)\s*\{)' app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php | while read -r line; do line_number=$(echo "$line" | cut -d':' -f1) method_name=$(echo "$line" | sed -E 's/.*function\s+(\w+).*/\1/') echo "Checking method $method_name at line $line_number" rg --type php -n -C 10 "(periodicEventForSemester|getPeriodicEvent|setPeriodicEvent)" -g "!PeriodicEventController.php" "app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php:$line_number" doneThis script will help us identify methods that might benefit from using our new periodic event functionality.
21-21: A most excellent modification to the class inheritance, I dare say!The change to extend
PeriodicEventControlleris perfectly aligned with the objective of decoupling periodic events from controller names. Jolly good show!However, might I suggest we verify the proper utilisation of the inherited methods? It would be prudent to ensure we're making the most of this new inheritance structure.
Let's run a quick check to see how we're using these inherited methods:
tests/Feature/ApplicationTest.php (1)
Line range hint
1-524: Recommendation for a comprehensive review of affected tests.While the change in the
setUpmethod appears to be isolated, I believe it would be prudent to conduct a thorough examination of all tests that might rely on theAPPLICATION_PERIODevent. This ensures that we maintain consistency across our test suite and prevents any potential issues that might arise from this modification.Might I suggest running the following script to identify tests that might need updating?
#!/bin/bash # Description: Identify tests potentially affected by the APPLICATION_PERIOD event change # Test: Search for occurrences of 'APPLICATION_PERIOD' in test files rg --type php -g 'tests/**/*Test.php' -e "APPLICATION_PERIOD" # Test: Search for PeriodicEvent creations in test files rg --type php -g 'tests/**/*Test.php' -e "PeriodicEvent::create"This script should help identify any tests that might need attention due to the recent changes.
app/Models/User.php (1)
Line range hint
542-546: Improved flexibility in thescopeActivemethod.The modification to allow a nullable
$semester_idparameter enhances the method's versatility. This change aligns well with the PR objective of decoupling periodic events from controller names.app/Http/Controllers/Auth/ApplicationController.php (1)
46-65:⚠️ Potential issueVerify the existence of the
ApplicationFormclassThe method
updateApplicationPeriodreferencesApplicationForm::class, but static analysis tools indicate thatApp\Models\ApplicationFormmay not be found. Kindly ensure that theApplicationFormclass exists within theApp\Modelsnamespace and is correctly imported.Run the following script to check for the existence of the
ApplicationFormclass:🧰 Tools
🪛 phpstan
48-48: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (5)
25-25: ExtendingPeriodicEventControllerenhances code reusabilityBy extending
PeriodicEventController, theSemesterEvaluationControllernow benefits from shared functionality related to periodic events, promoting consistency and reducing code duplication.
27-30: Constructor correctly initialises the parent controllerThe constructor appropriately calls the parent constructor with
PeriodicEvent::SEMESTER_EVALUATION_PERIOD, ensuring the correct periodic event type is set for this controller.
68-68: Return type declaration improves code clarityAdding the return type
Viewto theshow()method enhances readability and assists with static analysis by clearly indicating the method's expected return type.
157-160: Review the suppression of events when updating rolesUsing
RoleUser::withoutEventsto modify user roles bypasses any model events that would normally be dispatched. This might prevent essential side-effects such as audit logging, cache invalidation, or notification triggers. Please verify that suppressing these events will not lead to unintended consequences.To investigate where
RoleUserevents are utilised, you can execute:#!/bin/bash # Description: Identify event listeners associated with `RoleUser` model events. # Test: Search for event listeners or observers for `RoleUser` events. rg --type php -A 5 -B 2 'Event::listen\(|public function (created|updated|deleted)\(' app/
79-79:⚠️ Potential issueHandle potential null value from
self::semester()Ensure that
self::semester()cannot returnnullbefore callingsucc(). If there's a possibility of it beingnull, invokingsucc()will result in a runtime error.Consider modifying the code to include a null check:
-'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) ? User::doesntHaveStatusFor(self::semester()->succ())->get() : null, +'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) && self::semester() ? User::doesntHaveStatusFor(self::semester()->succ())->get() : null,Likely invalid or redundant comment.
app/Http/Controllers/Auth/AdmissionController.php (4)
20-20: Verify the impact of changing the parent class toPeriodicEventController.By extending
PeriodicEventController, please ensure that all required functionalities are maintained. Confirm that any methods or properties previously inherited fromControllerare still accessible or have been appropriately overridden.
37-37: Ensure compatibility with the new inheritance structure.Changing the parent class may affect the behaviour of
AdmissionController. Kindly verify that all controller actions continue to function as intended under the new inheritance.
43-43: Validate the initialisation of the parent constructor with the correct event.The call to
parent::__construct(PeriodicEvent::APPLICATION_PERIOD);initialises the parent class with a specific periodic event. Please confirm thatPeriodicEvent::APPLICATION_PERIODaccurately represents the desired event for this controller and aligns with the application's logic.
11-11: Confirm the necessity of importingPeriodicEvent.The
PeriodicEventmodel is being imported. Kindly verify that it is utilised within this controller to ensure all imports are essential.Please run the following script to check for usages of
PeriodicEventbeyond the import statement:✅ Verification successful
Import of
PeriodicEventis necessary.The
PeriodicEventmodel is utilised withinAdmissionController.phpas follows:
- The controller extends
PeriodicEventController.PeriodicEvent::APPLICATION_PERIODis used in the constructor.- The method
updatePeriodicEventis invoked.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'PeriodicEvent' in AdmissionController.php beyond the import line. # Expected result: Lines where 'PeriodicEvent' is used after line 11. grep -n 'PeriodicEvent' app/Http/Controllers/Auth/AdmissionController.php | awk -F':' '$1 > 11 { print }'Length of output: 378
| 'faculties' => Faculty::all(), | ||
| 'workshops' => Workshop::all(), | ||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', Semester::current()->id)->first(), | ||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', self::semester()->id)->first(), |
There was a problem hiding this comment.
💡 Codebase verification
Static semester() Method Not Found
- The
semester()method is not defined withinSemesterEvaluationControlleror its parent classes.
🔗 Analysis chain
Verify accessibility of self::semester() method
The use of self::semester() assumes that a static semester() method is available within the class hierarchy. Please ensure that this method exists and returns the expected Semester instance.
To confirm the existence and accessibility of the semester() method, you may run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the definition of the static method 'semester'.
# Test: Search for the 'static function semester()' declaration.
rg --type php -A 2 -B 2 'static function semester\(\)' app/
Length of output: 6556
There was a problem hiding this comment.
@kdmnk this is indeed true; what about this?
where('semester_id', $this->periodicEvent()?->semester()->id ?? Semester::current())There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
self::semester() is defined in parent class. I made it nullable though. Semester::current() might introduce confusion again.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/Http/Controllers/Auth/ApplicationController.php (1)
45-68: A jolly good implementation of theupdateApplicationPeriodmethod!This new method is a fine addition to the controller. It properly implements the functionality described in the PR objectives, with commendable attention to authorization and input validation. The use of Carbon for date parsing is spot on.
However, I do have one small suggestion for improvement. It would be prudent to wrap the
updatePeriodicEventcall in a try-catch block to handle any potential exceptions that might arise during the update process. This would allow for more graceful error handling and user feedback.Here's a suggested modification:
try { $this->updatePeriodicEvent( Semester::find($request->semester_id), Carbon::parse($request->start_date), Carbon::parse($request->end_date), $request->extended_end_date ? Carbon::parse($request->extended_end_date) : null ); return back()->with('message', __('general.successful_modification')); } catch (\Exception $e) { return back()->with('error', __('general.error_occurred')); }This change would ensure that any unexpected errors are caught and communicated to the user in a friendly manner. What do you say to that?
🧰 Tools
🪛 phpstan
51-51: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🪛 GitHub Check: phpunit
[failure] 51-51:
Class App\Models\ApplicationForm not found.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/Http/Controllers/Auth/ApplicationController.php (2 hunks)
- tests/Feature/AdmissionTest.php (1 hunks)
🧰 Additional context used
🪛 phpstan
app/Http/Controllers/Auth/ApplicationController.php
51-51: Class App\Models\ApplicationForm not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(class.notFound)
🪛 GitHub Check: phpunit
app/Http/Controllers/Auth/ApplicationController.php
[failure] 51-51:
Class App\Models\ApplicationForm not found.
🔇 Additional comments (3)
app/Http/Controllers/Auth/ApplicationController.php (3)
5-25: Splendid additions to the import statements, I must say!The new imports are quite fitting for the changes made to the class. The inclusion of
PeriodicEventController,Carbon, andAuthorizationExceptionis spot on for the new functionality. The reordering of imports, while not affecting functionality, does make for a more orderly appearance. Jolly good show!
30-30: A most excellent modification to the class declaration!The change to extend
PeriodicEventControlleris precisely what we needed to decouple periodic events from controller names. This inheritance will provide the class with all the necessary functionality for managing periodic events. Well done, indeed!
40-43: A most appropriate addition of a constructor, I must say!The new constructor is a splendid touch. It properly initializes the parent class with the
APPLICATION_PERIODconstant, ensuring that this controller is associated with the correct periodic event type. This is in perfect harmony with the changes made to the class declaration. Jolly good implementation!
kdmnk
force-pushed
the
decouple-periodic-events
branch
from
95b162e to
8f66c0c
Compare
viktorcsimma
left a comment
viktorcsimma
left a comment
There was a problem hiding this comment.
Sorry for being so late with reviewing this. Seems to be a nice change; more understandable, clear and useful:)
Besides the things I have written (for me, the semester evaluation page crashed because of self::semester(), there was one more thing, seen when I switched to another periodic event:
It's only 21:48, but it is still open. Maybe my fix was incorrect.
Also, can you check out CodeRabbit comments as well?
| switch ($this->event_name) { | ||
| case self::SEMESTER_EVALUATION_PERIOD: | ||
| event(new SemesterEvaluationPeriodStart($this)); | ||
| break; |
There was a problem hiding this comment.
This seems not to be open-closed; can we circumvent this somehow? Maybe through singletons derived from this class.
There was a problem hiding this comment.
Yeah I remember that I was trying a lot of stuff here. We can make this more fancier but I don't see the point of making this more complex, and with registers or whatever else we still need to do add some code manually (it was calculated automatically before, but now the point of this is to decouple the names from controllers, so the mapping has to be manual somewhere), just in a more abstracted, harder to find place.
But I'm curious to see if you find a better way to structure this, or move it to somewhere that makes more sense to you.
| 'faculties' => Faculty::all(), | ||
| 'workshops' => Workshop::all(), | ||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', Semester::current()->id)->first(), | ||
| 'evaluation' => user()->semesterEvaluations()->where('semester_id', self::semester()->id)->first(), |
There was a problem hiding this comment.
@kdmnk this is indeed true; what about this?
where('semester_id', $this->periodicEvent()?->semester()->id ?? Semester::current())
app/Http/Controllers/Secretariat/SemesterEvaluationController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Secretariat/SemesterEvaluationController.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Models/User.php (1)
553-566:⚠️ Potential issueCritical: Replace undefined method
withRole.The new scope method has the same issue as previously identified - it uses the undefined method
withRole. This will cause runtime errors.Apply this diff to fix the issue:
public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) { /** @var Builder|static $query */ - return $query->withRole(Role::COLLEGIST)->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { + return $query->whereHas('roles', function ($q) { + $q->where('role_id', Role::COLLEGIST->id); + })->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { $query->where('semester_id', $semester->id); }); }
🧹 Nitpick comments (1)
tests/Feature/AdmissionTest.php (1)
Line range hint
46-61: Consider enhancing the test data setup, if I may suggest.The
createApplicanthelper method could be made more robust with these refinements:
- Add optional parameters with sensible defaults
- Extract common test values into constants
Here's a proposed enhancement:
+ private const DEFAULT_VERIFIED = false; + private const DEFAULT_SUBMITTED = true; + private const DEFAULT_WORKSHOPS = []; - private function createApplicant(string $name, bool $submitted, $workshops): User + private function createApplicant( + string $name, + bool $submitted = self::DEFAULT_SUBMITTED, + array $workshops = self::DEFAULT_WORKSHOPS, + bool $verified = self::DEFAULT_VERIFIED + ): User { /* @var User $user */ - $user = User::factory()->create(['name' => $name, 'verified' => false]); + $user = User::factory()->create(['name' => $name, 'verified' => $verified]); $user->application()->create(['submitted' => $submitted]); $this->assertDatabaseHas('applications', [ 'user_id' => $user->id, 'submitted' => $submitted ]); $user->application->syncAppliedWorkshops($workshops); return $user; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Http/Controllers/Auth/ApplicationController.php(2 hunks)app/Http/Controllers/Secretariat/SemesterEvaluationController.php(4 hunks)app/Listeners/SemesterEvaluationPeriodStartListener.php(1 hunks)app/Models/User.php(3 hunks)tests/Feature/AdmissionTest.php(1 hunks)
🔇 Additional comments (10)
app/Models/User.php (2)
389-389: Documentation update looks good!The updated documentation accurately reflects the method's purpose and return type.
542-542: Parameter type enhancement looks splendid!The addition of nullable type and default value improves flexibility whilst maintaining backward compatibility.
app/Http/Controllers/Auth/ApplicationController.php (3)
31-31: Inheritance updated to 'PeriodicEventController'The
ApplicationControllernow extendsPeriodicEventController, which aligns with the application's architecture for managing periodic events.
41-44: Constructor initialises parent with 'APPLICATION_PERIOD'The constructor correctly calls the parent constructor with
PeriodicEvent::APPLICATION_PERIOD, ensuring proper initialisation.
50-69: Method 'updateApplicationPeriod' correctly manages application periodThe
updateApplicationPeriodmethod effectively handles authorisation, validation, and updates the periodic event as expected.app/Http/Controllers/Secretariat/SemesterEvaluationController.php (3)
25-30: Inheritance updated to 'PeriodicEventController'The
SemesterEvaluationControllernow extendsPeriodicEventController, which standardises the management of periodic events across controllers.
157-160: Role change handled without triggering eventsEncapsulating the role modification within
RoleUser::withoutEventsensures that no events are inadvertently triggered during the role update, which is appropriate in this context.
Line range hint
68-86: Verify the existence of 'self::semester()' methodPlease confirm that the
self::semester()method is defined in thePeriodicEventControllerclass and accessible here, as it is utilised in these lines.To ensure the method exists, you may run the following script:
✅ Verification successful
The
self::semester()method is properly accessibleThe method is indeed available through inheritance from
PeriodicEventController, where it's defined as a final public method. The inheritance chain is properly established, and the constructor correctly initialises the parent class.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the 'semester' method declaration in the codebase. # Test: Search for the 'public function semester()' method declaration. rg -A 2 -B 2 'public function semester\(\)'Length of output: 2733
Script:
#!/bin/bash # Check if SemesterEvaluationController extends PeriodicEventController ast-grep --pattern 'class SemesterEvaluationController extends $_'Length of output: 68
Script:
#!/bin/bash # Search for the SemesterEvaluationController class definition rg -A 5 "class SemesterEvaluationController" app/Http/Controllers/Secretariat/SemesterEvaluationController.phpLength of output: 299
app/Listeners/SemesterEvaluationPeriodStartListener.php (1)
18-30: Listener correctly sends emails upon evaluation period startThe
SemesterEvaluationPeriodStartListenerproperly queues emails to the configured contacts, including the secretary, director, and president, when the evaluation period begins.tests/Feature/AdmissionTest.php (1)
39-39: Splendid modification, old chap!I say, this change from
'event_model'to'event_name'is rather fitting. It aligns splendidly with the PR's objective of decoupling periodic events from controller names. Jolly good show!Might I suggest we run a quick verification to ensure this change is consistently applied throughout the test suite? Here's a script that should do the trick:
✅ Verification successful
Verification Complete: Splendid Consistency Across Test Suite
I'm pleased to report that your changes are perfectly consistent throughout the test suite. Both the AdmissionTest and ApplicationTest files are properly aligned with the new convention, and there are no lingering instances of the old 'event_model' pattern. Jolly good work!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of 'event_name' in test files # Test: Search for any remaining instances of 'event_model' in test files rg --type php "'event_model'" tests/ # Test: Confirm the usage of 'event_name' with PeriodicEvent::APPLICATION_PERIOD in test files rg --type php "'event_name'.*PeriodicEvent::APPLICATION_PERIOD" tests/Length of output: 315
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Models/User.php (1)
558-571:⚠️ Potential issueReplace undefined
withRolemethod with standard Eloquent query.The method logic is sound, but it uses the
withRolemethod which may cause runtime errors as it's not defined in theBuilderclass.Apply this diff to use standard Eloquent query methods:
public function scopeDoesntHaveStatusFor(Builder $query, Semester $semester) { /** @var Builder|static $query */ - return $query->withRole(Role::COLLEGIST)->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { + return $query->whereHas('roles', function ($q) { + $q->where('role_id', Role::COLLEGIST->id); + })->whereDoesntHave('semesterStatuses', function ($query) use ($semester) { $query->where('semester_id', $semester->id); }); }
🧹 Nitpick comments (1)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (1)
76-78: Regarding the semester retrieval, if I may make a suggestion.Based on the previous discussions in the review comments, while
self::semester()is indeed defined in the parent class, we might want to add a bit of defensive programming here.Perhaps consider this slightly more robust approach:
- 'evaluation' => user()->semesterEvaluations()->where('semester_id', self::semester()?->id)->first(), + 'evaluation' => user()->semesterEvaluations() + ->when(self::semester(), function ($query, $semester) { + return $query->where('semester_id', $semester->id); + }) + ->first(),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php(3 hunks)app/Models/User.php(3 hunks)resources/views/emails/evaluation_form_reminder.blade.php(0 hunks)tests/Feature/ApplicationTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- resources/views/emails/evaluation_form_reminder.blade.php
🧰 Additional context used
🪛 phpstan (2.0.3)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php
81-81: Call to an undefined static method App\Http\Controllers\Secretariat\SemesterEvaluationController::usersHaventFilledOutTheForm().
(staticMethod.notFound)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: phpunit
- GitHub Check: phpunit
🔇 Additional comments (6)
tests/Feature/ApplicationTest.php (1)
35-35: Splendid modification to the event creation parameters.I must say, this change to use
'event_name'instead of'event_model'and the introduction of thePeriodicEvent::APPLICATION_PERIODconstant is rather well thought out. It aligns splendidly with the PR's objective of decoupling periodic events from controller names.Inconsistency Detected in Event Naming Convention
It appears that while most test files have adopted the
'event_name'key andPeriodicEvent::APPLICATION_PERIODconstant, theAdmissionTest.phpstill utilises'event_model'. Please update this file to maintain consistency across the test suite.#!/bin/bash # Description: Verify consistent usage of 'event_name' and PeriodicEvent::APPLICATION_PERIOD # Test: Search for occurrences of 'event_model' or 'event_name' in test files rg --type php -g 'tests/**/*Test.php' -e "('event_model'|'event_name')" # Test: Search for occurrences of PeriodicEvent::APPLICATION_PERIOD in test files rg --type php -g 'tests/**/*Test.php' -e "PeriodicEvent::APPLICATION_PERIOD"app/Models/User.php (2)
389-389: Documentation update looks good!The clarification in the documentation better reflects the method's purpose of returning semester evaluations.
547-547: Parameter type enhancement is appropriate!The nullable integer type with a default value of null aligns well with the method's implementation, allowing it to fall back to the current semester when no specific semester is provided.
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (3)
7-25: Splendid work on the proper type declarations and inheritance!I must say, the addition of proper type imports and the extension of
PeriodicEventControlleralign rather nicely with the objective of decoupling periodic events.
27-30: Excellent implementation of the constructor!The constructor properly initialises the periodic event type, which is jolly good for maintaining the separation of concerns.
156-159: Brilliant use of withoutEvents!I must commend the proper handling of role changes within a
withoutEventsclosure. This is a rather elegant way to prevent unwanted event cascades during role transitions.
| 'position_roles' => user()->roles()->whereIn('name', Role::STUDENT_POSTION_ROLES)->get(), | ||
| 'periodicEvent' => $this->periodicEvent(), | ||
| 'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) ? SemesterEvaluationController::usersHaventFilledOutTheForm($this->semester()) : null, | ||
| 'users_havent_filled_out' => user()->can('manage', SemesterEvaluation::class) && self::semester() ? SemesterEvaluationController::usersHaventFilledOutTheForm(self::semester()) : null, |
There was a problem hiding this comment.
I say, there appears to be a missing static method.
The static analysis tool has rather astutely pointed out that usersHaventFilledOutTheForm is undefined as a static method. This requires immediate attention.
Would you be so kind as to implement this static method or adjust the call to use an instance method instead?
🧰 Tools
🪛 phpstan (2.0.3)
81-81: Call to an undefined static method App\Http\Controllers\Secretariat\SemesterEvaluationController::usersHaventFilledOutTheForm().
(staticMethod.notFound)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Console/Commands/FinalizeSemesterEvaluation.php(1 hunks)app/Http/Controllers/Secretariat/SemesterEvaluationController.php(4 hunks)app/Listeners/SemesterEvaluationPeriodEndListener.php(1 hunks)app/Models/User.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Models/User.php
🧰 Additional context used
🪛 phpstan (2.0.3)
app/Console/Commands/FinalizeSemesterEvaluation.php
43-43: Call to static method transaction() on an unknown class App\Console\Commands\DB.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
44-44: Call to static method withoutEvents() on an unknown class App\Console\Commands\RoleUser.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
45-45: Call to static method collegist() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
46-46: Call to static method alumni() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Call to static method to() on an unknown class App\Console\Commands\Mail.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Instantiated class App\Console\Commands\StatusDeactivated not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Call to static method error() on an unknown class App\Console\Commands\Log.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
🔇 Additional comments (3)
app/Listeners/SemesterEvaluationPeriodEndListener.php (1)
36-38: Kindly verify the email content consistency for workshop leadersThe
EvaluationFormClosedemail sent to workshop leaders does not include the$users_namesparameter, whereas it is included for the secretary, president, and director. If it is intended for workshop leaders to receive information about users who have not completed the evaluation form, please consider passing$users_namesto ensure they receive the same information.app/Http/Controllers/Secretariat/SemesterEvaluationController.php (2)
76-78: Everything looks goodThe use of
self::semester()is appropriate as it is defined in the parentPeriodicEventController.
157-160: Please verify the use ofwithoutEventsonRoleUserCould you please confirm that invoking
withoutEventsonRoleUseris appropriate for suppressing events when modifying user roles? IfRoleUserdoes not support this method, consider usingUser::withoutEventsor another suitable class to ensure event listeners are properly suppressed during role updates.
| DB::transaction(function () use ($user) { | ||
| RoleUser::withoutEvents(function () use ($user) { | ||
| $user->removeRole(Role::collegist()); | ||
| $user->addRole(Role::alumni()); | ||
| }); | ||
| Mail::to($user)->queue(new StatusDeactivated($user->name)); | ||
| }); | ||
| } catch (\Exception $e) { | ||
| Log::error('Error deactivating collegist: ' . $user->name . ' - ' . $e->getMessage()); |
There was a problem hiding this comment.
Please add the missing import statements for the used classes
The classes DB, RoleUser, Role, Mail, Log, and StatusDeactivated are being utilised but their namespaces have not been imported. Kindly add the appropriate use statements to ensure the code functions correctly.
You may include the following import statements at the beginning of the file:
use Illuminate\Support\Facades\DB;
use App\Models\RoleUser;
use App\Models\Role;
use Illuminate\Support\Facades\Mail;
use Illuminate\Support\Facades\Log;
use App\Mail\StatusDeactivated;🧰 Tools
🪛 phpstan (2.0.3)
43-43: Call to static method transaction() on an unknown class App\Console\Commands\DB.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
44-44: Call to static method withoutEvents() on an unknown class App\Console\Commands\RoleUser.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
45-45: Call to static method collegist() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
46-46: Call to static method alumni() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Call to static method to() on an unknown class App\Console\Commands\Mail.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Instantiated class App\Console\Commands\StatusDeactivated not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Call to static method error() on an unknown class App\Console\Commands\Log.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
|
Sorry for this that it took so long. Now it also supports the deactivation through the command line that was introduced in #681. However, I have a docker bug now so I can not test it at the moment manually. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Feature/StatusTest.php (2)
68-73: Brilliant consistency in test implementation.The parallel implementation with the previous test maintains good consistency. However, consider adding assertions to verify the command's execution status.
Consider adding:
Artisan::call('app:finalize-semester-evaluation', [ 'year' => Semester::current()->year, 'part' => Semester::current()->part ]); + $this->assertEquals(0, Artisan::call('app:finalize-semester-evaluation')); $this->assertTrue($this->user->isActive(Semester::next()));
Line range hint
42-73: Rather comprehensive test coverage, but consider edge cases.While the current tests cover the primary scenarios, consider adding tests for:
- Edge cases around semester boundaries
- Multiple status changes within the same evaluation period
- Error conditions in the Artisan command
Would you like me to provide example test cases for these scenarios?
🧰 Tools
🪛 GitHub Actions: CI
[error] 48-48: Test 'test_set_collegist_to_alumni' failed due to missing DB class dependency
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Feature/StatusTest.php(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
tests/Feature/StatusTest.php
[error] 48-48: Test 'test_set_collegist_to_alumni' failed due to missing DB class dependency
🔇 Additional comments (2)
tests/Feature/StatusTest.php (2)
21-21: Splendid addition of the protected property.The introduction of
$periodicEventas a protected property improves test maintainability by centralising the periodic event instance.
Line range hint
30-35: Rather good refactoring of the periodic event creation.The switch to using
event_namewith a proper constant improves code clarity and maintainability. However, do ensure the test data window of 24 hours is sufficient for all test scenarios.Let's verify if any tests might be time-sensitive:
✅ Verification successful
The 24-hour test window is indeed sufficient for the periodic event setup.
I've thoroughly examined the time-dependent operations across the test suite, and I can confirm that the 24-hour window in the periodic event setup is perfectly adequate. The test doesn't rely on specific time intervals for its assertions, and other time-sensitive tests maintain their own temporal contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for time-dependent assertions or conditions rg -l "addDays|subDays|addHours|subHours" tests/Length of output: 159
Script:
#!/bin/bash # Check time-related operations in the identified test files echo "=== StatusTest.php ===" rg "add(Days|Hours)|sub(Days|Hours)" tests/Feature/StatusTest.php -B 2 -A 2 echo -e "\n=== ReservationTest.php ===" rg "add(Days|Hours)|sub(Days|Hours)" tests/Feature/ReservationTest.php -B 2 -A 2 echo -e "\n=== AdminInternetControllerTest.php ===" rg "add(Days|Hours)|sub(Days|Hours)" tests/Feature/AdminInternetControllerTest.php -B 2 -A 2Length of output: 6811
| Artisan::call('app:finalize-semester-evaluation', [ | ||
| 'year' => Semester::current()->year, 'part' => Semester::current()->part | ||
| ]); | ||
|
|
||
| $this->assertFalse($this->user->hasRole(Role::COLLEGIST)); | ||
| $this->assertTrue($this->user->hasRole(Role::ALUMNI)); | ||
| $this->assertTrue($this->user->isCollegist()); |
There was a problem hiding this comment.
Jolly good transition to Artisan commands, but mind the dependency.
The shift to using Artisan commands is a proper improvement, but the pipeline failure suggests a missing database dependency.
Please add the necessary database setup before the Artisan command:
+ // Ensure database is properly set up
+ $this->app->make('db');
+
Artisan::call('app:finalize-semester-evaluation', [📝 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.
| Artisan::call('app:finalize-semester-evaluation', [ | |
| 'year' => Semester::current()->year, 'part' => Semester::current()->part | |
| ]); | |
| $this->assertFalse($this->user->hasRole(Role::COLLEGIST)); | |
| $this->assertTrue($this->user->hasRole(Role::ALUMNI)); | |
| $this->assertTrue($this->user->isCollegist()); | |
| // Ensure database is properly set up | |
| $this->app->make('db'); | |
| Artisan::call('app:finalize-semester-evaluation', [ | |
| 'year' => Semester::current()->year, 'part' => Semester::current()->part | |
| ]); | |
| $this->assertFalse($this->user->hasRole(Role::COLLEGIST)); | |
| $this->assertTrue($this->user->hasRole(Role::ALUMNI)); | |
| $this->assertTrue($this->user->isCollegist()); |
🧰 Tools
🪛 GitHub Actions: CI
[error] 48-48: Test 'test_set_collegist_to_alumni' failed due to missing DB class dependency
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Console/Commands/FinalizeSemesterEvaluation.php (1)
8-10:⚠️ Potential issueI say, we're still missing some rather crucial imports, old chap.
Splendid work adding the Facade imports, but we're still missing a few essential ones. The pipeline is failing due to these oversights. Do allow me to assist:
use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Mail; +use App\Models\RoleUser; +use App\Models\Role; +use App\Mail\StatusDeactivated;
🧹 Nitpick comments (2)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (2)
Line range hint
165-165: I dare say, this error message ought to be localised.The hard-coded Hungarian error message "A státusz megadása kötelező!" should be moved to the localisation system for consistency with the rest of the application.
- return back()->with('error', "A státusz megadása kötelező!")->with('section', $request->section); + return back()->with('error', __('evaluation.status_required'))->with('section', $request->section);
180-192: Might I suggest a dash of validation, good sir?While the implementation is quite proper, it would be rather prudent to add parameter validation to ensure the semester is not null and has a successor:
public static function usersHaventFilledOutTheForm(Semester $semester) { + if (!$semester->succ()) { + throw new \InvalidArgumentException('Semester must have a successor'); + } + return User::doesntHaveStatusFor($semester->succ()) ->whereDoesntHave('roles', function ($query) { $query->where('name', Role::SENIOR); # seniors are ignored })->get(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Console/Commands/FinalizeSemesterEvaluation.php(2 hunks)app/Http/Controllers/Secretariat/SemesterEvaluationController.php(4 hunks)
🧰 Additional context used
🪛 phpstan (2.0.3)
app/Console/Commands/FinalizeSemesterEvaluation.php
47-47: Call to static method withoutEvents() on an unknown class App\Console\Commands\RoleUser.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Call to static method collegist() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
49-49: Call to static method alumni() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Instantiated class App\Console\Commands\StatusDeactivated not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Parameter #1 $mailable of method Illuminate\Mail\PendingMail::queue() expects Illuminate\Contracts\Mail\Mailable, App\Console\Commands\StatusDeactivated given.
(argument.type)
🪛 GitHub Actions: CI
app/Console/Commands/FinalizeSemesterEvaluation.php
[error] 47-47: Class "App\Console\Commands\RoleUser" not found
🔇 Additional comments (3)
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (3)
7-7: I say, splendid architectural improvement!The refactoring to extend
PeriodicEventControllerand the introduction of the constructor with a specific periodic event type demonstrates a rather proper separation of concerns. Jolly good show!Also applies to: 15-15, 25-30
157-160: Rather good use of withoutEvents, I must say!Splendid implementation using
withoutEventsto prevent unwanted event triggers during role transitions. This should prevent any unintended side effects during the status change.
68-68: Do ensure proper handling of nullable semester references, old chap.While the type hints are a splendid addition, we ought to verify the handling of nullable semester references in these view parameters:
self::semester()?->idself::usersHaventFilledOutTheForm(self::semester())Also applies to: 76-76, 78-82
| foreach ($users as $user) { | ||
| try { | ||
| //deactivate collegist, give them alumni role. | ||
| DB::transaction(function () use ($user) { | ||
| RoleUser::withoutEvents(function () use ($user) { | ||
| $user->removeRole(Role::collegist()); | ||
| $user->addRole(Role::alumni()); | ||
| }); | ||
| Mail::to($user)->queue(new StatusDeactivated($user->name)); | ||
| }); | ||
| } catch (\Exception $e) { | ||
| Log::error('Error deactivating collegist: ' . $user->name . ' - ' . $e->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
I must point out some concerns regarding the deactivation process.
While the error handling is appreciated, there are several refinements we ought to consider:
- The nested callbacks could be simplified for better readability
- We should validate the role changes were successful
- The command should provide feedback about successes and failures
- Silent failures could leave the system in an inconsistent state
Here's a more robust implementation:
foreach ($users as $user) {
+ $this->info("Processing user: {$user->name}");
try {
- DB::transaction(function () use ($user) {
- RoleUser::withoutEvents(function () use ($user) {
- $user->removeRole(Role::collegist());
- $user->addRole(Role::alumni());
- });
- Mail::to($user)->queue(new StatusDeactivated($user->name));
- });
+ DB::transaction(function () use ($user) {
+ $user->removeRole(Role::collegist());
+ if (!$user->hasRole(Role::alumni())) {
+ $user->addRole(Role::alumni());
+ }
+ Mail::to($user)->queue(new StatusDeactivated($user->name));
+ });
+ $this->info("Successfully deactivated: {$user->name}");
} catch (\Exception $e) {
- Log::error('Error deactivating collegist: ' . $user->name . ' - ' . $e->getMessage());
+ $message = "Error deactivating collegist: {$user->name} - {$e->getMessage()}";
+ Log::error($message);
+ $this->error($message);
+ throw $e; // Prevent silent failures
}
}
+ $this->info('Semester evaluation finalisation completed.');Additionally, consider adding a database transaction isolation level to prevent concurrent modifications:
DB::transaction(function () use ($user) {
DB::statement('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE');
// ... rest of the transaction code
}, 5);🧰 Tools
🪛 phpstan (2.0.3)
47-47: Call to static method withoutEvents() on an unknown class App\Console\Commands\RoleUser.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
48-48: Call to static method collegist() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
49-49: Call to static method alumni() on an unknown class App\Console\Commands\Role.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Instantiated class App\Console\Commands\StatusDeactivated not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Parameter #1 $mailable of method Illuminate\Mail\PendingMail::queue() expects Illuminate\Contracts\Mail\Mailable, App\Console\Commands\StatusDeactivated given.
(argument.type)
🪛 GitHub Actions: CI
[error] 47-47: Class "App\Console\Commands\RoleUser" not found
| $users = SemesterEvaluationController::usersHaventFilledOutTheForm($semester); | ||
| $users_names = $users->pluck('name')->toArray(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Might I suggest a more Laravel-esque approach to controller usage?
Rather than using static methods, it would be more proper to inject the controller through Laravel's service container. This approach aligns better with Laravel's dependency injection patterns and makes the code more testable.
Consider this refined approach:
- $users = SemesterEvaluationController::usersHaventFilledOutTheForm($semester);
+ $evaluationController = app()->make(SemesterEvaluationController::class);
+ $users = $evaluationController->usersHaventFilledOutTheForm($semester);📝 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.
| $users = SemesterEvaluationController::usersHaventFilledOutTheForm($semester); | |
| $users_names = $users->pluck('name')->toArray(); | |
| $evaluationController = app()->make(SemesterEvaluationController::class); | |
| $users = $evaluationController->usersHaventFilledOutTheForm($semester); | |
| $users_names = $users->pluck('name')->toArray(); |
kdmnk
force-pushed
the
decouple-periodic-events
branch
from
ff66799 to
16f35dc
Compare
kdmnk
force-pushed
the
decouple-periodic-events
branch
from
16f35dc to
72951eb
Compare
horcsinbalint
force-pushed
the
development
branch
2 times, most recently
from
758a095 to
86e46a2
Compare
Now, the controllers using a periodicEvent can set which periodicEvent do they connected to.
The Controllers that use a periodicEvents can define which periodicEvent they want to edit and use in their constructor. The possible options can be found in the PeriodicEvents.
When we handle the start/end dates, we fire events and use event handlers.
Note: I had to move some functions out from the controllers so that the event handlers can take care of the functionalities.