Implement Single Transferable Voting #708
Conversation
📝 WalkthroughWalkthroughThis pull request refactors several controllers to centralise question creation and vote saving within a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PHPStan (2.0.3)/bin/bash: line 1: /vendor/bin/phpstan: No such file or directory 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: 9
🔭 Outside diff range comments (2)
resources/views/utils/question_card.blade.php (1)
24-35: 🛠️ Refactor suggestionI say, we've got some redundant code here.
The
toggleLongAnswersscript appears to be unused since the long answers checkbox has been removed. Might I suggest removing this section entirely?-@push('scripts') -{{-- disable answer options if this is checked --}} -<script> -function toggleLongAnswers(checkbox) { - document.getElementById('max_options').disabled = checkbox.checked; - const toDisable = document.getElementsByClassName('parent-child'); - for (let i = 0; i < toDisable.length; i++) { - toDisable[i].disabled = checkbox.checked; - } -} -</script> -@endpushdatabase/seeders/GeneralAssemblySeeder.php (1)
66-69: 🛠️ Refactor suggestionConsider adding error handling for the storeAnswers method.
I must point out that the storeAnswers method calls could benefit from proper error handling to ensure robustness.
- $openQuestion->storeAnswers($collegist, [$openQuestion->options->random()]); + try { + $openQuestion->storeAnswers($collegist, [$openQuestion->options->random()]); + } catch (\Exception $e) { + \Log::error("Failed to store answers for collegist {$collegist->id}: " . $e->getMessage()); + }
🧹 Nitpick comments (27)
resources/views/utils/questions/results/selection.blade.php (3)
1-1: Consider handling empty options collection.I say, it would be rather prudent to add a check for empty options to provide appropriate feedback to users when no options are available.
+@if($question->options->isEmpty()) + <tr><td colspan="2">No options available for this question.</td></tr> +@else @foreach($question->options->sortByDesc('votes') as $option)
2-7: Enhance table accessibility and structure.I must point out that the table structure could benefit from proper accessibility attributes and semantic markup.
-<tr> +<tr role="row"> - <td>{{$option->title}}</td> + <td role="cell">{{ e($option->title) }}</td> @if($question->hasBeenOpened()) - <td><b>{{$option->votes}}</b></td> + <td role="cell"><strong>{{ $option->votes }}</strong></td> @endif </tr>Additionally, consider adding table headers and a caption for better context:
<table role="grid" aria-label="Question options and votes"> <caption>Results for: {{ e($question->title) }}</caption> <thead> <tr role="row"> <th role="columnheader" scope="col">Option</th> @if($question->hasBeenOpened()) <th role="columnheader" scope="col">Votes</th> @endif </tr> </thead> <tbody> <!-- existing rows here --> </tbody> </table>
1-8: Enhance visual clarity for voting results.I believe it would be most helpful to visually distinguish the winning option(s) and provide additional context about the voting results.
+@php + $maxVotes = $question->options->max('votes'); +@endphp @foreach($question->options->sortByDesc('votes') as $option) -<tr> +<tr class="{{ $option->votes === $maxVotes ? 'bg-success-subtle' : '' }}"> <td>{{ e($option->title) }}</td> @if($question->hasBeenOpened()) - <td><b>{{$option->votes}}</b></td> + <td> + <strong>{{ $option->votes }}</strong> + @if($option->votes === $maxVotes) + <span class="badge bg-success">Winner</span> + @endif + <small class="text-muted">({{ number_format($option->votes / $question->options->sum('votes') * 100, 1) }}%)</small> + </td> @endif </tr> @endforeach +@if($question->hasBeenOpened()) +<tr class="table-info"> + <td><strong>Total Votes</strong></td> + <td><strong>{{ $question->options->sum('votes') }}</strong></td> +</tr> +@endifapp/Http/Controllers/StudentsCouncil/QuestionController.php (1)
66-88: Good modular design for vote saving
ThesaveVoteForQuestionmethod neatly delegates the core answer-storing logic tostoreAnswers(...). This approach aids reusability and keeps controller code tidy. It may be beneficial to add additional checks fornullreturns inQuestionOption::find(...)before proceeding, but overall it looks robust.app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (2)
99-107: Passcode validation
Your custom validation closure is a neat way to check the passcode. Consider surfacing hints or more context in error messages if passcodes differ by subtle criteria, but otherwise this is a straightforward approach.
115-126: Ensure correct parent type on deletion
The check for$question['parent_type'] != GeneralAssembly::classensures you only delete questions belonging to general assemblies. This is sensible; however, consider referencing$question->parent_typeas a property, instead of array-access syntax, for clarity.- if($question['parent_type'] != GeneralAssembly::class) { + if($question->parent_type !== GeneralAssembly::class) {app/Models/Question.php (2)
277-304:rankingData()method
Retrieving and randomising ballots, then reading fromresults_cache, is neatly done. You might consider separate smaller methods for retrieving options, processing statistics, and randomising ballots to further enhance clarity and testability.
350-372: Use of Condorcet for STV
ThecomputeElectionResultsmethod applies STV using Condorcet, which is quite clever. If performance or concurrency becomes a concern with very large ballots, consider async processing or queueing to offload the election tally.resources/views/utils/questions/selection.blade.php (1)
1-7: Rather well done, though might I suggest a small enhancement?The implementation is quite proper, handling both single and multiple selections with elegance. However, one might consider adding ARIA labels for enhanced accessibility.
A modest improvement, if I may:
- <x-input.radio :name="$name ?? $question->formKey()" value="{{$option->id}}" text="{{$option->title}}" :checked="old($question->formKey()) == $option->id" /> + <x-input.radio :name="$name ?? $question->formKey()" value="{{$option->id}}" text="{{$option->title}}" :checked="old($question->formKey()) == $option->id" aria-label="{{$option->title}}" />resources/views/utils/questions/results/ranking.blade.php (3)
2-4: Consider adding error handling for rankingData()The direct call to
rankingData()without error handling could cause issues if the method fails.@php - $rdata = $question->rankingData() + try { + $rdata = $question->rankingData(); + } catch (\Exception $e) { + $rdata = []; + Log::error('Failed to fetch ranking data: ' . $e->getMessage()); + } @endphp
30-33: Enhance accessibility of the copy buttonThe copy button lacks proper ARIA attributes for screen readers.
- <button class="waves-effect waves-light btn" onclick="copyData('election-results-data')"> + <button class="waves-effect waves-light btn" + onclick="copyData('election-results-data')" + aria-label="Copy election results data to clipboard"> Copy data <i class="material-icons right">content_copy</i> </button>
43-51: Improve error handling in copyData functionThe error message should be more descriptive and potentially translated.
function copyData(id) { const data = document.getElementById(id).textContent; navigator.clipboard.writeText(data).then(() => { M.toast({html: '<span class="white-text">@lang("internet.copied")</span>'}); }).catch(err => { - M.toast({html: '<span class="white-text">Failed to copy data</span>'}); + M.toast({html: '<span class="white-text">@lang("internet.copy_failed"): ' + err.message + '</span>'}); + console.error('Copy failed:', err); }); }resources/views/secretariat/evaluation-form/anonymous_questions.blade.php (1)
26-32: Consider using a more maintainable question type handling approachThe current implementation might become difficult to maintain as new question types are added.
- @if($question->question_type == \App\Models\Question::TEXT_ANSWER) - @include('utils.questions.text_answer', ['question' => $question]) - @elseif($question->question_type == \App\Models\Question::SELECTION) - @include('utils.questions.selection', ['question' => $question]) - @elseif($question->question_type == \App\Models\Question::RANKING) - @include('utils.questions.ranking', ['question' => $question]) - @endif + @php + $viewMap = [ + \App\Models\Question::TEXT_ANSWER => 'text_answer', + \App\Models\Question::SELECTION => 'selection', + \App\Models\Question::RANKING => 'ranking', + ]; + $view = $viewMap[$question->question_type] ?? 'text_answer'; + @endphp + @include("utils.questions.{$view}", ['question' => $question])app/Livewire/ParentChildForm.php (1)
58-59: Consider adding validation before reindexingThe current implementation assumes the operation will always succeed.
+ if (!isset($this->items[$index])) { + return; + } unset($this->items[$index]); $this->items = array_values($this->items);app/Rules/RankingVote.php (1)
16-19: Enhance type safety in isSubset methodThe method should validate input types to prevent potential issues.
private function isSubset($subset, $superset) { + if (!is_array($subset) || !is_array($superset)) { + return false; + } return count(array_intersect($subset, $superset)) == count($subset); }resources/views/utils/questions/results/ranking_stats.blade.php (1)
13-15: Might I suggest moving the inline style to CSS?Rather than using inline styles, it would be more proper to define this in your stylesheet.
-<p style="font-size: 20px"> +<p class="details-heading"> @lang('general.details') </p>And in your CSS:
.details-heading { font-size: 20px; }app/Models/QuestionOption.php (1)
49-56: Might I suggest some refinements to the initials method?While the implementation is quite clever, there are a few edge cases we ought to handle:
- Empty strings
- Special characters
- Performance optimisation
public function initials(): string { + if (empty($this->title)) return ''; $letters = array(); - foreach(explode(' ', $this->title, 3) as $word) { + $words = array_slice(preg_split('/\s+/', trim($this->title)), 0, 3); + foreach($words as $word) { - $letters[] = substr($word, 0, 1); + $firstLetter = preg_replace('/[^a-zA-Z0-9]/', '', substr($word, 0, 1)); + if (!empty($firstLetter)) $letters[] = $firstLetter; } return implode($letters); }database/seeders/AnonymousQuestionSeeder.php (1)
31-40: Might I suggest some structural improvements?The question creation logic could be more maintainable by extracting common parameters and using constants for magic numbers.
+ private const SINGLE_CHOICE_OPTIONS = 4; + private const MULTIPLE_CHOICE_OPTIONS = 3; + private const DEFAULT_OPTIONS = [ + 'opened_at' => now(), + 'closed_at' => null, + ]; + public function run(): void { $faker = \Faker\Factory::create(); foreach(Semester::all()->filter( function (Semester $semester) {return $semester->isCurrent() || $semester->isClosed();} ) as $semester) { $singleChoice = Question::factory() ->for($semester, 'parent') - ->hasOptions(4) - ->create(['opened_at' => now(), 'closed_at' => null, 'max_options' => 1, 'question_type' => Question::SELECTION]); + ->hasOptions(self::SINGLE_CHOICE_OPTIONS) + ->create(array_merge(self::DEFAULT_OPTIONS, [ + 'max_options' => 1, + 'question_type' => Question::SELECTION + ]));database/seeders/GeneralAssemblySeeder.php (1)
24-29: Consider extracting question creation logic to a helper method.I say, the question creation logic appears rather repetitive. One might consider extracting it to a helper method for improved maintainability.
+ private function createQuestion($sitting, $title, $maxOptions) + { + return $sitting->questions()->create([ + 'title' => $title, + 'max_options' => $maxOptions, + 'question_type' => Question::SELECTION, + 'opened_at' => now() + ]); + }database/migrations/2025_02_06_214931_ranking_question.php (1)
21-26: Add index for question_type column.Given that we'll be querying by question_type frequently, I suggest adding an index for improved performance.
$table->enum('question_type', ['selection', 'text_answer', 'ranking'])->default('selection'); + $table->index('question_type');resources/lang/hu/voting.php (1)
16-16: Consider splitting the combined string into separate entries.The string 'max_options' combines two distinct concepts (maximum selectable options and number of seats) which could lead to confusion. Consider separating these into two distinct translation keys for better maintainability.
- 'max_options' => 'Legfeljebb hány opció választható ki/Mandátumok száma:', + 'max_selectable_options' => 'Legfeljebb hány opció választható ki:', + 'number_of_seats' => 'Mandátumok száma:',resources/lang/en/voting.php (1)
16-16: Consider splitting the combined string into separate entries.Similar to the Hungarian translation, the string 'max_options' combines two distinct concepts. Consider separating these for clarity and maintainability.
- 'max_options' => 'At most how many options should be chosen?/Number of seats?', + 'max_selectable_options' => 'At most how many options should be chosen?', + 'number_of_seats' => 'Number of seats to be allocated:',app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (1)
147-158: Ensure proper error handling in delete method.While the authorization and validation checks are good, consider adding:
- Transaction wrapping for data consistency
- Error handling for the delete operation
public function delete(Semester $semester, Question $question) { $this->authorize('administer', AnswerSheet::class); if($question['parent_type'] != AnswerSheet::class) { abort(400); } - $question->delete(); + try { + DB::transaction(function() use ($question) { + $question->delete(); + }); + } catch (\Exception $e) { + return back()->with('error', 'Failed to delete question'); + } return redirect(route('anonymous_questions.index_semesters')); }tests/Unit/AnonymousQuestionTest.php (1)
319-319: Consider adding explicit question type.While the default works, being explicit about the question type would make the test more readable.
- ->create(); + ->create(['question_type' => Question::SELECTION]);resources/views/student-council/general-assemblies/show.blade.php (1)
144-155: Well-implemented delete functionality with proper safeguards.Good implementation with:
- CSRF protection
- User confirmation
- Proper authorization check
- Clear visual indication (red button)
Consider adding a more descriptive confirmation message.
- <form method="post" action="{{ route('general_assemblies.questions.votes.delete', ['general_assembly' => $general_assembly, 'question' => $question]) }}" onsubmit="return confirm('Do you really want to delete the question?');"> + <form method="post" action="{{ route('general_assemblies.questions.votes.delete', ['general_assembly' => $general_assembly, 'question' => $question]) }}" onsubmit="return confirm('Are you sure you want to delete the question \"{{ $question->title }}\"? This action cannot be undone.');">routes/web.php (1)
290-291: Consider adding scopeBindings() to the general assembly question delete route.The route is missing the
scopeBindings()method which is used on other similar routes. This ensures that the question belongs to the specified general assembly.Route::delete('/general_assemblies/{general_assembly}/questions/{question}', [GeneralAssemblyQuestionController::class, 'delete']) - ->name('general_assemblies.questions.votes.delete'); + ->name('general_assemblies.questions.votes.delete') + ->scopeBindings();resources/sass/materialize.scss (1)
121-135: Consider enhancing accessibility for the ranking stats table.While the styling is well-structured, there are a few accessibility improvements to consider:
- The red/green colour scheme may be problematic for colour-blind users
- The text contrast ratios should be verified
Consider adding additional visual indicators and ensuring WCAG compliance:
table.ranking-stats td.winner.colored { color: green; + position: relative; + &::after { + content: "★"; + position: absolute; + top: 0; + right: 0; + } } table.ranking-stats td.colored { color: red; + position: relative; + &::after { + content: "×"; + position: absolute; + top: 0; + right: 0; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php(5 hunks)app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php(3 hunks)app/Http/Controllers/StudentsCouncil/QuestionController.php(1 hunks)app/Livewire/ParentChildForm.php(1 hunks)app/Models/AnonymousQuestions/AnswerSheet.php(2 hunks)app/Models/Question.php(7 hunks)app/Models/QuestionOption.php(1 hunks)app/Rules/RankingVote.php(1 hunks)composer.json(2 hunks)database/factories/QuestionFactory.php(2 hunks)database/migrations/2025_02_06_214931_ranking_question.php(1 hunks)database/seeders/AnonymousQuestionSeeder.php(1 hunks)database/seeders/GeneralAssemblySeeder.php(3 hunks)package.json(1 hunks)resources/js/site.js(0 hunks)resources/lang/en/general.php(1 hunks)resources/lang/en/voting.php(2 hunks)resources/lang/hu/general.php(1 hunks)resources/lang/hu/voting.php(2 hunks)resources/sass/materialize.scss(1 hunks)resources/views/layouts/app.blade.php(1 hunks)resources/views/secretariat/evaluation-form/anonymous_questions.blade.php(1 hunks)resources/views/student-council/anonymous-questions/index_semesters.blade.php(1 hunks)resources/views/student-council/general-assemblies/questions/show.blade.php(2 hunks)resources/views/student-council/general-assemblies/show.blade.php(2 hunks)resources/views/utils/question_card.blade.php(1 hunks)resources/views/utils/questions/ranking.blade.php(1 hunks)resources/views/utils/questions/results/ranking.blade.php(1 hunks)resources/views/utils/questions/results/ranking_stats.blade.php(1 hunks)resources/views/utils/questions/results/selection.blade.php(1 hunks)resources/views/utils/questions/results/text_answer.blade.php(1 hunks)resources/views/utils/questions/selection.blade.php(1 hunks)resources/views/utils/questions/text_answer.blade.php(1 hunks)routes/web.php(2 hunks)tests/Unit/AnonymousQuestionTest.php(2 hunks)webpack.mix.js(1 hunks)
💤 Files with no reviewable changes (1)
- resources/js/site.js
✅ Files skipped from review due to trivial changes (2)
- resources/views/utils/questions/text_answer.blade.php
- resources/lang/en/general.php
🔇 Additional comments (26)
resources/lang/hu/general.php (1)
21-21: Splendid addition to the translations!I say, the new translation entry has been rather thoughtfully placed in alphabetical order, and the Hungarian translation 'Részletek' for 'details' is quite spot-on. Jolly good work!
resources/views/utils/questions/results/selection.blade.php (1)
3-3:⚠️ Potential issuePrevent potential XSS vulnerabilities.
I must insist on properly escaping the option title using Laravel's
e()helper function to prevent cross-site scripting attacks.- <td>{{$option->title}}</td> + <td>{{ e($option->title) }}</td>Likely an incorrect or invalid review comment.
app/Http/Controllers/StudentsCouncil/GeneralAssemblyQuestionController.php (1)
13-13: Wise choice to extendQuestionController
Inheriting fromQuestionControllerelegantly consolidates question-related logic, reducing duplication and enhancing maintainability across controllers.app/Models/Question.php (5)
75-78: Soft-deletion and new fillable attributes
EnablingSoftDeletesand addingquestion_typeplusresults_cacheto$fillablefields is an excellent way to preserve historical data and prepare for advanced question types.
79-83: Clear enumeration of question types
DefiningSELECTION,TEXT_ANSWER, andRANKINGin constants, along with an array of valid types, is a clean approach ensuring easy reference and robust validation.
173-183: Result caching concerns
The logic for computing and storing STV results withinclose()is well structured. If there is a chance of rerunning or re-closing a question, ensure you avoid overwriting existing cached results inadvertently or consider incremental updates if necessary.
254-255: Guard against incompatible text answers
The exception thrown here pre-empts any improper usage of text answers for non-text or non-ranking question types. This defensive approach helps maintain data integrity.
318-346: Contextual validation rules
Using$this->formKey()ensures no collisions within the request, and switching logic based onquestion_typeis laudable. The approach for multiple-choice selection with'max:' . $this->max_optionsis effective, though watch for potential edge cases wheremax_optionsmight need further validation or constraints.resources/views/utils/questions/results/text_answer.blade.php (1)
1-5: Splendid implementation, I must say!The template elegantly handles the display of text answers with proper escaping. Most satisfactory indeed.
resources/views/utils/question_card.blade.php (1)
11-13: Most excellent form structure, I must say!The question type selection is implemented quite elegantly using the custom select component. The localisation support is particularly well thought out.
webpack.mix.js (1)
20-21: Kindly verify the version of sortablejs package.I notice you've added the Sortable.min.js library. While this is a splendid choice for implementing drag-and-drop ranking functionality, we ought to ensure we're using a secure version.
✅ Verification successful
🌐 Web query:
What is the latest version of sortablejs and are there any known security vulnerabilities?💡 Result:
The latest version of SortableJS is v1.15.6, released on November 28, 2024[1]. This update included fixes for reverting nested sortables and added the
forceAutoScrollFallbackoption[1].Regarding security vulnerabilities:
- No direct vulnerabilities have been reported for SortableJS in Snyk's vulnerability database as of February 2025[4][9]
- The @vadevteam/sortablejs fork also shows no direct vulnerabilities[5]
- A low-severity version disclosure advisory exists (CWE-200) where server headers might expose the SortableJS version number[8], though this doesn't directly impact functionality
Recent updates have addressed:
- Multi-drag selection issues on iOS devices (v1.15.6)[1]
- Text input interaction problems during drag operations (v1.15.5)[1]
- Nested list errors and Safari pointer event handling (v1.13.0)[1]
Developers should monitor open issues like:
- MultiDragPlugin errors in nested sortables (#2420)[2]
- Element flickering during previews (#2411)[2]
- Click event issues when using
pull: 'clone'(#2397)[2]The library maintains active development with regular updates addressing browser compatibility and edge case behaviors[1][7].
Citations:
- 1: https://github.com/SortableJS/Sortable/releases
- 2: https://github.com/SortableJS/Sortable/issues
- 3: https://www.cisa.gov/news-events/news/apache-log4j-vulnerability-guidance
- 4: https://security.snyk.io/package/npm/sortablejs
- 5: https://security.snyk.io/package/npm/@vadevteam%2Fsortablejs
- 6: https://www.wolfssl.com/docs/security-vulnerabilities/
- 7: https://www.youtube.com/watch?v=7WAJx3EyyZk
- 8: https://www.invicti.com/web-vulnerability-scanner/vulnerabilities/version-disclosure-sortablejs/
- 9: https://security.snyk.io/package/npm/npm-sortablejs
- 10: https://www.commoninja.com/blog/using-drag-and-drop-react-apps-sortablejs
SortableJS Version and Security Verification
The latest version of SortableJS is v1.15.6, and there are no significant security vulnerabilities reported beyond a low-severity version disclosure advisory. The inclusion of the "Sortable.min.js" file in your webpack.mix.js appears to be secure and in line with our requirements for the new ranking functionality.
resources/lang/hu/voting.php (1)
58-59: LGTM! Clear and informative voting instructions.The ranking instructions and explanation effectively communicate the Single Transferable Vote mechanism to users. The language is clear and the explanation of vote transfer is particularly well-crafted.
resources/lang/en/voting.php (1)
58-63: LGTM! Well-structured voting instructions.The English translations for ranking instructions, explanations, and UI guidance are clear, concise, and effectively communicate the Single Transferable Vote mechanism.
app/Models/AnonymousQuestions/AnswerSheet.php (1)
101-101: LGTM! Improved type checking using constants.The change from
has_long_answersboolean toquestion_typeconstant improves code clarity and maintainability. This change aligns well with the Single Transferable Vote implementation by providing a more flexible question type system.resources/views/layouts/app.blade.php (1)
64-64: LGTM! Enhanced modularity with stack directives.The addition of
@stackdirectives for styles and scripts improves modularity and allows components (like the new voting interface) to inject their required resources.Also applies to: 126-126
app/Http/Controllers/StudentsCouncil/AnonymousQuestionController.php (3)
22-22: Excellent use of inheritance to promote code reuse!The change to extend QuestionController centralises question-related functionality and reduces code duplication.
73-73: Good refactoring to use createQuestion method.The change simplifies the store method by delegating question creation to a dedicated method in the parent controller.
124-126: Well-structured loop with proper encapsulation.The use of saveVoteForQuestion method improves code maintainability and readability.
resources/views/student-council/general-assemblies/questions/show.blade.php (3)
30-38: Well-structured conditional blocks for question types.Good use of blade directives to handle different question types with clear instructions for each type.
40-47: Excellent modularization using component includes.The use of separate components for different question types improves maintainability and reusability.
84-92: Good separation of concerns in results rendering.The conditional includes for results maintain consistency with the question type structure.
tests/Unit/AnonymousQuestionTest.php (1)
285-285: Good update to test question type attribute.The test correctly reflects the model changes from has_long_answers to question_type.
routes/web.php (1)
336-336: LGTM! The anonymous question delete route is well-structured.The route follows RESTful conventions and uses the appropriate HTTP method.
package.json (1)
26-26: LGTM! The SortableJS package addition is appropriate.The package version is recent and stable, and the use of the caret (^) allows for safe minor version updates while maintaining compatibility.
composer.json (2)
81-86: Verify the long-term maintenance plan for the forked repository.Using a custom fork of the Condorcet package could pose maintenance challenges. Please ensure there's a plan to:
- Keep the fork in sync with upstream
- Contribute changes back to the main repository
- Handle security updates
14-14: Consider using a stable version or adding version constraints.Using a development branch (
dev-stv-tie) without version constraints could lead to unexpected behaviour if the branch is updated.✅ Verification successful
🌐 Web query:
What is the latest stable version of julien-boudry/condorcet?💡 Result:
The latest stable version of julien-boudry/Condorcet is v4.7.0, released on June 14, 2024[1][10]. This version requires PHP ≥8.3 and is actively supported[1]. It follows previous versions like v4.6.0 (January 2024) and v4.5.0 (October 2023), which are now considered older stable releases[10].
Citations:
- 1: https://github.com/julien-boudry/Condorcet
- 2: https://www.condorcet.io/2.AsCommandLineApplication/1.Installation
- 3: https://arxiv.org/html/2412.13809v1
- 4: https://www.getty.edu/publications/resources/virtuallibrary/0892365803.pdf
- 5: https://electowiki.org/wiki/Condorcet_PHP
- 6: https://electowiki.org/wiki/Online_poll
- 7: https://cultureunbound.ep.liu.se/article/download/4933/4400
- 8: https://php-download.com/package/julien-boudry/condorcet
- 9: https://oll.libertyfund.org/titles/condorcet-outlines-of-an-historical-view-of-the-progress-of-the-human-mind
- 10: https://github.com/julien-boudry/Condorcet/releases
Action: Revisit dependency specification in composer.json
The dependency on
"julien-boudry/condorcet": "dev-stv-tie"is currently configured to track a development branch without any version constraint. Given that the latest stable release is v4.7.0, relying on the development branch may introduce instability if the branch is updated unexpectedly.
- File: composer.json, line 14
- Current State:
"julien-boudry/condorcet": "dev-stv-tie"- Recommendation: Switch to a stable version (e.g.
"^4.7.0") or, if the development branch is necessary, include appropriate version constraints to minimise potential risks.
| protected function createQuestion(Request $request, Semester|GeneralAssembly $parent = null): Question | ||
| { | ||
| $validatedData = $request->validate([ | ||
| 'title' => 'required|string', | ||
| 'question_type' => [ | ||
| 'required', | ||
| Rule::in(Question::QUESTION_TYPES) | ||
| ], | ||
| 'max_options' => ['required', 'min:1', Rule::excludeIf($request['question_type'] == QUESTION::TEXT_ANSWER), 'integer'], | ||
| 'options' => ['required', 'min:1', Rule::excludeIf($request['question_type'] != QUESTION::SELECTION && $request['question_type'] != QUESTION::RANKING), 'array'], | ||
| 'options.*' => ['required', 'min:1', 'max:255', Rule::excludeIf($request['question_type'] != QUESTION::SELECTION && $request['question_type'] != QUESTION::RANKING), 'string'], | ||
| ]); | ||
| if ($validatedData['question_type'] == Question::SELECTION || $validatedData['question_type'] == Question::RANKING) { | ||
| $options = array_filter($validatedData['options'], function ($s) { | ||
| return $s != null; | ||
| }); | ||
| if (count($options) == 0) { | ||
| $validator->after(function ($validator) { | ||
| $validator->errors()->add('options', __('voting.at_least_one_option')); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential reference to undefined $validator object
It appears that the method relies on $request->validate(...) but then attempts to use $validator->after(...) at lines 45–48, though $validator was never defined. This could cause an error at runtime. Consider explicitly creating a validator instance via Validator::make(...) if you need to add an after() callback.
- $validatedData = $request->validate([
- ...
- ]);
- ...
- if (count($options) == 0) {
- $validator->after(function ($validator) {
- ...
- });
- }
+ $validator = Validator::make($request->all(), [
+ ...
+ ]);
+ $validatedData = $validator->validate();
+ if (count($options) == 0) {
+ $validator->after(function ($validator) {
+ ...
+ });
+ }Committable suggestion skipped: line range outside the PR's diff.
| <tr><td colspan="2"> | ||
| <table class="ranking-stats"> |
There was a problem hiding this comment.
I say, there appears to be a table structure issue.
The table markup contains several structural issues:
- The outer
<tr><td>wrapping is peculiar and unnecessary - There's a mismatched closing tag (
<table>instead of</table>)
-<tr><td colspan="2">
<table class="ranking-stats">
-</tr>
+ </table>Also applies to: 54-54
| <input type="hidden" name="_method" value="DELETE"> | ||
| <x-input.button | ||
| type="submit" | ||
| floating | ||
| class="right red" | ||
| icon="delete" | ||
| /> | ||
| <b style="font-size: 110%;">{{$question->title}}</b> | ||
| @if($question->has_long_answers) | ||
| <form method="post" action="{{ route('anonymous_questions.delete', ['semester' => $semester, 'question' => $question]) }}" onsubmit="return confirm('Do you really want to delete the question?');"> | ||
| @csrf | ||
| </form> |
There was a problem hiding this comment.
Restructure the form for better security and usability.
I must say, the form structure requires attention. The method field should be within the form element, and the button should be properly associated.
- <input type="hidden" name="_method" value="DELETE">
- <x-input.button
- type="submit"
- floating
- class="right red"
- icon="delete"
- />
- <form method="post" action="{{ route('anonymous_questions.delete', ['semester' => $semester, 'question' => $question]) }}" onsubmit="return confirm('Do you really want to delete the question?');">
- @csrf
- </form>
+ <form method="post"
+ action="{{ route('anonymous_questions.delete', ['semester' => $semester, 'question' => $question]) }}"
+ onsubmit="return confirm('{{ __('messages.confirm_delete') }}');"
+ class="right">
+ @csrf
+ @method('DELETE')
+ <x-input.button
+ type="submit"
+ floating
+ class="red"
+ icon="delete"
+ />
+ </form>📝 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.
| <input type="hidden" name="_method" value="DELETE"> | |
| <x-input.button | |
| type="submit" | |
| floating | |
| class="right red" | |
| icon="delete" | |
| /> | |
| <b style="font-size: 110%;">{{$question->title}}</b> | |
| @if($question->has_long_answers) | |
| <form method="post" action="{{ route('anonymous_questions.delete', ['semester' => $semester, 'question' => $question]) }}" onsubmit="return confirm('Do you really want to delete the question?');"> | |
| @csrf | |
| </form> | |
| <b style="font-size: 110%;">{{$question->title}}</b> | |
| <form method="post" | |
| action="{{ route('anonymous_questions.delete', ['semester' => $semester, 'question' => $question]) }}" | |
| onsubmit="return confirm('{{ __('messages.confirm_delete') }}');" | |
| class="right"> | |
| @csrf | |
| @method('DELETE') | |
| <x-input.button | |
| type="submit" | |
| floating | |
| class="red" | |
| icon="delete" | |
| /> | |
| </form> |
| public function up(): void | ||
| { | ||
| Schema::table('long_answers', function (Blueprint $table) { | ||
| $table->dropForeign(['question_id']); | ||
| $table->dropForeign(['answer_sheet_id']); | ||
|
|
||
| $table->foreign('question_id')->references('id')->on('questions')->onDelete('cascade'); | ||
| $table->foreign('answer_sheet_id')->references('id')->on('answer_sheets')->onDelete('cascade'); | ||
| }); | ||
| Schema::table('questions', function (Blueprint $table) { | ||
| $table->enum('question_type', ['selection', 'text_answer', 'ranking'])->default('selection'); | ||
| $table->integer('max_options')->nullable()->change(); | ||
| $table->text('results_cache')->nullable(); | ||
| $table->timestamps(); | ||
| $table->softDeletes(); | ||
| }); | ||
| Schema::table('long_answers', function (Blueprint $table) { | ||
| $table->unsignedBigInteger('answer_sheet_id')->nullable()->change(); | ||
| }); | ||
| DB::table('questions')->where('has_long_answers', 1)->update(['question_type' => 'text_answer']); | ||
| Schema::table('questions', function (Blueprint $table) { | ||
| $table->dropColumn('has_long_answers'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap migration in a transaction for atomicity.
I believe it would be prudent to wrap these operations in a transaction to ensure atomicity.
public function up(): void
{
+ DB::beginTransaction();
+ try {
// ... existing code ...
+ DB::commit();
+ } catch (\Exception $e) {
+ DB::rollBack();
+ throw $e;
+ }
}An empty array is equal to `null` when checked with `==`. Use identity comparison (`===`) instead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/Rules/RankingVote.php (4)
11-14: Kindly declare the question property with proper type hinting.I say, it would be most appropriate to declare the class property with proper visibility and type hinting.
+ private Question $question; + public function __construct(Question $question) { $this->question = $question; }
16-24: Rather splendid methods, but they could do with some proper documentation and type hints.These helper methods would benefit from proper PHP type hints and documentation.
+ /** + * Check if one array is a subset of another. + * + * @param array<int> $subset + * @param array<int> $superset + * @return bool + */ - private function isSubset($subset, $superset) + private function isSubset(array $subset, array $superset): bool { return count(array_intersect($subset, $superset)) == count($subset); } + /** + * Check if array contains only unique elements. + * + * @param array<int> $array + * @return bool + */ - private function isArrUni($array) + private function isArrUni(array $array): bool { return count($array) === count(array_unique($array)); }
52-56: A more efficient approach to collecting valid IDs would be rather welcome.The current implementation could be optimised using array mapping.
- $valid_ids = []; - foreach($this->question->options as $option) { - $valid_ids[] = $option->id; - } + $valid_ids = $this->question->options->pluck('id')->toArray();
26-31: The docblock could do with a bit more detail, if I may say so.Adding parameter and return type documentation would improve clarity.
/** * Run the validation rule. * + * @param string $attribute The name of the attribute being validated + * @param mixed $value The value being validated * @param \Closure(string, ?string=): \Illuminate\Translation\PotentiallyTranslatedString $fail + * @return void */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Rules/RankingVote.php(1 hunks)
🔇 Additional comments (1)
app/Rules/RankingVote.php (1)
33-41: I must concur with the previous review regarding JSON handling.The JSON decoding could indeed be more robust with proper error handling.
- $converted = json_decode($value); - if ($converted === null) { - $fail("The value must be a JSON string."); + $converted = json_decode($value, true); + if (json_last_error() !== JSON_ERROR_NONE) { + $fail("Invalid JSON format: " . json_last_error_msg()); return; }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Thank you very much; it is nice!
One thing: some kind of seeder for ranking questions is sorely missing; that would make testing much easier. Even if not with randomised data, but some fixed ballots.
Most of my other comments are nitpicks; check them out if you have the time.
| use App\Exports\UsersSheets\AnonymousQuestionsExport; | ||
|
|
||
| /** | ||
| * Controls actions related to anonymous questions. |
There was a problem hiding this comment.
Is this comment up-to-date?
| @@ -65,8 +72,15 @@ | |||
| class Question extends Model | |||
| { | |||
| use HasFactory; | |||
| use SoftDeletes; | |||
|
|
|||
| protected $fillable = ['title', 'max_options', 'opened_at', 'closed_at', 'question_type', 'results_cache']; | |||
There was a problem hiding this comment.
Maybe a comment for max_options (that explains what it means for ranking questions) would be of use later.
There was a problem hiding this comment.
The thing is that repurposing max_options is a hack, and we should come up with a better way to store metadata that's only relevant to a certain type of question. Anyway, I'll leave a comment for now.
| @@ -226,11 +251,12 @@ public function storeAnswers(User $user, QuestionOption|array|string $answer, ?A | |||
| } | |||
| } | |||
| } // else it is a string | |||
| elseif (!$this->has_long_answers) { | |||
| elseif ($this->question_type != Question::TEXT_ANSWER && | |||
| $this->question_type != Question::RANKING) { | |||
There was a problem hiding this comment.
Oh, so is it like a text question but with a JSON as the textual answer? Or do I understand something wrong?
There was a problem hiding this comment.
Yup, exactly. A JSON string containing an array of (numeric) answer IDs.
app/Rules/RankingVote.php
Outdated
| $this->question = $question; | ||
| } | ||
|
|
||
| private function isSubset($subset, $superset) |
There was a problem hiding this comment.
Nitpick: maybe this could be static.
app/Rules/RankingVote.php
Outdated
| return count(array_intersect($subset, $superset)) == count($subset); | ||
| } | ||
|
|
||
| private function isArrUni($array) |
| @if($question->question_type == \App\Models\Question::TEXT_ANSWER) | ||
| @include('utils.questions.results.text_answer', ['question' => $question]) | ||
| @elseif($question->question_type == \App\Models\Question::SELECTION) | ||
| @include('utils.questions.results.selection', ['question' => $question]) | ||
| @elseif($question->question_type == \App\Models\Question::RANKING) | ||
| @include('utils.questions.results.ranking', ['question' => $question]) |
| 'text_answer' => 'Szöveges válasz', | ||
| 'ranking' => 'Preferenciális szavazás', | ||
| ], | ||
| 'ranking_instructions' => 'Húzd a jelöltek neveit az általad preferált sorrendben a fenti részre (legfelülre azt, akit legjobban támogatsz). Tetszőleges számú jelöltet rangsorolhatsz; hagyd a vonal alatt azokat, akiket semmiképp sem támogatsz.', |
There was a problem hiding this comment.
We should add some instructions on abstention; like 'Tartózkodáshoz hagyd az összes jelöltet a vonal alatt.' Same for English.
resources/lang/en/voting.php
Outdated
| 'drag_to_rank' => 'Drag options here to rank them', | ||
| 'drag_to_exclude' => 'Drag options here to exclude from ranking', |
There was a problem hiding this comment.
Nitpick: should we drag this up to keep the alphabetical order?
resources/lang/hu/voting.php
Outdated
| 'drag_to_rank' => 'Húzd ide az opciókat a sorrend felállításához', | ||
| 'drag_to_exclude' => 'Húzd ide az opciókat, amikre nem kívánsz szavazni', |
This is the version we use in deployment, and the use of the Condorcet library in #708 will require PHP 8.3.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/Models/Question.php (1)
283-309: Consider caching the ranking data.The
rankingDatamethod performs multiple database queries and JSON operations on each call. Consider caching the results.public function rankingData() { + return cache()->remember("question_{$this->id}_ranking_data", 60, function () { $obj = array(); if($this->question_type == self::RANKING) { // ... existing code ... return $obj; } else { throw new \Exception("rankingData queried for a non-ranking question"); } + }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Http/Controllers/StudentsCouncil/QuestionController.php(1 hunks)app/Models/Question.php(7 hunks)app/Rules/RankingVote.php(1 hunks)resources/lang/en/voting.php(3 hunks)resources/lang/hu/voting.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Http/Controllers/StudentsCouncil/QuestionController.php
- app/Rules/RankingVote.php
- resources/lang/en/voting.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: phpunit
🔇 Additional comments (3)
resources/lang/hu/voting.php (3)
13-14: LGTM! Clear drag-and-drop instructions.The new strings provide clear instructions for the drag-and-drop functionality in both ranking and exclusion scenarios.
18-18: Consider enhancing the max_options description.The string could be more self-explanatory by clearly distinguishing its dual purpose.
- 'max_options' => 'Legfeljebb hány opció választható ki/Mandátumok száma:', + 'max_options' => 'Legfeljebb hány opció választható ki (feleletválasztós kérdésnél) vagy a kiosztandó mandátumok száma (rangsorolásos kérdésnél):',
61-62: Consider adding abstention instructions.The ranking instructions should include guidance on how to abstain from voting.
- 'ranking_instructions' => 'Húzd a jelöltek neveit az általad preferált sorrendben a fenti részre (legfelülre azt, akit legjobban támogatsz). Tetszőleges számú jelöltet rangsorolhatsz; hagyd a vonal alatt azokat, akiket semmiképp sem támogatsz.', + 'ranking_instructions' => 'Húzd a jelöltek neveit az általad preferált sorrendben a fenti részre (legfelülre azt, akit legjobban támogatsz). Tetszőleges számú jelöltet rangsorolhatsz; hagyd a vonal alatt azokat, akiket semmiképp sem támogatsz. Tartózkodáshoz hagyd az összes jelöltet a vonal alatt.',
| /* | ||
| * HACK: max_option is used for both: | ||
| * - number of permitted options to select in a multiple-choice question | ||
| * - number of seats in a ranking question (required for quota calculation) | ||
| */ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a dedicated field for seats.
The current use of max_options for both selection limit and seat count is a hack, as noted in the comment. This could lead to confusion and maintenance issues.
Consider adding a dedicated seats field for ranking questions:
+ protected $fillable = ['title', 'max_options', 'opened_at', 'closed_at', 'question_type', 'results_cache', 'seats'];Then update the migration to add this field:
ALTER TABLE questions ADD COLUMN seats INTEGER NULL;| private function computeElectionResults(): Result | ||
| { | ||
| if($this->question_type != self::RANKING) { | ||
| throw new Exception('This question is not a ranking question'); | ||
| } | ||
|
|
||
| $election = new Election(); | ||
|
|
||
| $election->setNumberOfSeats($this->max_options); | ||
| foreach($this->options as $option) { | ||
| $election->addCandidate($option->id); | ||
| } | ||
| foreach($this->longAnswers as $ballot) { | ||
| $preferences = json_decode($ballot->text); | ||
| if(count($preferences) == 0) { | ||
| continue; | ||
| } | ||
|
|
||
| $election->addVote(array_map('strval', $preferences)); | ||
| } | ||
|
|
||
| return $election->getResult('STV'); | ||
| } |
There was a problem hiding this comment.
Validate ballot data before processing.
The computeElectionResults method doesn't validate the structure of the ballot data before processing it.
private function computeElectionResults(): Result
{
foreach($this->longAnswers as $ballot) {
$preferences = json_decode($ballot->text);
+ if (!is_array($preferences)) {
+ throw new Exception('Invalid ballot format: expected array');
+ }
+ foreach ($preferences as $preference) {
+ if (!is_numeric($preference) || !in_array($preference, $this->options->pluck('id')->toArray())) {
+ throw new Exception('Invalid ballot: contains invalid option ID');
+ }
+ }
if(count($preferences) == 0) {
continue;
}
$election->addVote(array_map('strval', $preferences));
}📝 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.
| private function computeElectionResults(): Result | |
| { | |
| if($this->question_type != self::RANKING) { | |
| throw new Exception('This question is not a ranking question'); | |
| } | |
| $election = new Election(); | |
| $election->setNumberOfSeats($this->max_options); | |
| foreach($this->options as $option) { | |
| $election->addCandidate($option->id); | |
| } | |
| foreach($this->longAnswers as $ballot) { | |
| $preferences = json_decode($ballot->text); | |
| if(count($preferences) == 0) { | |
| continue; | |
| } | |
| $election->addVote(array_map('strval', $preferences)); | |
| } | |
| return $election->getResult('STV'); | |
| } | |
| private function computeElectionResults(): Result | |
| { | |
| if($this->question_type != self::RANKING) { | |
| throw new Exception('This question is not a ranking question'); | |
| } | |
| $election = new Election(); | |
| $election->setNumberOfSeats($this->max_options); | |
| foreach($this->options as $option) { | |
| $election->addCandidate($option->id); | |
| } | |
| foreach($this->longAnswers as $ballot) { | |
| $preferences = json_decode($ballot->text); | |
| if (!is_array($preferences)) { | |
| throw new Exception('Invalid ballot format: expected array'); | |
| } | |
| foreach ($preferences as $preference) { | |
| if (!is_numeric($preference) || !in_array($preference, $this->options->pluck('id')->toArray())) { | |
| throw new Exception('Invalid ballot: contains invalid option ID'); | |
| } | |
| } | |
| if(count($preferences) == 0) { | |
| continue; | |
| } | |
| $election->addVote(array_map('strval', $preferences)); | |
| } | |
| return $election->getResult('STV'); | |
| } |
| if($this->question_type == self::RANKING) { | ||
| // 'results' will contain the final vote counts and | ||
| // 'stats' the table with the vote counts in each round | ||
| $resultObject = $this->computeElectionResults(); | ||
| $toWriteToCache = array(); | ||
| $toWriteToCache['results'] = $resultObject->getResultAsArray(true); | ||
| $toWriteToCache['stats'] = $resultObject->getStats(); | ||
| $this->update( | ||
| ['results_cache' => json_encode($toWriteToCache)] | ||
| ); | ||
| } | ||
| $this->update(['closed_at' => now()]); |
There was a problem hiding this comment.
Ensure error handling for election results computation.
The election results computation could fail if there are no valid ballots or if the Condorcet library throws an exception.
public function close(): void
{
if($this->question_type == self::RANKING) {
- $resultObject = $this->computeElectionResults();
- $toWriteToCache = array();
- $toWriteToCache['results'] = $resultObject->getResultAsArray(true);
- $toWriteToCache['stats'] = $resultObject->getStats();
- $this->update(
- ['results_cache' => json_encode($toWriteToCache)]
- );
+ try {
+ $resultObject = $this->computeElectionResults();
+ $toWriteToCache = array();
+ $toWriteToCache['results'] = $resultObject->getResultAsArray(true);
+ $toWriteToCache['stats'] = $resultObject->getStats();
+ $this->update(
+ ['results_cache' => json_encode($toWriteToCache)]
+ );
+ } catch (Exception $e) {
+ throw new Exception("Failed to compute election results: " . $e->getMessage());
+ }
}📝 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.
| if($this->question_type == self::RANKING) { | |
| // 'results' will contain the final vote counts and | |
| // 'stats' the table with the vote counts in each round | |
| $resultObject = $this->computeElectionResults(); | |
| $toWriteToCache = array(); | |
| $toWriteToCache['results'] = $resultObject->getResultAsArray(true); | |
| $toWriteToCache['stats'] = $resultObject->getStats(); | |
| $this->update( | |
| ['results_cache' => json_encode($toWriteToCache)] | |
| ); | |
| } | |
| $this->update(['closed_at' => now()]); | |
| public function close(): void | |
| { | |
| if($this->question_type == self::RANKING) { | |
| try { | |
| $resultObject = $this->computeElectionResults(); | |
| $toWriteToCache = array(); | |
| $toWriteToCache['results'] = $resultObject->getResultAsArray(true); | |
| $toWriteToCache['stats'] = $resultObject->getStats(); | |
| $this->update( | |
| ['results_cache' => json_encode($toWriteToCache)] | |
| ); | |
| } catch (Exception $e) { | |
| throw new Exception("Failed to compute election results: " . $e->getMessage()); | |
| } | |
| } | |
| $this->update(['closed_at' => now()]); | |
| } |
| if($question['parent_type'] != AnswerSheet::class) { | ||
| abort(400); | ||
| } |
There was a problem hiding this comment.
probably irrrelevant with scopeBindings() as that ensurers the question is the child of the specified general assembly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
database/seeders/GeneralAssemblySeeder.php (2)
24-69: Consider extracting test data to a more maintainable structure.Rather than having the question and option data hardcoded, it would be more maintainable to define them in a separate configuration array or constant. This would make it easier to modify the test data and reduce duplication.
Here's a suggested approach:
private const TEST_QUESTIONS = [ 'election_support' => [ 'title' => "I support the election of the new Students' Council.", 'max_options' => 1, 'question_type' => Question::SELECTION, 'options' => [ ['title' => 'Yes'], ['title' => 'No'], ['title' => 'I abstain'], ], ], 'curatorium_multiple' => [ 'title' => 'Curatorium members (multiple choice)', 'max_options' => 3, 'question_type' => Question::SELECTION, 'options' => [ ['title' => 'A'], ['title' => 'B'], ['title' => 'C'], ['title' => 'D'], ['title' => 'E'], ['title' => 'I abstain'], ], ], ];
71-104: Consider using more contextually appropriate test data.While the Ghostbusters characters are delightful, it might be more appropriate to use names that better reflect the actual use case of a student council election. This would make the test data more meaningful and easier to understand in context.
Consider using more realistic student names or placeholder formats like "Candidate A", "Candidate B", etc.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
database/seeders/GeneralAssemblySeeder.php(4 hunks)resources/lang/en/voting.php(3 hunks)resources/lang/hu/voting.php(3 hunks)resources/views/utils/questions/results/ranking_stats.blade.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/utils/questions/results/ranking_stats.blade.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: phpunit
🔇 Additional comments (9)
resources/lang/hu/voting.php (4)
13-14: LGTM! Clear and concise instructions for the drag-and-drop functionality.
18-18: Consider improving clarity of the max_options text.Based on previous feedback, consider making it more self-explanatory:
"Legfeljebb hány opció választható ki (vagy rangsorolásos kérdésnél a kiosztandó mandátumok száma)"
23-26: LGTM! Clear and accurate translations for voting statistics.Also applies to: 47-47
64-64: Consider adding explicit abstention instructions.Based on previous feedback, consider adding explicit instructions for abstention:
"Tartózkodáshoz hagyd az összes jelöltet a vonal alatt."resources/lang/en/voting.php (4)
13-14: LGTM! Clear and concise instructions for the drag-and-drop functionality.
18-18: Consider improving clarity of the max_options text.Based on previous feedback, consider using:
"Maximal number of options chosen (or for ranking questions, the number of seats to be distributed)"
23-26: LGTM! Clear and accurate translations for voting statistics.Also applies to: 47-47
64-64: Consider adding explicit abstention instructions.Based on previous feedback, consider adding:
"To abstain from voting, leave all candidates below the line."database/seeders/GeneralAssemblySeeder.php (1)
7-7: Splendid addition of the Question model import!The import is necessary for utilising the Question constants in the seeder.
| // generating random ranking votes | ||
| $rankingVoteCount = rand(0, 300); | ||
| for ($i = 0; $i < $rankingVoteCount; ++$i) { | ||
| shuffle($rankingOptionIds); | ||
| $orderedOptionCount = rand(0, count($rankingOptionIds) - 1); | ||
| $answer = array_slice($rankingOptionIds, 0, $orderedOptionCount); | ||
| // we won't connect them to users, | ||
| // just write them among the answers | ||
| $openRankingQuestion->longAnswers()->create([ | ||
| 'text' => json_encode($answer) | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance vote generation with validation and consistent data.
The current implementation has several areas that could be improved:
- The random vote count (0-300) might not provide consistent test scenarios
- There's no validation of the ranking order
- The JSON structure isn't documented
Consider these improvements:
- $rankingVoteCount = rand(0, 300);
+ $rankingVoteCount = 100; // Fixed number for consistent testing
for ($i = 0; $i < $rankingVoteCount; ++$i) {
shuffle($rankingOptionIds);
- $orderedOptionCount = rand(0, count($rankingOptionIds) - 1);
+ // Ensure at least 3 candidates are ranked for meaningful STV testing
+ $orderedOptionCount = rand(3, count($rankingOptionIds));
$answer = array_slice($rankingOptionIds, 0, $orderedOptionCount);
+ // Validate the ranking order
+ assert(count($answer) >= 3, 'Each vote must rank at least 3 candidates');
$openRankingQuestion->longAnswers()->create([
- 'text' => json_encode($answer)
+ 'text' => json_encode($answer, JSON_THROW_ON_ERROR)
]);
}Also, consider adding a PHPDoc comment to document the JSON structure:
/**
* Generate random ranking votes.
* The votes are stored as JSON arrays of option IDs in order of preference.
* Example: [5, 2, 8] means option 5 is ranked first, 2 second, and 8 third.
*/
description TODO