Anonymous questions #542
Anonymous questions #542
Conversation
WalkthroughWalkthroughThe recent updates add anonymous question functionality to the evaluation forms. This includes new models, migrations, seeders, and views to handle anonymous questions, relationships between entities, and policies for authorisation. An Changes
Sequence Diagram(s)sequenceDiagram
participant Student as Student
participant App as Application
participant Model as Model
participant Database as Database
Student->>App: Submit anonymous question form
App->>Model: Save answers to AnswerSheet model
Model->>Database: Store answers in anonymous_questions table
Student->>App: Request list of anonymous questions
App->>Database: Fetch questions based on semester
Database->>App: Return list of questions
App->>Student: Display list of questions for semester
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional context usedLearnings (1)
Additional comments not posted (6)
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: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- app/Exports/UsersSheets/AnonymousQuestionsExport.php (1 hunks)
- app/Http/Controllers/Secretariat/AnonymousQuestionController.php (1 hunks)
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php (3 hunks)
- app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (9 hunks)
- app/Models/AnonymousQuestions/AnswerSheet.php (1 hunks)
- app/Models/AnonymousQuestions/LongAnswer.php (1 hunks)
- app/Models/GeneralAssemblies/GeneralAssembly.php (2 hunks)
- app/Models/GeneralAssemblies/Question.php (8 hunks)
- app/Models/Semester.php (4 hunks)
- app/Policies/AnswerSheetPolicy.php (1 hunks)
- database/factories/GeneralAssemblies/QuestionFactory.php (1 hunks)
- database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php (1 hunks)
- database/seeders/AnonymousQuestionSeeder.php (1 hunks)
- database/seeders/DatabaseSeeder.php (1 hunks)
- database/seeders/GeneralAssemblySeeder.php (3 hunks)
- resources/lang/en/anonymous_questions.php (1 hunks)
- resources/lang/en/general.php (1 hunks)
- resources/lang/hu/anonymous_questions.php (1 hunks)
- resources/lang/hu/general.php (1 hunks)
- resources/views/layouts/navbar.blade.php (1 hunks)
- resources/views/livewire/excused-users.blade.php (1 hunks)
- resources/views/livewire/parent-child-form.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/app.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/index_semesters.blade.php (1 hunks)
- resources/views/student-council/general-assemblies/questions/create.blade.php (3 hunks)
- resources/views/student-council/general-assemblies/questions/show.blade.php (4 hunks)
- routes/web.php (2 hunks)
- tests/Unit/AnonymousQuestionTest.php (1 hunks)
- tests/Unit/GeneralAssemblyTest.php (9 hunks)
Files skipped from review due to trivial changes (7)
- resources/lang/en/anonymous_questions.php
- resources/lang/en/general.php
- resources/lang/hu/anonymous_questions.php
- resources/lang/hu/general.php
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
- resources/views/secretariat/evaluation-form/index_semesters.blade.php
- resources/views/student-council/general-assemblies/questions/create.blade.php
Additional comments not posted (51)
app/Policies/AnswerSheetPolicy.php (1)
14-19: LGTM! The authorization logic correctly restricts access to functionalities related to anonymous questions.database/seeders/DatabaseSeeder.php (1)
22-22: LGTM! The inclusion ofAnonymousQuestionSeederensures that the database is populated with data necessary for anonymous questions.database/factories/GeneralAssemblies/QuestionFactory.php (1)
27-27: LGTM! Settinghas_long_answersto false ensures that the factory creates questions appropriate for general assemblies, aligning with the PR's focus on anonymous questions.app/Models/AnonymousQuestions/LongAnswer.php (1)
26-32: LGTM! Disabling timestamps enhances anonymity, and the configuration of fillable attributes is appropriate for handling mass assignments in the context of anonymous questions.resources/views/livewire/parent-child-form.blade.php (3)
21-21: LGTM! Ensure the new classes align with the intended CSS and JavaScript functionality.
27-27: LGTM! Ensure the new classes align with the intended CSS and JavaScript functionality.
29-29: LGTM! Ensure the new classes align with the intended CSS and JavaScript functionality.database/seeders/GeneralAssemblySeeder.php (2)
26-26: LGTM! Ensure thehas_long_answersattribute is correctly utilized in the application logic.Also applies to: 43-43
67-67: LGTM! Ensure the new voting logic correctly handles multiple selections.app/Models/AnonymousQuestions/AnswerSheet.php (3)
30-30: LGTM! The setting of timestamps tofalseenhances privacy.
65-74: LGTM! ThecreateForUsermethod enhances code reusability and maintainability.
39-58: LGTM! The defined relationships support the new functionality of handling anonymous questions.database/seeders/AnonymousQuestionSeeder.php (1)
19-50: LGTM! The seeder logic supports the new functionality of handling anonymous questions and their answers.resources/views/livewire/excused-users.blade.php (1)
16-16: Corrected attribute syntax in Blade component.This change corrects the attribute syntax from
:allowEmptytoallowEmpty, ensuring consistency with Blade's expected syntax for non-expression attributes.app/Exports/UsersSheets/AnonymousQuestionsExport.php (3)
22-25: Constructor correctly initializes the semester property.The constructor is well-implemented, with clear documentation and appropriate initialization of the
semesterproperty.
42-64: Mapping function correctly handles different question types.The
mapmethod is robust, handling both long answers and multiple choice questions efficiently. The logic is clear and well-documented.
70-77: Headings method correctly utilizes localization and dynamic question titles.The
headingsmethod dynamically generates column titles based on the semester's questions, correctly utilizing localization for static fields. This enhances maintainability and user experience.database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php (3)
14-22: Creation ofanswer_sheetstable is well-defined with appropriate foreign keys.The
answer_sheetstable is correctly set up with necessary fields and foreign key constraints, ensuring data integrity.
24-34: Modifications toquestionstable support polymorphic relationships effectively.The changes to the
questionstable, including the introduction of a polymorphic relationship and makingparent_typenon-nullable, are well-executed and enhance the model's flexibility.
45-54: Creation oflong_answerstable is well-defined with appropriate foreign keys.The
long_answerstable is correctly set up with necessary fields and foreign key constraints, ensuring data integrity and appropriate links to questions and answer sheets.tests/Unit/GeneralAssemblyTest.php (6)
30-30: Test method correctly uses updated model relationships.The test method
test_voting_on_closed_questioncorrectly uses theparentrelationship in the factory method, aligning with the updated model relationships.
47-47: Consistent use of updated model relationships in test method.The test method
test_voting_on_not_opened_questionconsistently uses theparentrelationship, ensuring that the tests align with the updated model structure.
64-64: Correct implementation of model relationships in test for voting restrictions.The test method
test_voting_twicecorrectly uses theparentrelationship, which is crucial for accurately simulating the voting scenario in the context of the updated model structure.
82-82: Proper use of model relationships in radio button voting test.The test method
test_voting_radiocorrectly uses theparentrelationship and includes appropriate assertions to verify the behavior of radio button voting.
125-125: Correct model relationship usage in checkbox voting test.The test method
test_voting_checkboxcorrectly uses theparentrelationship, ensuring accurate testing of checkbox voting functionality.
151-151: Consistent model relationship usage in test for checkbox voting limits.The test method
test_voting_checkbox_with_more_optionsconsistently uses theparentrelationship, accurately testing the limits of checkbox voting.resources/views/student-council/general-assemblies/questions/show.blade.php (3)
5-5: Update breadcrumb links to reflect the new parent-child relationship.
17-17: Ensure the updated form action URLs correctly point to the new routes handling the parent-child relationship.Also applies to: 101-101, 109-109
Verification successful
The routes for handling the parent-child relationship between
general_assembliesandquestionsare correctly defined in theroutes/web.phpfile. Here are the relevant routes:
Route::get('/general_assemblies/{general_assembly}/questions/create', [GeneralAssemblyQuestionController::class, 'create'])->name('general_assemblies.questions.create');Route::post('/general_assemblies/{general_assembly}/questions', [GeneralAssemblyQuestionController::class, 'store'])->name('general_assemblies.questions.store');Route::get('/general_assemblies/{general_assembly}/questions/{question}', [GeneralAssemblyQuestionController::class, 'show'])->name('general_assemblies.questions.show');Route::post('/general_assemblies/{general_assembly}/questions/{question}/open', [GeneralAssemblyQuestionController::class, 'openQuestion'])->name('general_assemblies.questions.open');Route::post('/general_assemblies/{general_assembly}/questions/{question}/close', [GeneralAssemblyQuestionController::class, 'closeQuestion'])->name('general_assemblies.questions.close');Route::post('/general_assemblies/{general_assembly}/questions/{question}/votes', [GeneralAssemblyQuestionController::class, 'saveVote'])->name('general_assemblies.questions.votes.store')->withoutMiddleware(LogRequests::class);These routes confirm that the form action URLs in the
show.blade.phpfile are correctly pointing to the new routes handling the parent-child relationship.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new routes are correctly defined in the routes file. # Test: Search for the new routes in the routes file. Expect: Definitions for the new routes. rg --type php $'route('general_assemblies.questions.*')' routes/web.phpLength of output: 70
Script:
#!/bin/bash # Description: Verify the new routes are correctly defined in the routes file. # Test: Search for general route definitions related to general_assemblies and questions. rg --type php 'general_assemblies' routes/web.php rg --type php 'questions' routes/web.phpLength of output: 4918
118-118: Validate the logic that prevents opening questions after the assembly has been closed.Verification successful
The
isOpenmethod in theGeneralAssemblymodel correctly checks if the assembly has been opened and not closed. This logic aligns with the conditional check in theshow.blade.phpfile.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic that checks if the assembly is open before allowing questions to be opened. # Test: Search for the method `isOpen` in the `GeneralAssembly` model. Expect: Correct implementation of the `isOpen` method. ast-grep --lang php --pattern $'class GeneralAssembly { $$$ isOpen($_) { $$$ } $$$ }'Length of output: 207
Script:
#!/bin/bash # Description: Verify the logic that checks if the assembly is open before allowing questions to be opened. # Step 1: Find the `GeneralAssembly` model file. general_assembly_file=$(fd GeneralAssembly.php) # Step 2: Search for the `isOpen` method in the `GeneralAssembly` model. if [[ -n "$general_assembly_file" ]]; then rg 'function isOpen' "$general_assembly_file" else echo "GeneralAssembly model file not found." fiLength of output: 265
Script:
#!/bin/bash # Description: Extract and display the implementation of the `isOpen` method in the `GeneralAssembly` model. # Step 1: Find the `GeneralAssembly` model file. general_assembly_file=$(fd GeneralAssembly.php) # Step 2: Extract the implementation of the `isOpen` method. if [[ -n "$general_assembly_file" ]]; then rg -A 10 'function isOpen' "$general_assembly_file" else echo "GeneralAssembly model file not found." fiLength of output: 478
app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (3)
7-7: Ensure theQuestionmodel is correctly utilized in the controller, especially with the new parent-child relationship.
183-183: Ensure the redirect after voting correctly uses the updated parent-child relationship.
103-103: Validate the conditions under which questions can be opened or closed, ensuring they align with the assembly's state.Also applies to: 124-124
Verification successful
The
isOpenandisClosedmethods in theQuestionmodel are correctly implemented to check the conditions for opening and closing questions. TheisOpenmethod ensures that a question is considered open if it has been opened and not closed, while theisClosedmethod checks if the question has aclosed_attimestamp that is not null and is in the past.
isOpenmethod:public function isOpen(): bool { return $this->isForAssembly() ? ($this->hasBeenOpened() && !$this->isClosed()) : !$this->parent->isClosed(); }
isClosedmethod:public function isClosed(): bool { return $this->isForAssembly() ? ($this->closed_at != null && $this->closed_at <= now()) : $this->parent->isClosed(); }These implementations align with the assembly's state and ensure that questions are managed correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditions for opening and closing questions are correctly implemented. # Test: Search for the methods `isOpen` and `isClosed` in the `Question` model. Expect: Correct implementation of these methods. ast-grep --lang php --pattern $'class Question { $$$ isOpen($_) { $$$ } $$$ isClosed($_) { $$$ } $$$ }'Length of output: 235
Script:
#!/bin/bash # Description: Verify the conditions for opening and closing questions are correctly implemented. # Test: Search for the methods `isOpen` and `isClosed` in the `Question` model. Expect: Correct implementation of these methods. rg 'function isOpen' --glob '**/Question.php' rg 'function isClosed' --glob '**/Question.php'Length of output: 253
Script:
#!/bin/bash # Description: Extract the implementations of the `isOpen` and `isClosed` methods from the `Question` model. # Extract the implementation of the `isOpen` method. rg -A 10 'function isOpen' app/Models/GeneralAssemblies/Question.php # Extract the implementation of the `isClosed` method. rg -A 10 'function isClosed' app/Models/GeneralAssemblies/Question.phpLength of output: 720
app/Models/GeneralAssemblies/GeneralAssembly.php (1)
63-65: Update thequestionsrelationship to useMorphManyto support the new polymorphic relationship.tests/Unit/AnonymousQuestionTest.php (1)
1-206: Comprehensive test coverage for anonymous question functionality, including edge cases for closed semesters and multiple submissions.app/Http/Controllers/Secretariat/AnonymousQuestionController.php (7)
32-34: Ensure proper authorization checks are in place for viewing semesters.
44-46: Handle attempts to add questions to closed semesters gracefully.
63-69: Validate input data rigorously to prevent invalid data submission.
83-88: Ensure that questions are created with correct attributes and timestamps.
111-113: Check if the question belongs to the correct semester to prevent data inconsistencies.
161-187: Ensure that answer sheets are created and stored correctly, handling all questions at once.
197-200: Facilitate the export of answer sheets to Excel, ensuring data privacy and integrity.resources/views/secretariat/evaluation-form/app.blade.php (1)
147-150: Integrate the anonymous questions section into the evaluation form.This change aligns with the PR objectives to include anonymous questions in the evaluation forms.
app/Http/Controllers/Secretariat/SemesterEvaluationController.php (1)
91-91: Update validation rules to includeanonymous_questionsin thesectionfield.This change is necessary to support the new functionality of handling anonymous questions within the evaluation forms.
app/Models/GeneralAssemblies/Question.php (4)
62-65: Ensure theparentrelationship is polymorphic to support different types of parent entities.
88-91: Support long answers in questions when applicable.
219-255: Handle anonymous answers correctly, ensuring they are associated with the right question and answer sheet.
262-276: Create long answers for questions that accept them, ensuring data integrity.app/Models/Semester.php (2)
173-173: The new relationships and methods are well-implemented and align with the PR objectives.Also applies to: 181-181, 225-225, 230-230, 248-248, 257-257, 266-266
162-168: Ensure theisClosedmethod correctly handles time zones.resources/views/layouts/navbar.blade.php (1)
119-129: The new navigation item for anonymous questions is correctly gated with permissions.routes/web.php (1)
268-277: The new routes for handling anonymous questions are well-structured and follow RESTful conventions.
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/Policies/AnswerSheetPolicy.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/Policies/AnswerSheetPolicy.php
|
Thank you, I just quickly ran through the code, and the code quality seems to be quite nice.
|
viktorcsimma
force-pushed
the
anonymous-questions
branch
from
eb52d0b to
2c3892c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- app/Exports/UsersSheets/AnonymousQuestionsExport.php (1 hunks)
- app/Http/Controllers/Secretariat/AnonymousQuestionController.php (1 hunks)
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php (4 hunks)
- app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (9 hunks)
- app/Models/AnonymousQuestions/AnswerSheet.php (1 hunks)
- app/Models/AnonymousQuestions/LongAnswer.php (1 hunks)
- app/Models/GeneralAssemblies/GeneralAssembly.php (2 hunks)
- app/Models/GeneralAssemblies/Question.php (8 hunks)
- app/Models/Semester.php (4 hunks)
- app/Policies/AnswerSheetPolicy.php (1 hunks)
- database/factories/GeneralAssemblies/QuestionFactory.php (1 hunks)
- database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php (1 hunks)
- database/seeders/AnonymousQuestionSeeder.php (1 hunks)
- database/seeders/DatabaseSeeder.php (1 hunks)
- database/seeders/GeneralAssemblySeeder.php (3 hunks)
- resources/lang/en/anonymous_questions.php (1 hunks)
- resources/lang/en/general.php (1 hunks)
- resources/lang/hu/anonymous_questions.php (1 hunks)
- resources/lang/hu/general.php (1 hunks)
- resources/views/layouts/navbar.blade.php (1 hunks)
- resources/views/livewire/excused-users.blade.php (1 hunks)
- resources/views/livewire/parent-child-form.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/app.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/index_semesters.blade.php (1 hunks)
- resources/views/student-council/general-assemblies/questions/create.blade.php (3 hunks)
- resources/views/student-council/general-assemblies/questions/show.blade.php (4 hunks)
- routes/web.php (2 hunks)
- tests/Unit/AnonymousQuestionTest.php (1 hunks)
- tests/Unit/GeneralAssemblyTest.php (9 hunks)
Files skipped from review as they are similar to previous changes (29)
- app/Exports/UsersSheets/AnonymousQuestionsExport.php
- app/Http/Controllers/Secretariat/AnonymousQuestionController.php
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php
- app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php
- app/Models/AnonymousQuestions/AnswerSheet.php
- app/Models/AnonymousQuestions/LongAnswer.php
- app/Models/GeneralAssemblies/GeneralAssembly.php
- app/Models/Semester.php
- app/Policies/AnswerSheetPolicy.php
- database/factories/GeneralAssemblies/QuestionFactory.php
- database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
- database/seeders/AnonymousQuestionSeeder.php
- database/seeders/DatabaseSeeder.php
- database/seeders/GeneralAssemblySeeder.php
- resources/lang/en/anonymous_questions.php
- resources/lang/en/general.php
- resources/lang/hu/anonymous_questions.php
- resources/lang/hu/general.php
- resources/views/layouts/navbar.blade.php
- resources/views/livewire/excused-users.blade.php
- resources/views/livewire/parent-child-form.blade.php
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
- resources/views/secretariat/evaluation-form/app.blade.php
- resources/views/secretariat/evaluation-form/index_semesters.blade.php
- resources/views/student-council/general-assemblies/questions/create.blade.php
- resources/views/student-council/general-assemblies/questions/show.blade.php
- routes/web.php
- tests/Unit/AnonymousQuestionTest.php
- tests/Unit/GeneralAssemblyTest.php
Additional comments not posted (9)
app/Models/GeneralAssemblies/Question.php (9)
12-12: Ensure theMorphToimport is utilized effectively in the model.
17-18: Addition ofAnswerSheetandLongAnswermodels supports the new functionality for anonymous questions.
32-32: Update the@property-readannotation to reflect the polymorphic relationship.
62-65: Theparent()method correctly implements a polymorphic relationship, which is crucial for associating questions with either a general assembly or a semester.
71-73: The methodisForAssembly()provides a clear way to check the type of the parent model. This is useful for conditional logic based on the parent type.
[APROVED]
106-108: The methodshasBeenOpened(),isOpen(), andisClosed()use the new polymorphic relationship to determine the state of the question based on its parent. This is a critical change for the functionality of these methods.Also applies to: 116-119, 127-130
88-91: ThelongAnswers()method correctly defines aHasManyrelationship. Ensure that theLongAnswermodel is properly set up to handle this relationship.Verification successful
The
LongAnswermodel includesBelongsTorelationships foranswerSheetandquestion, but it does not define anyHasManyrelationships. This setup is sufficient for theQuestionmodel to have aHasManyrelationship withLongAnswer.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the setup of the `LongAnswer` model to ensure it supports the `HasMany` relationship. # Test: Search for the `LongAnswer` model definition. Expect: Proper relationship methods and properties. rg --type php 'class LongAnswer' -A 20Length of output: 1366
Script:
#!/bin/bash # Description: Verify the setup of the `LongAnswer` model to ensure it supports the `HasMany` relationship. # Test: Search for relationship methods within the `LongAnswer` model file. rg --type php 'function ' app/Models/AnonymousQuestions/LongAnswer.phpLength of output: 159
262-276: ThegiveLongAnswer()method correctly handles the creation of long answers. Ensure that thehas_long_answersproperty is properly managed and that theLongAnswermodel supports this functionality.Verification successful
The
has_long_answersproperty is properly managed across the codebase, including its definition in theQuestionmodel, its addition in migrations, and its usage in controllers for validation and creation of questions. TheLongAnswermodel is also correctly set up and supports the required functionality, with appropriate relationships to theQuestionandAnswerSheetmodels.
- The
has_long_answersproperty is defined in theQuestionmodel and used in various parts of the codebase, including views, seeders, migrations, factories, and controllers.- The
LongAnswermodel is properly set up with necessary relationships and properties, and it is used in theQuestionandAnswerSheetmodels.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the management of the `has_long_answers` property and the support in the `LongAnswer` model. # Test: Search for the `has_long_answers` property usage and the `LongAnswer` model setup. Expect: Proper management and support. rg --type php -e 'has_long_answers' -e 'LongAnswer' -C 10Length of output: 47176
53-53: Ensure all newly added fields in$fillableare intended to be mass assignable.Verification successful
The searches did not reveal any dynamic modifications to the
$fillablearray or any mass assignment methods involving theQuestionmodel. This suggests that the fields in$fillableare not being used in a way that could lead to security vulnerabilities.
The
$fillablearray in theQuestionmodel is defined as:protected $fillable = ['title', 'sitting_id', 'max_options', 'opened_at', 'closed_at', 'has_long_answers'];No instances of
Question::create(,Question::update(,Question::fill(, orQuestion::updateOrCreate(were found in the codebase.No dynamic modifications to the
$fillablearray were detected.Based on this information, it appears that the fields in
$fillableare intended to be mass assignable and are used appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all fields in `$fillable` are safe for mass assignment and are used appropriately in the application. # Test: Search for usages of mass assignment in the application. Expect: Safe usage of mass assignment. rg --type php 'new Question\('Length of output: 32
Script:
#!/bin/bash # Description: Verify that all fields in `$fillable` are safe for mass assignment and are used appropriately in the application. # Test: Search for usages of mass assignment in the application. Expect: Safe usage of mass assignment. rg --type php 'Question::create\(' rg --type php 'Question::update\('Length of output: 73
Script:
#!/bin/bash # Description: Verify that all fields in `$fillable` are safe for mass assignment and are used appropriately in the application. # Test: Search for usages of mass assignment in the application. Expect: Safe usage of mass assignment. rg --type php 'Question::fill\(' rg --type php 'Question::updateOrCreate\('Length of output: 79
Script:
#!/bin/bash # Description: Verify that the `$fillable` array in the `Question` model is not being modified dynamically elsewhere in the codebase. # Test: Search for any direct assignments to the `fillable` array in the `Question` model. rg --type php '\$fillable\s*=' # Test: Ensure that the `fillable` array is not being modified dynamically elsewhere in the codebase. rg --type php 'Question::\$fillable'Length of output: 2937
| @@ -106,7 +136,7 @@ public function isClosed(): bool | |||
| */ | |||
| public function open(): void | |||
| { | |||
| if (!$this->generalAssembly->isOpen()) { | |||
| if (!$this->parent->isOpen()) { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The output from the shell script indicates that the giveAnonymousAnswer() method is called in the app/Http/Controllers/Secretariat/AnonymousQuestionController.php file without explicit exception handling. This confirms that there is a potential issue with exception handling in this specific instance.
To address this, the giveAnonymousAnswer() method calls should be wrapped in try-catch blocks to ensure that any exceptions thrown are properly handled.
Locations that need attention:
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Suggested Action:
Wrap the giveAnonymousAnswer() method calls in try-catch blocks to handle exceptions appropriately.
try {
$question->giveAnonymousAnswer(user(), $answerSheet, $options);
} catch (\Exception $e) {
// Handle the exception, e.g., log the error, return an error response, etc.
return back()->withErrors(['error' => $e->getMessage()]);
}
try {
$question->giveAnonymousAnswer(user(), $answerSheet, $option);
} catch (\Exception $e) {
// Handle the exception, e.g., log the error, return an error response, etc.
return back()->withErrors(['error' => $e->getMessage()]);
}This ensures that any exceptions thrown by the giveAnonymousAnswer() method are caught and handled appropriately, preventing potential issues during execution.
Analysis chain
Exception handling in the open(), vote(), and giveAnonymousAnswer() methods should be reviewed to ensure that they are thrown appropriately and can be caught or handled where these methods are called.
Also applies to: 187-187, 219-219
The output from the shell script shows that the methods open(), vote(), and giveAnonymousAnswer() are called in various test and controller files. In the test files, exceptions are expected and asserted using $this->expectException(\Exception::class);. In the controller files, exceptions are handled using abort() with appropriate HTTP status codes.
Here are the locations where these methods are called and their exception handling mechanisms:
-
Test Files:
tests/Unit/AnonymousQuestionTest.phptests/Unit/GeneralAssemblyTest.php- Exceptions are asserted using
$this->expectException(\Exception::class);.
-
Controller Files:
app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.phpopen(): Handled usingabort(403, "tried to open a question when the sitting itself was not open");andabort(403, "tried to open a question which has already been opened");.vote(): Handled usingabort(401, "Tried to vote for an option which does not belong to the question");.
app/Http/Controllers/Secretariat/AnonymousQuestionController.phpgiveAnonymousAnswer(): No explicit exception handling observed in the provided snippet.
Given this information, it appears that most instances have proper exception handling. However, the giveAnonymousAnswer() method in app/Http/Controllers/Secretariat/AnonymousQuestionController.php lacks explicit exception handling in the provided snippet.
To ensure comprehensive verification, let's generate a script to search for all instances of giveAnonymousAnswer() in the app/Http/Controllers/Secretariat/AnonymousQuestionController.php file and check for exception handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that exceptions thrown in these methods are handled appropriately in the calling code.
# Test: Search for usages of these methods and check for exception handling. Expect: Proper try-catch blocks or error handling mechanisms.
rg --type php -e 'open\(' -e 'vote\(' -e 'giveAnonymousAnswer\(' -C 10
Length of output: 48820
Script:
#!/bin/bash
# Description: Verify exception handling for giveAnonymousAnswer() in app/Http/Controllers/Secretariat/AnonymousQuestionController.php
# Search for all instances of giveAnonymousAnswer() in the specified controller file and check for exception handling.
rg 'giveAnonymousAnswer\(' -C 10 app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Length of output: 1088
There was a problem hiding this comment.
@kdmnk, could you please clarify what specific check you would like to add in the parent within the open() method? Are you referring to additional conditions to verify before a question can be opened, or is there a particular validation concerning the state or attributes of the parent entity? Any additional details would help ensure the implementation aligns with your expectations.
There was a problem hiding this comment.
The comment is meant to be for @viktorcsimma
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.
@kdmnk I don't understand this one; are you proposing to remove that check? Or moving it to the controller?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- database/seeders/AnonymousQuestionSeeder.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- database/seeders/AnonymousQuestionSeeder.php
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/Policies/AnswerSheetPolicy.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/Policies/AnswerSheetPolicy.php
|
@horcsinbalint thanks for checking it out so quickly:)
|
I have concerns with making it mandatory to answer anything on these (even if there is an option of "I don't want to answer"). I think I might have spotted some bugs in the code. But as I have major concerns with the planned functionality proposed in this PR, I am unable to process the PR further. Unfortunately, the automatic deployment is not working and I will have to manually sign these changes off. Is it okay with you if I consult with the Ethics Commissioners to put my worries to rest? If there are no issues with the proposed functionality, it should not be a problem. But if they share my concerns, it is much better to resolve the problems before I deploy these changes. |
|
Huh; the problem is that it would be needed ASAP… what if I took personal responsibility for it? Or if we looked for someone else to approve it? @kdmnk? |
|
Okay, I can parallelize processing this issue and discussing my concerns with the Ethics Commissioners if it is okay with you. |
|
OK, thank you:) |
|
Wait, is it mandatory to answer anonymous questions? If not, then I have no big concerns |
database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
Outdated
Show resolved
Hide resolved
viktorcsimma
force-pushed
the
anonymous-questions
branch
from
4aced75 to
dccec31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- app/Exports/UsersSheets/AnonymousQuestionsExport.php (1 hunks)
- app/Http/Controllers/Secretariat/AnonymousQuestionController.php (1 hunks)
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php (3 hunks)
- app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (9 hunks)
- app/Models/AnonymousQuestions/AnswerSheet.php (1 hunks)
- app/Models/AnonymousQuestions/LongAnswer.php (1 hunks)
- app/Models/GeneralAssemblies/GeneralAssembly.php (2 hunks)
- app/Models/GeneralAssemblies/Question.php (8 hunks)
- app/Models/Semester.php (4 hunks)
- app/Policies/AnswerSheetPolicy.php (1 hunks)
- database/factories/GeneralAssemblies/QuestionFactory.php (1 hunks)
- database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php (1 hunks)
- database/seeders/AnonymousQuestionSeeder.php (1 hunks)
- database/seeders/DatabaseSeeder.php (1 hunks)
- database/seeders/GeneralAssemblySeeder.php (3 hunks)
- resources/lang/en/anonymous_questions.php (1 hunks)
- resources/lang/en/general.php (1 hunks)
- resources/lang/hu/anonymous_questions.php (1 hunks)
- resources/lang/hu/general.php (1 hunks)
- resources/views/layouts/navbar.blade.php (1 hunks)
- resources/views/livewire/excused-users.blade.php (1 hunks)
- resources/views/livewire/parent-child-form.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/app.blade.php (1 hunks)
- resources/views/secretariat/evaluation-form/index_semesters.blade.php (1 hunks)
- resources/views/student-council/general-assemblies/questions/create.blade.php (3 hunks)
- resources/views/student-council/general-assemblies/questions/show.blade.php (4 hunks)
- routes/web.php (2 hunks)
- tests/Unit/AnonymousQuestionTest.php (1 hunks)
- tests/Unit/GeneralAssemblyTest.php (9 hunks)
Files skipped from review as they are similar to previous changes (30)
- app/Exports/UsersSheets/AnonymousQuestionsExport.php
- app/Http/Controllers/Secretariat/AnonymousQuestionController.php
- app/Http/Controllers/Secretariat/SemesterEvaluationController.php
- app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php
- app/Models/AnonymousQuestions/AnswerSheet.php
- app/Models/AnonymousQuestions/LongAnswer.php
- app/Models/GeneralAssemblies/GeneralAssembly.php
- app/Models/GeneralAssemblies/Question.php
- app/Models/Semester.php
- app/Policies/AnswerSheetPolicy.php
- database/factories/GeneralAssemblies/QuestionFactory.php
- database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
- database/seeders/AnonymousQuestionSeeder.php
- database/seeders/DatabaseSeeder.php
- database/seeders/GeneralAssemblySeeder.php
- resources/lang/en/anonymous_questions.php
- resources/lang/en/general.php
- resources/lang/hu/anonymous_questions.php
- resources/lang/hu/general.php
- resources/views/layouts/navbar.blade.php
- resources/views/livewire/excused-users.blade.php
- resources/views/livewire/parent-child-form.blade.php
- resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
- resources/views/secretariat/evaluation-form/app.blade.php
- resources/views/secretariat/evaluation-form/index_semesters.blade.php
- resources/views/student-council/general-assemblies/questions/create.blade.php
- resources/views/student-council/general-assemblies/questions/show.blade.php
- routes/web.php
- tests/Unit/AnonymousQuestionTest.php
- tests/Unit/GeneralAssemblyTest.php
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/Unit/AnonymousQuestionTest.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/Unit/AnonymousQuestionTest.php
Now, it's not technically mandatory to fill them; my personal opinion is that we need not explicitly tell fillers about it, as they otherwise won't fill:D But could you talk to the Ethics Commissioners since then?
Is there anyone here who could check this out ASAP? (I know, I was slow in responding to requests; sorry) |
I will respond to the other thread soon which probably needs to be addressed. |
|
OK; thank you:) |
There was a problem hiding this comment.
Wow, that's nice job. This is a complex problem so I added a lot of comments related how I think the morph relation should work. 2 reasons: without proper abstrabtion levels, it's just simpler to make another, similar module without combining them; this will also enable us to add anonym questions for other modules (eg. mr and miss).
Read through them and let me know if something is not clear.
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Secretariat/AnonymousQuestionController.php
Outdated
Show resolved
Hide resolved
resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
Outdated
Show resolved
Hide resolved
resources/views/secretariat/evaluation-form/anonymous_questions.blade.php
Outdated
Show resolved
Hide resolved
resources/views/student-council/general-assemblies/questions/create.blade.php
Outdated
Show resolved
Hide resolved
About these, if the user does not fill something in, you can still store an empty string or zero voted options in the database to mark the question as filled if you need to. |
|
Okay; it seems to work now. (For some reason, it was only stable if I used the DB facade directly instead of Eloquent operations. I still don't know what happened, but I'm glad I could find a workaround:D) If you agree to the others, I think there is only one thing left; this conversation. |
| 'title' => $request->title, | ||
| 'max_options' => $hasLongAnswers ? 0 : $request->max_options, | ||
| 'has_long_answers' => $hasLongAnswers, | ||
| 'opened_at' => (app(SemesterEvaluationController::class))->getStartDate() ?? Carbon::now(), |
There was a problem hiding this comment.
didn't the solution I suggested to add HasPeriodicEvents here work?
There was a problem hiding this comment.
I think it does:) What about adding a periodicEventForSemester method so that we can access the event for the correct semester? I've tried that; it seems to work for me.
There was a problem hiding this comment.
It does not really make sense. We have one periodicEvent which might belong to the semester you are looking for or not. The attached semester to the periodicEvent is only there to fix the semester used by the context. You should set the semester id of the feedback based on that, if you don't have that yet.
|
And good job on the tests! |
if you want, I can take a look tomorrow |
app/Utils/HasPeriodicEvent.php
Outdated
| /** | ||
| * Get the PeriodicEvent connected to the controller | ||
| * and belonging to the given semester | ||
| * (by default the current one). | ||
| * | ||
| * @return PeriodicEvent|null | ||
| */ | ||
| final public function periodicEventForSemester(?Semester $semester): ?PeriodicEvent | ||
| { | ||
| if (is_null($semester)) $semester = Semester::current(); | ||
| return PeriodicEvent::where('event_model', $this->underlyingControllerName) | ||
| ->where('semester_id', $semester->id) | ||
| ->first(); | ||
| } | ||
|
|
There was a problem hiding this comment.
@kdmnk that's what I thought of. And we call this method with the semester for which we are creating the question. And if that semester is in the future, this will return null, and the question will not be opened.
There was a problem hiding this comment.
This won't give you any data for older semesters.
The information flow should be: you get/set the periodicEvent, and through that, you get the semester that you can use.
When you create a questions, you store them with the semester you get from the current periodicEvent.
Afterwards, you can still query older questions by directly using the semester ids, without the periodicEvent as it might have been updated for a new voting period.
There was a problem hiding this comment.
Oh; so those are only there for querying older ones... Maybe the easiest way for now would be to only allow admins to add questions for the current semester. (They probably won't want to do so for future semesters anyway.)
Let me know what you think. If you agree, I'll proceed with that.
There was a problem hiding this comment.
Well if you need this asap I think go back to the embedded google forms :D
We could try to release it, but I'm trying not to release half done features anymore. I will have time during the weekend to have a deeper look again and give some solutions for our uncertainties. (Currently the morph relation's complexity overcomes its advantages, but I have to take another look).
Again, after the previous years of experience, our priority here is to produce software that is easily maintainable and understandable, which is more important to me than releasing new features.
After the discussions, I might also need to go back to the PeriodicEvents as I can see it is not clearly understandable and does not really cover this case.
What do you think?
There was a problem hiding this comment.
I also understand if you want to release this and will go back to it afterwards if you are confident about the current solution's behaviour.
There was a problem hiding this comment.
I also understand if you want to release this and will go back to it afterwards if you are confident about the current solution's behaviour.
Yes, that would be the best for me now:) and after the form gets closed, we can open a new issue for it. I think I can work on this more on July.
Thank you 😊
There was a problem hiding this comment.
I advise against deploying changes in the codebase that are not sure to be correct. I also have a problem on an operational standpoint as in June there will not be enough human resources to restore a backup if that is needed due to a mistake presented in the newly deployed code. But I will deploy the changes if someone takes the responsibility even though I would not recommend it.
There was a problem hiding this comment.
Okay; I think I've played with it long enough to say it's going to be alright. So I'll take the blame if something happens.
Thank you:) but it's not important for now; the priority would be to deploy this as soon as possible. |
Is this test finished on the staging system? #532 (comment) |
Yes; it was quite a long saga, but in the end, it got merged to development. I think you have also tested it; see #547. |
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
app/Utils/HasPeriodicEvent.php (1)
Line range hint
35-62: The methodsperiodicEventandperiodicEventForSemesterseem well-implemented. However, consider adding error handling or logging to manage cases where the database queries fail or return unexpected results.+ try { + // Existing code + } catch (\Exception $e) { + \Log::error("Failed to retrieve periodic event: " . $e->getMessage()); + return null; + }
database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
Show resolved
Hide resolved
database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
Show resolved
Hide resolved
database/migrations/2024_06_01_173029_add_anonymous_questions_to_evaluation_form.php
Show resolved
Hide resolved
It was requested from me to deploy it to staging #532 (comment) :
I have deployed the changes to the staging server and sent credentials privately. #543 and #548 are quite big and I have not received confirmation whether it works with live data and if it is safe to deploy on the live environment. |
|
Or at least I have not tested it (other than logging into it and catching #544). But I have no idea if the code is fit for production. |
|
@horcsinbalint please keep the chat here only for the PR and code related messages, it is already quite hectic. |
| public static function createForUser(User $user, Semester $semester = null): AnswerSheet | ||
| { | ||
| if (is_null($user)) { | ||
| $user = user(); | ||
| } | ||
| if (is_null($semester)) { | ||
| $semester = Semester::current(); | ||
| } | ||
|
|
||
| return $semester->answerSheets()->create([ | ||
| 'year_of_acceptance' => $user->educationalInformation->year_of_acceptance | ||
| ]); | ||
| } |
There was a problem hiding this comment.
The method handles null users and semesters effectively. Consider adding a comment to clarify that a null user defaults to the current authenticated user.
- if (is_null($user)) {
+ // Default to the current authenticated user if none is provided
+ if (is_null($user)) {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 static function createForUser(User $user, Semester $semester = null): AnswerSheet | |
| { | |
| if (is_null($user)) { | |
| $user = user(); | |
| } | |
| if (is_null($semester)) { | |
| $semester = Semester::current(); | |
| } | |
| return $semester->answerSheets()->create([ | |
| 'year_of_acceptance' => $user->educationalInformation->year_of_acceptance | |
| ]); | |
| } | |
| public static function createForUser(User $user, Semester $semester = null): AnswerSheet | |
| { | |
| // Default to the current authenticated user if none is provided | |
| if (is_null($user)) { | |
| $user = user(); | |
| } | |
| if (is_null($semester)) { | |
| $semester = Semester::current(); | |
| } | |
| return $semester->answerSheets()->create([ | |
| 'year_of_acceptance' => $user->educationalInformation->year_of_acceptance | |
| ]); | |
| } |
|
Then if you are fine with it; I'll merge this now and then we can reopen discussion in the summer. And I'll take the blame if anything blows up;) Thank you both very much! |
Description
Adds anonymous questions to evaluation forms, which can be managed by the Students' Council
and whose results can be exported into an Excel spreadsheet.
Related Issue
Closes #530.
Changes
Note
It would be nice if we could merge it until Sunday; the form should be opened next week. Thanks in advance:) I will check out #532 until then.