Suggestions to use periodicEvents for AnonymousQuestions #554
Conversation
WalkthroughWalkthroughThe recent updates focus on refining the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as AnonymousQuestionController
participant V as View
participant M as Model
U->>C: Request to create question (POST /questions/create)
C->>M: Validate semester status, create question
M-->>C: Question creation success/failure
C-->>V: Render response view
U->>C: Request to view questions (GET /questions)
C->>M: Fetch questions list
M-->>C: Return questions list
C-->>V: Render questions list view
U->>C: Request to store answers (POST /{semester}/sheets/)
C->>M: Validate and store answers
M-->>C: Answers storage success/failure
C-->>V: Render response view
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (2)
Line range hint
61-97: The refactoring in thestoremethod removes the need for aSemesterparameter, relying instead on theperiodicEventto determine the applicable semester. The validation logic is comprehensive, but consider using a more robust approach for managing optional fields to enhance maintainability.- 'options.*' => 'exclude_if:has_long_answers,on|nullable|string|max:255', + 'options.*' => 'exclude_if:has_long_answers,on|required|string|max:255',Additionally, ensure that the
optionsarray is always validated as an array of strings, even whenhas_long_answersis on, to prevent potential issues with data integrity.
[!TIP]
Codebase VerificationThe route definition for the
showmethod inAnonymousQuestionControllerstill includes the{semester}parameter, which is inconsistent with the updated method signature that only accepts aQuestionparameter. This needs to be corrected to ensure consistency across the application.
routes/web.php: Update the route definition forAnonymousQuestionController::showto remove the{semester}parameter.Analysis chain
Line range hint
116-123: Theshowmethod has been simplified to accept only aQuestionparameter. This change is consistent with the PR's theme of simplifying the handling of semesters. Ensure that all references to this method across the application are updated to reflect this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all method calls to `show` have been updated. # Test: Search for the method usage. Expect: Only occurrences with the new signature. rg --type php $'show'Length of output: 14781
Script:
#!/bin/bash # Description: Verify that the route definition for AnonymousQuestionController::show has been updated to reflect the new signature. # Test: Search for the route definition in routes/web.php. Expect: No occurrences with the old signature. rg 'Route::get\(\'/\{semester\}/questions/\{question\}\', \[AnonymousQuestionController::class, \'show\'\]\)' routes/web.phpLength of output: 93
Script:
#!/bin/bash # Description: Verify that the route definition for AnonymousQuestionController::show has been updated to reflect the new signature. # Test: Search for the route definition in routes/web.php. Expect: No occurrences with the old signature. rg "Route::get\('/\{semester\}/questions/\{question\}', \[AnonymousQuestionController::class, 'show'\]\)" routes/web.phpLength of output: 249
| public function create() | ||
| { | ||
| $this->authorize('administer', AnswerSheet::class); | ||
|
|
||
| if ($semester->isClosed()) { | ||
| if (!$this->isActive()) { | ||
| abort(403, "tried to add a question to a closed semester"); | ||
| } | ||
| return view('student-council.anonymous-questions.create', [ | ||
| "semester" => $semester | ||
| ]); | ||
| return view('student-council.anonymous-questions.create'); |
There was a problem hiding this comment.
The create method now includes a check for active periodic events before allowing the creation of a new question. This is a sensible addition to prevent modifications outside the designated period. However, the error message could be more descriptive to enhance user understanding.
- abort(403, "tried to add a question to a closed semester");
+ abort(403, "Attempted to add a question outside the active period. Please check the current settings and try again.");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.
| public function create() | |
| { | |
| $this->authorize('administer', AnswerSheet::class); | |
| if ($semester->isClosed()) { | |
| if (!$this->isActive()) { | |
| abort(403, "tried to add a question to a closed semester"); | |
| } | |
| return view('student-council.anonymous-questions.create', [ | |
| "semester" => $semester | |
| ]); | |
| return view('student-council.anonymous-questions.create'); | |
| public function create() | |
| { | |
| $this->authorize('administer', AnswerSheet::class); | |
| if (!$this->isActive()) { | |
| abort(403, "Attempted to add a question outside the active period. Please check the current settings and try again."); | |
| } | |
| return view('student-council.anonymous-questions.create'); |
| @@ -108,15 +105,15 @@ public function store(Request $request, Semester $semester) | |||
| } | |||
| } | |||
|
|
|||
| session()->put('section', $semester->id); | |||
| //session()->put('section', $semester->id); | |||
There was a problem hiding this comment.
The redirect route anonymous_questions.index_semesters appears to be outdated or incorrect as per the changes in this PR which suggest a simplification of route names.
- return redirect()->route('anonymous_questions.index_semesters')
+ return redirect()->route('anonymous_questions.index')This change aligns the redirection with the updated routing structure.
Committable suggestion was skipped due to low confidence.
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
|
Thank you:) I start to get the idea. So if I understand right, there is only at most one PeriodicEvent connected to a controller. If a new semester comes, the I think it's fine if they can only create questions for the current semester; I agree it isn't worth the fuss to allow them to manipulate future semesters which are half a year later. I just didn't understand then. Thank you again; I think I'll pick it up here in the summer;) |
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
resources/views/student-council/anonymous-questions/index.blade.php
Outdated
Show resolved
Hide resolved
resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
Outdated
Show resolved
Hide resolved
…ate of the periodic event
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
| /** | ||
| * Checks whether the form exists and has not yet been closed; | ||
| * aborts the request if necessary. | ||
| * If successful, it returns the periodic event. | ||
| */ | ||
| private function checkPeriodicEvent(): PeriodicEvent | ||
| { | ||
| $periodicEvent = $this->periodicEvent(); | ||
| if (is_null($periodicEvent)) { | ||
| abort(404, "no evaluation form exists yet"); | ||
| } elseif ($periodicEvent->endDate()?->isPast() ?? false) { | ||
| abort(403, "tried to add a question to a closed form"); | ||
| } else { | ||
| return $periodicEvent; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the 'new question' page. | ||
| */ | ||
| public function create(Semester $semester) | ||
| public function create() | ||
| { | ||
| $this->authorize('administer', AnswerSheet::class); | ||
| $this->checkPeriodicEvent(); |
| @php | ||
| $isActive = $currentSemester?->id == $semester->id | ||
| && (!$periodicEvent->endDate()?->isPast() ?? false); | ||
| @endphp |
There was a problem hiding this comment.
Handle the scenario where $periodicEvent is null more gracefully.
Consider adding a check to handle the case where $periodicEvent is null to avoid potential issues.
- $isActive = $currentSemester?->id == $semester->id && (!$periodicEvent->endDate()?->isPast() ?? false);
+ $isActive = $currentSemester?->id == $semester->id && ($periodicEvent ? !$periodicEvent->endDate()?->isPast() : false);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.
| @php | |
| $isActive = $currentSemester?->id == $semester->id | |
| && (!$periodicEvent->endDate()?->isPast() ?? false); | |
| @endphp | |
| @php | |
| $isActive = $currentSemester?->id == $semester->id | |
| && ($periodicEvent ? !$periodicEvent->endDate()?->isPast() : false); | |
| @endphp |
| @php | ||
| // let the current semester be found based on the periodic event itself | ||
| // beware: it might be null | ||
| $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent(); | ||
| $currentSemester = $periodicEvent?->semester; |
There was a problem hiding this comment.
Handle the scenario where $periodicEvent is null more gracefully.
Consider adding a check to handle the case where $periodicEvent is null to avoid potential issues.
- $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent();
- $currentSemester = $periodicEvent?->semester;
+ $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent();
+ $currentSemester = $periodicEvent ? $periodicEvent->semester : null;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.
| @php | |
| // let the current semester be found based on the periodic event itself | |
| // beware: it might be null | |
| $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent(); | |
| $currentSemester = $periodicEvent?->semester; | |
| @php | |
| // let the current semester be found based on the periodic event itself | |
| // beware: it might be null | |
| $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent(); | |
| $currentSemester = $periodicEvent ? $periodicEvent->semester : null; |
| @@ -136,6 +136,9 @@ public function show() | |||
| $this->authorize('fillOrManage', SemesterEvaluation::class); | |||
|
|
|||
| return view('secretariat.evaluation-form.app', [ | |||
| // let the current semester be found based on the periodic event itself | |||
| // we can safely assume it is not null | |||
| 'semester' => app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->semester(), | |||
There was a problem hiding this comment.
You are already in the controlller. Use $this->semester().
| @php | ||
| // let the current semester be found based on the periodic event itself | ||
| // beware: it might be null | ||
| $periodicEvent = app(\App\Http\Controllers\Secretariat\SemesterEvaluationController::class)->periodicEvent(); |
There was a problem hiding this comment.
Can you pass this to the blade from the controller?
horcsinbalint
force-pushed
the
development
branch
2 times, most recently
from
758a095 to
86e46a2
Compare
@viktorcsimma I think I know what you found confusing about the periodicEvents.
With PeriodicEvents, you cannot create a question for a previous semester, you can only create and answer a question for the current period (which makes sense to me, but let me know if you don't agree).
When the dates of the semesterEvaluation are set, the semester to which it applies must also be specified. You can use that semester via the periodicEvent which will also define the deadlines for the questions.
Questions for older semesters can still be exported as they are stored for their semesters.
This PR contains how I think the controller should utilise the PeriodicEvents. The other parts of the solution should be modified accordingly.
Feel free to take over this branch or create a new one.