E2602 - Reimplement student task view#352
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughStudentTask model gains rich state tracking and teammate/timeline lookup features; deadline-type constants enable stage resolution. Controllers add participant-driven show endpoint and team endpoint. Grades heatgrid adds section-header injection and response improvements. Tests comprehensively cover all new behavior. ChangesStudent Task Lifecycle and API Endpoint Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 7 | ❌ 7❌ Failed checks (6 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Thanks for the pull request, and welcome! The Expertiza team is excited to review your changes, and you should hear from us soon. Please make sure the PR passes all checks and you have run This repository is being automatically checked for code-quality issues using CodeRabbit, Danger, and CI workflows. Please address newly introduced issues before marking the PR ready for review. Also, please spend some time looking at the instructions at the top of your course project writeup. Your pull request is more than 500 LoC. One or more of your test expectations only focus on the return value not being Generated by 🚫 Danger |
|
🚨 RSpec Tests Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/assignment_team.rb (1)
160-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate method block overrides earlier team-cleanup behavior.
Line 183 redefines
remove_participantand overrides the earlier implementation (Line 145), droppingdestroy if participants.empty?. Because Ruby uses the last definition, empty teams are no longer auto-destroyed.
🧹 Nitpick comments (3)
spec/models/student_task_spec.rb (1)
242-298: ⚡ Quick winAdd regression specs for ordering and submitted-only timeline behavior.
The new suite doesn’t lock two changed contracts:
.from_usermulti-key ordering and exclusion of unsubmittedResponserecords from.get_timeline_data. Please add explicit examples for both to prevent silent regressions.As per coding guidelines, "Check that model specs cover validations, associations, callbacks, and important edge cases with meaningful assertions instead of shallow examples."
Source: Coding guidelines
config/routes.rb (1)
52-53: ⚡ Quick winAvoid duplicate show paths; keep one canonical
showroute
resources :student_tasksalready providesGET /student_tasks/:id. AddingGET /student_tasks/show/:idcreates duplicate endpoint contracts for the same action and increases routing/spec/swagger drift risk.Suggested fix
resources :student_tasks do collection do get :list, action: :list - get 'show/:id', action: :show get :team, action: :team end endAs per coding guidelines,
config/routes.rbshould flag duplicate or shadowed routes and surprising non-RESTful patterns.Source: Coding guidelines
spec/requests/api/v1/student_tasks_controller_spec.rb (1)
179-183: ⚡ Quick winStrengthen
/student_tasks/teamsuccess assertions beyond type-checkingAsserting only
Hashwill pass even if grouping/output semantics regress. Add fixture data and assert at least one expected course key and teammate name list to lock the API contract.As per coding guidelines,
spec/requests/**/*.rbtests should include meaningful expectations and avoid shallow assertions that only prove superficial structure.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a154aa7-bbca-405c-a95a-fac228b63523
📒 Files selected for processing (10)
app/controllers/student_tasks_controller.rbapp/models/assignment_team.rbapp/models/concerns/expertiza_constants.rbapp/models/due_date.rbapp/models/student_task.rbconfig/routes.rbspec/models/due_date_spec.rbspec/models/student_task_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rbspec/routing/student_tasks_routing_spec.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (6)
spec/models/**/*.rb
⚙️ CodeRabbit configuration file
spec/models/**/*.rb: Check that model specs cover validations, associations, callbacks, and important edge cases
with meaningful assertions instead of shallow examples.
Also apply the legacy Danger policy: discouragecreate(in unit tests whenbuildor doubles are enough,
reject.should, skipped/focused specs, wildcard matcher overuse, missing expectations, matcher-less expectations,
and expectations that only prove values are not nil, empty, or zero.
Files:
spec/models/due_date_spec.rbspec/models/student_task_spec.rb
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit Setup For This RepositoryThis repository uses CodeRabbit as an AI pull request reviewer for the Expertiza backend reimplementation. The goal is to improve review quality and preserve project-specific policy without replacing the existing CI runners.
What CodeRabbit Should Do Here
- Review pull requests automatically and re-review on new commits.
- Leave useful inline comments on Rails, API, database, workflow, and test changes.
- Summarize the impact of a pull request clearly for human reviewers.
- Surface policy issues that used to live in Danger-like rules, but start in warning mode instead of blocking merges.
- Read GitHub check results and use them as context when tests or workflows fail.
What CodeRabbit Should Not Replace Here
- GitHub Actions still runs the real work.
main.ymlandTestPR.ymlstill execute RSpec, database setup, coverage generation, and Docker-related jobs.lint.ymlstill remains the source of truth for runnable spellcheck and lint workflows unless those checks are intentionally retired.- CodeRabbit is a reviewer and explainer, not the test runner or deployment engine.
Repository Context
- Language and framework: Ruby 3.4.5 with Rails 8.
- Test stack: RSpec, mainly under
spec/modelsandspec/requests.- API surface: controllers plus request specs, with Swagger documentation checked into
swagger/v1/swagger.yaml.- Database: MySQL-backed Rails app with a large migration history and
db/schema.rbcommitted.- Automation: GitHub Actions workflows in
.github/workflows, including CI, PR test reporting, linting, and Danger.Review Standards For This Codebase
Controllers
- Verify authorization behavior and strong parameters.
- Check HTTP status codes and response payload consistency.
- Flag N+1 queries or heavy database work in request paths.
- Expect request-spec coverage for behavior changes.
- If an endpoint contract changes, expect Swagger documentation to stay in sync.
Models
...
Files:
spec/models/due_date_spec.rbapp/models/concerns/expertiza_constants.rbconfig/routes.rbapp/models/due_date.rbspec/routing/student_tasks_routing_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rbapp/controllers/student_tasks_controller.rbapp/models/assignment_team.rbspec/models/student_task_spec.rbapp/models/student_task.rb
app/models/**/*.rb
⚙️ CodeRabbit configuration file
app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior,
foreign keys, data integrity, and query efficiency.
Files:
app/models/concerns/expertiza_constants.rbapp/models/due_date.rbapp/models/assignment_team.rbapp/models/student_task.rb
config/routes.rb
⚙️ CodeRabbit configuration file
config/routes.rb: Flag duplicate or shadowed routes, surprising non-RESTful patterns, and route changes without matching request specs.
Files:
config/routes.rb
spec/requests/**/*.rb
⚙️ CodeRabbit configuration file
spec/requests/**/*.rb: Check authentication, authorization, invalid input, response payload assertions, and failure cases.
When endpoints change, expect Swagger docs to stay aligned.
Apply the legacy shallow-test rules: each new test should have a meaningful expectation,
avoid commented-out expectations, and do not rely only on page content or non-nil checks.
Files:
spec/requests/api/v1/student_tasks_controller_spec.rb
app/controllers/**/*.rb
⚙️ CodeRabbit configuration file
app/controllers/**/*.rb: Review authorization, strong parameters, HTTP status codes, and response shape consistency.
Flag N+1 queries, missing request-spec coverage, and behavior changes not reflected in Swagger docs.
Files:
app/controllers/student_tasks_controller.rb
🪛 RuboCop (1.87.0)
spec/requests/api/v1/student_tasks_controller_spec.rb
[convention] 163-163: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
[convention] 187-187: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
spec/models/student_task_spec.rb
[convention] 14-14: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 15-15: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 16-16: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 18-18: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 19-19: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 20-20: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 33-33: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 34-34: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 35-35: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 36-36: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 37-37: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 183-183: Line is too long. [192/120]
(Layout/LineLength)
[convention] 184-184: Line is too long. [138/120]
(Layout/LineLength)
[convention] 185-185: Line is too long. [134/120]
(Layout/LineLength)
[convention] 186-186: Line is too long. [192/120]
(Layout/LineLength)
[convention] 187-187: Line is too long. [192/120]
(Layout/LineLength)
[convention] 231-231: Line is too long. [175/120]
(Layout/LineLength)
[convention] 245-245: Line is too long. [178/120]
(Layout/LineLength)
app/models/student_task.rb
[convention] 4-4: Line is too long. [147/120]
(Layout/LineLength)
[convention] 19-37: Assignment Branch Condition size for create_from_participant is too high. [<7, 22, 5> 23.62/20]
(Metrics/AbcSize)
[convention] 26-26: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 27-27: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 28-28: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 29-29: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Line is too long. [147/120]
(Layout/LineLength)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 39-40: Extra blank line detected.
(Layout/EmptyLines)
[convention] 41-41: Expected 1 empty line between method definitions; found 2.
(Layout/EmptyLineBetweenDefs)
[convention] 56-99: Assignment Branch Condition size for get_timeline_data is too high. [<7, 39, 7> 40.24/20]
(Metrics/AbcSize)
[convention] 56-99: Method has too many lines. [33/20]
(Metrics/MethodLength)
[convention] 62-62: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 63-63: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 63-63: Line is too long. [160/120]
(Layout/LineLength)
[convention] 64-64: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 65-65: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 76-76: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 77-77: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 78-78: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 79-79: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 90-90: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 91-91: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 92-92: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 93-93: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 103-118: Assignment Branch Condition size for teamed_students is too high. [<7, 20, 10> 23.43/20]
(Metrics/AbcSize)
[convention] 103-118: Cyclomatic complexity for teamed_students is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 103-118: Perceived complexity for teamed_students is too high. [11/8]
(Metrics/PerceivedComplexity)
[convention] 106-106: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 139-139: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 141-141: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 146-146: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[warning] 154-154: private (on line 131) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.
(Lint/IneffectiveAccessModifier)
[convention] 155-155: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
app/models/student_task.rb (1)
44-47: 🚀 Performance & Scalability | 🏗️ Heavy liftPreload the list endpoint's participant graph.
from_usermaps over a bareAssignmentParticipant.where(user_id: user.id), thencreate_from_participantdereferencesassignment,assignment.course,team,user, due dates, and review state for each row. On/student_tasks/list, that becomes an N+1 query pattern per task.As per coding guidelines, "Flag N+1 queries or heavy database work in request paths."
Source: Coding guidelines
spec/requests/api/v1/student_tasks_controller_spec.rb (1)
106-152: 🗄️ Data Integrity & Integration | ⚡ Quick winMerge the duplicate RSwag declaration for
GET /student_tasks/show/{id}.Lines 106-221 define the same path and verb twice. Depending on how
rswagcollapses duplicate metadata, one block's responses can overwrite the other in generated Swagger output, which makes the published contract unstable. Keep a singlepath '/student_tasks/show/{id}'block and fold the 200/401/403/404 cases into it.As per coding guidelines, "When endpoints change, expect Swagger docs to stay aligned."
Also applies to: 157-221
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9643f2af-7b3d-4fe4-8ad7-565ce5e53463
📒 Files selected for processing (17)
app/controllers/concerns/authorization.rbapp/controllers/grades_controller.rbapp/controllers/participants_controller.rbapp/controllers/student_tasks_controller.rbapp/controllers/teams_participants_controller.rbapp/models/assignment_team.rbapp/models/concerns/expertiza_constants.rbapp/models/due_date.rbapp/models/response.rbapp/models/student_task.rbconfig/routes.rbspec/models/due_date_spec.rbspec/models/response_spec.rbspec/models/student_task_spec.rbspec/requests/api/v1/grades_controller_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rbspec/routing/student_tasks_routing_spec.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- spec/routing/student_tasks_routing_spec.rb
- app/models/due_date.rb
- app/models/assignment_team.rb
- config/routes.rb
- spec/models/due_date_spec.rb
- app/models/concerns/expertiza_constants.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (5)
app/controllers/**/*.rb
⚙️ CodeRabbit configuration file
app/controllers/**/*.rb: Review authorization, strong parameters, HTTP status codes, and response shape consistency.
Flag N+1 queries, missing request-spec coverage, and behavior changes not reflected in Swagger docs.
Files:
app/controllers/participants_controller.rbapp/controllers/teams_participants_controller.rbapp/controllers/concerns/authorization.rbapp/controllers/student_tasks_controller.rbapp/controllers/grades_controller.rb
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit Setup For This RepositoryThis repository uses CodeRabbit as an AI pull request reviewer for the Expertiza backend reimplementation. The goal is to improve review quality and preserve project-specific policy without replacing the existing CI runners.
What CodeRabbit Should Do Here
- Review pull requests automatically and re-review on new commits.
- Leave useful inline comments on Rails, API, database, workflow, and test changes.
- Summarize the impact of a pull request clearly for human reviewers.
- Surface policy issues that used to live in Danger-like rules, but start in warning mode instead of blocking merges.
- Read GitHub check results and use them as context when tests or workflows fail.
What CodeRabbit Should Not Replace Here
- GitHub Actions still runs the real work.
main.ymlandTestPR.ymlstill execute RSpec, database setup, coverage generation, and Docker-related jobs.lint.ymlstill remains the source of truth for runnable spellcheck and lint workflows unless those checks are intentionally retired.- CodeRabbit is a reviewer and explainer, not the test runner or deployment engine.
Repository Context
- Language and framework: Ruby 3.4.5 with Rails 8.
- Test stack: RSpec, mainly under
spec/modelsandspec/requests.- API surface: controllers plus request specs, with Swagger documentation checked into
swagger/v1/swagger.yaml.- Database: MySQL-backed Rails app with a large migration history and
db/schema.rbcommitted.- Automation: GitHub Actions workflows in
.github/workflows, including CI, PR test reporting, linting, and Danger.Review Standards For This Codebase
Controllers
- Verify authorization behavior and strong parameters.
- Check HTTP status codes and response payload consistency.
- Flag N+1 queries or heavy database work in request paths.
- Expect request-spec coverage for behavior changes.
- If an endpoint contract changes, expect Swagger documentation to stay in sync.
Models
...
Files:
app/controllers/participants_controller.rbapp/models/response.rbspec/models/response_spec.rbapp/controllers/teams_participants_controller.rbapp/controllers/concerns/authorization.rbspec/requests/api/v1/grades_controller_spec.rbapp/controllers/student_tasks_controller.rbapp/controllers/grades_controller.rbspec/requests/api/v1/student_tasks_controller_spec.rbspec/models/student_task_spec.rbapp/models/student_task.rb
app/models/**/*.rb
⚙️ CodeRabbit configuration file
app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior,
foreign keys, data integrity, and query efficiency.
Files:
app/models/response.rbapp/models/student_task.rb
spec/models/**/*.rb
⚙️ CodeRabbit configuration file
spec/models/**/*.rb: Check that model specs cover validations, associations, callbacks, and important edge cases
with meaningful assertions instead of shallow examples.
Also apply the legacy Danger policy: discouragecreate(in unit tests whenbuildor doubles are enough,
reject.should, skipped/focused specs, wildcard matcher overuse, missing expectations, matcher-less expectations,
and expectations that only prove values are not nil, empty, or zero.
Files:
spec/models/response_spec.rbspec/models/student_task_spec.rb
spec/requests/**/*.rb
⚙️ CodeRabbit configuration file
spec/requests/**/*.rb: Check authentication, authorization, invalid input, response payload assertions, and failure cases.
When endpoints change, expect Swagger docs to stay aligned.
Apply the legacy shallow-test rules: each new test should have a meaningful expectation,
avoid commented-out expectations, and do not rely only on page content or non-nil checks.
Files:
spec/requests/api/v1/grades_controller_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rb
🪛 ast-grep (0.43.0)
spec/models/student_task_spec.rb
[warning] 136-136: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(courses).to eq(courses.sort), "tasks should be sorted by course first"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 140-140: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(assignments).to eq(assignments.sort), "tasks within same course should be sorted by assignment name"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.87.0)
app/models/response.rb
[convention] 19-19: Redundant self detected.
(Style/RedundantSelf)
spec/requests/api/v1/grades_controller_spec.rb
[convention] 366-366: Line is too long. [125/120]
(Layout/LineLength)
[convention] 388-388: Line is too long. [125/120]
(Layout/LineLength)
[convention] 4-573: Block has too many lines. [448/120]
(Metrics/BlockLength)
app/controllers/grades_controller.rb
[convention] 284-284: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
[convention] 284-284: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 285-285: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 286-286: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 288-288: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 292-292: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
[convention] 292-292: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 293-293: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 294-294: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 380-380: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 380-380: Line is too long. [158/120]
(Layout/LineLength)
[convention] 380-380: Space found before comma.
(Layout/SpaceBeforeComma)
[convention] 394-394: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 394-394: Don't use parentheses around a literal.
(Style/RedundantParentheses)
[convention] 398-398: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 402-402: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 408-408: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 408-408: Line is too long. [153/120]
(Layout/LineLength)
[convention] 417-417: Redundant return detected.
(Style/RedundantReturn)
[convention] 425-425: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 426-426: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 430-430: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 433-433: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 434-434: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 436-436: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 442-442: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
spec/requests/api/v1/student_tasks_controller_spec.rb
[convention] 117-117: Line is too long. [136/120]
(Layout/LineLength)
[convention] 119-119: Line is too long. [168/120]
(Layout/LineLength)
[convention] 120-120: Line is too long. [136/120]
(Layout/LineLength)
[convention] 167-167: Line is too long. [136/120]
(Layout/LineLength)
[convention] 169-169: Line is too long. [168/120]
(Layout/LineLength)
[convention] 170-170: Line is too long. [136/120]
(Layout/LineLength)
[convention] 216-216: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
[convention] 240-240: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
spec/models/student_task_spec.rb
[convention] 14-14: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 15-15: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 16-16: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 18-18: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 19-19: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 20-20: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 33-33: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 34-34: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 35-35: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 36-36: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 37-37: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[warning] 119-119: Useless assignment to variable - deadline_near. Did you mean deadline_far?
(Lint/UselessAssignment)
[warning] 120-120: Useless assignment to variable - deadline_far. Did you mean deadline_near?
(Lint/UselessAssignment)
[convention] 136-136: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 232-232: Line is too long. [192/120]
(Layout/LineLength)
[convention] 233-233: Line is too long. [138/120]
(Layout/LineLength)
[convention] 234-234: Line is too long. [134/120]
(Layout/LineLength)
[convention] 235-235: Line is too long. [192/120]
(Layout/LineLength)
[convention] 236-236: Line is too long. [192/120]
(Layout/LineLength)
[convention] 280-280: Line is too long. [175/120]
(Layout/LineLength)
[convention] 294-294: Line is too long. [178/120]
(Layout/LineLength)
[warning] 341-341: Useless assignment to variable - draft_response.
(Lint/UselessAssignment)
app/models/student_task.rb
[convention] 4-4: Line is too long. [147/120]
(Layout/LineLength)
[convention] 19-37: Assignment Branch Condition size for create_from_participant is too high. [<7, 22, 5> 23.62/20]
(Metrics/AbcSize)
[convention] 26-26: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 27-27: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 28-28: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 29-29: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 30-30: Line is too long. [147/120]
(Layout/LineLength)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 39-40: Extra blank line detected.
(Layout/EmptyLines)
[convention] 44-44: Expected 1 empty line between method definitions; found 2.
(Layout/EmptyLineBetweenDefs)
[convention] 59-110: Assignment Branch Condition size for get_timeline_data is too high. [<7, 40, 7> 41.21/20]
(Metrics/AbcSize)
[convention] 59-110: Method has too many lines. [33/20]
(Metrics/MethodLength)
[convention] 65-65: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 66-66: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 66-66: Line is too long. [160/120]
(Layout/LineLength)
[convention] 67-67: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 68-68: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 86-86: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 87-87: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 88-88: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 89-89: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 101-101: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 102-102: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 103-103: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 104-104: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 114-129: Assignment Branch Condition size for teamed_students is too high. [<7, 20, 10> 23.43/20]
(Metrics/AbcSize)
[convention] 114-129: Cyclomatic complexity for teamed_students is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 114-129: Perceived complexity for teamed_students is too high. [11/8]
(Metrics/PerceivedComplexity)
[convention] 117-117: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 150-150: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 152-152: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 157-157: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[warning] 165-165: private (on line 142) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.
(Lint/IneffectiveAccessModifier)
[convention] 166-166: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/models/student_task.rb (1)
197-204: 📐 Maintainability & Code Quality | 💤 Low value
parse_stage_deadlineis not actually private.The
privatekeyword on line 167 only affects instance methods. Class methods likeself.parse_stage_deadlineremain public. Useprivate_class_methodto restrict access as intended.♻️ Proposed fix
end +private_class_method :parse_stage_deadline endOr move into a
class << selfblock withprivateinside it.Source: Linters/SAST tools
spec/requests/api/v1/student_tasks_controller_spec.rb (2)
106-152: 📐 Maintainability & Code Quality | ⚡ Quick winConsolidate
GET /student_tasks/show/{id}into a single path block.This endpoint is declared twice with overlapping responses, which makes the contract easy to drift and harder to maintain. Keep one
path '/student_tasks/show/{id}'block and group all200/401/403/404responses there.Also applies to: 157-221
232-236: 📐 Maintainability & Code Quality | ⚡ Quick winStrengthen the
/student_tasks/teamsuccess assertion beyondHashtype.The current
200check can still pass for an empty or structurally wrong payload. Add fixture data and assert expected course-key grouping and teammate values to validate contract behavior.As per coding guidelines, "
spec/requests/**/*.rb: each new test should have a meaningful expectation ... and should not rely only on shallow checks."Source: Coding guidelines
app/controllers/teams_participants_controller.rb (1)
50-53: 🩺 Stability & Availability
TeamsParticipant.user_idis present/non-null, so the 500-risk concern is unfounded; consider replacingpluck/include?withexists?for efficiency.
db/schema.rbdefinesteams_participants.user_idasnull: false(with an index),TeamsParticipantvalidatesuser_idpresence, and migration20250427014225_add_user_id_to_teams_participants.rbadds the column.- Current
TeamsParticipant.where(team_id: ...).pluck(:user_id).include?(current_user.id)works but loads all participant ids;TeamsParticipant.exists?(team_id: current_team.id, user_id: current_user.id)avoids materializing the list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 33e0b854-0dc9-4fe7-a116-70c8d11088bb
📒 Files selected for processing (16)
app/controllers/grades_controller.rbapp/controllers/participants_controller.rbapp/controllers/student_tasks_controller.rbapp/controllers/teams_participants_controller.rbapp/models/assignment_team.rbapp/models/concerns/expertiza_constants.rbapp/models/due_date.rbapp/models/response.rbapp/models/student_task.rbconfig/routes.rbspec/models/due_date_spec.rbspec/models/response_spec.rbspec/models/student_task_spec.rbspec/requests/api/v1/grades_controller_spec.rbspec/requests/api/v1/student_tasks_controller_spec.rbspec/routing/student_tasks_routing_spec.rb
🚧 Files skipped from review as they are similar to previous changes (9)
- config/routes.rb
- app/models/concerns/expertiza_constants.rb
- app/models/due_date.rb
- spec/models/response_spec.rb
- spec/models/due_date_spec.rb
- app/controllers/participants_controller.rb
- app/models/assignment_team.rb
- app/models/response.rb
- app/controllers/student_tasks_controller.rb
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit Setup For This RepositoryThis repository uses CodeRabbit as an AI pull request reviewer for the Expertiza backend reimplementation. The goal is to improve review quality and preserve project-specific policy without replacing the existing CI runners.
What CodeRabbit Should Do Here
- Review pull requests automatically and re-review on new commits.
- Leave useful inline comments on Rails, API, database, workflow, and test changes.
- Summarize the impact of a pull request clearly for human reviewers.
- Surface policy issues that used to live in Danger-like rules, but start in warning mode instead of blocking merges.
- Read GitHub check results and use them as context when tests or workflows fail.
What CodeRabbit Should Not Replace Here
- GitHub Actions still runs the real work.
main.ymlandTestPR.ymlstill execute RSpec, database setup, coverage generation, and Docker-related jobs.lint.ymlstill remains the source of truth for runnable spellcheck and lint workflows unless those checks are intentionally retired.- CodeRabbit is a reviewer and explainer, not the test runner or deployment engine.
Repository Context
- Language and framework: Ruby 3.4.5 with Rails 8.
- Test stack: RSpec, mainly under
spec/modelsandspec/requests.- API surface: controllers plus request specs, with Swagger documentation checked into
swagger/v1/swagger.yaml.- Database: MySQL-backed Rails app with a large migration history and
db/schema.rbcommitted.- Automation: GitHub Actions workflows in
.github/workflows, including CI, PR test reporting, linting, and Danger.Review Standards For This Codebase
Controllers
- Verify authorization behavior and strong parameters.
- Check HTTP status codes and response payload consistency.
- Flag N+1 queries or heavy database work in request paths.
- Expect request-spec coverage for behavior changes.
- If an endpoint contract changes, expect Swagger documentation to stay in sync.
Models
...
Files:
spec/routing/student_tasks_routing_spec.rbapp/controllers/teams_participants_controller.rbspec/requests/api/v1/student_tasks_controller_spec.rbapp/controllers/grades_controller.rbapp/models/student_task.rbspec/models/student_task_spec.rbspec/requests/api/v1/grades_controller_spec.rb
app/controllers/**/*.rb
⚙️ CodeRabbit configuration file
app/controllers/**/*.rb: Review authorization, strong parameters, HTTP status codes, and response shape consistency.
Flag N+1 queries, missing request-spec coverage, and behavior changes not reflected in Swagger docs.
Files:
app/controllers/teams_participants_controller.rbapp/controllers/grades_controller.rb
spec/requests/**/*.rb
⚙️ CodeRabbit configuration file
spec/requests/**/*.rb: Check authentication, authorization, invalid input, response payload assertions, and failure cases.
When endpoints change, expect Swagger docs to stay aligned.
Apply the legacy shallow-test rules: each new test should have a meaningful expectation,
avoid commented-out expectations, and do not rely only on page content or non-nil checks.
Files:
spec/requests/api/v1/student_tasks_controller_spec.rbspec/requests/api/v1/grades_controller_spec.rb
app/models/**/*.rb
⚙️ CodeRabbit configuration file
app/models/**/*.rb: Focus on associations, validations, callbacks, transactions, STI and polymorphic behavior,
foreign keys, data integrity, and query efficiency.
Files:
app/models/student_task.rb
spec/models/**/*.rb
⚙️ CodeRabbit configuration file
spec/models/**/*.rb: Check that model specs cover validations, associations, callbacks, and important edge cases
with meaningful assertions instead of shallow examples.
Also apply the legacy Danger policy: discouragecreate(in unit tests whenbuildor doubles are enough,
reject.should, skipped/focused specs, wildcard matcher overuse, missing expectations, matcher-less expectations,
and expectations that only prove values are not nil, empty, or zero.
Files:
spec/models/student_task_spec.rb
🪛 ast-grep (0.43.0)
spec/models/student_task_spec.rb
[warning] 142-142: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(courses).to eq(courses.sort), "tasks should be sorted by course first"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 146-146: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(assignments).to eq(assignments.sort), "tasks within same course should be sorted by assignment name"
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.87.0)
spec/requests/api/v1/student_tasks_controller_spec.rb
[convention] 117-117: Line is too long. [136/120]
(Layout/LineLength)
[convention] 119-119: Line is too long. [168/120]
(Layout/LineLength)
[convention] 120-120: Line is too long. [136/120]
(Layout/LineLength)
[convention] 167-167: Line is too long. [136/120]
(Layout/LineLength)
[convention] 169-169: Line is too long. [168/120]
(Layout/LineLength)
[convention] 170-170: Line is too long. [136/120]
(Layout/LineLength)
[convention] 216-216: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
[convention] 240-240: Do not use strings for word-like symbol literals.
(Style/SymbolLiteral)
app/controllers/grades_controller.rb
[convention] 21-21: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 22-22: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 27-27: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 34-34: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 287-287: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
[convention] 287-287: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 288-288: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 289-289: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 291-291: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 295-295: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
[convention] 295-295: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 296-296: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 297-297: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 383-383: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 383-383: Line is too long. [158/120]
(Layout/LineLength)
[convention] 383-383: Space found before comma.
(Layout/SpaceBeforeComma)
[convention] 392-392: Align .joins with AssignmentQuestionnaire on line 391.
(Layout/MultilineMethodCallIndentation)
[convention] 393-393: Align .where with AssignmentQuestionnaire on line 391.
(Layout/MultilineMethodCallIndentation)
[convention] 394-394: Align .where with AssignmentQuestionnaire on line 391.
(Layout/MultilineMethodCallIndentation)
[convention] 395-395: Align .first with AssignmentQuestionnaire on line 391.
(Layout/MultilineMethodCallIndentation)
[convention] 405-405: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 405-405: Avoid using {...} for multi-line blocks.
(Style/BlockDelimiters)
[convention] 406-406: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 413-413: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 413-413: Don't use parentheses around a literal.
(Style/RedundantParentheses)
[convention] 417-417: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 417-417: Avoid using {...} for multi-line blocks.
(Style/BlockDelimiters)
[convention] 418-418: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 423-423: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 429-429: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 429-429: Avoid using {...} for multi-line blocks.
(Style/BlockDelimiters)
[convention] 430-430: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 443-443: Redundant return detected.
(Style/RedundantReturn)
[convention] 451-451: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 452-452: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 456-456: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 459-459: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 460-460: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 462-462: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 468-468: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
app/models/student_task.rb
[convention] 4-4: Line is too long. [162/120]
(Layout/LineLength)
[convention] 20-47: Assignment Branch Condition size for create_from_participant is too high. [<9, 23, 8> 25.96/20]
(Metrics/AbcSize)
[convention] 20-47: Cyclomatic complexity for create_from_participant is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 20-47: Perceived complexity for create_from_participant is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 35-35: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 36-36: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 37-37: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 38-38: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 39-39: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 39-39: Line is too long. [147/120]
(Layout/LineLength)
[convention] 41-41: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 42-42: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 49-50: Extra blank line detected.
(Layout/EmptyLines)
[convention] 54-54: Expected 1 empty line between method definitions; found 2.
(Layout/EmptyLineBetweenDefs)
[convention] 84-135: Assignment Branch Condition size for get_timeline_data is too high. [<7, 40, 7> 41.21/20]
(Metrics/AbcSize)
[convention] 84-135: Method has too many lines. [33/20]
(Metrics/MethodLength)
[convention] 90-90: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 91-91: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 91-91: Line is too long. [160/120]
(Layout/LineLength)
[convention] 92-92: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 93-93: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 111-111: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 112-112: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 113-113: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 114-114: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 126-126: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 127-127: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 128-128: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 129-129: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 139-154: Assignment Branch Condition size for teamed_students is too high. [<7, 20, 10> 23.43/20]
(Metrics/AbcSize)
[convention] 139-154: Cyclomatic complexity for teamed_students is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 139-154: Perceived complexity for teamed_students is too high. [11/8]
(Metrics/PerceivedComplexity)
[convention] 142-142: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 175-175: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 177-177: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 183-183: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
[convention] 191-191: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
[warning] 198-198: private (on line 167) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.
(Lint/IneffectiveAccessModifier)
[convention] 199-199: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
spec/models/student_task_spec.rb
[convention] 14-14: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 15-15: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 16-16: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 18-18: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 19-19: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 20-20: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 32-32: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 33-33: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 34-34: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 35-35: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 36-36: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[convention] 37-37: Align the keys of a hash literal if they span more than one line.
(Layout/HashAlignment)
[warning] 119-119: Useless assignment to variable - deadline_near. Did you mean deadline_far?
(Lint/UselessAssignment)
[warning] 120-120: Useless assignment to variable - deadline_far. Did you mean deadline_near?
(Lint/UselessAssignment)
[convention] 142-142: Unnecessary spacing detected.
(Layout/ExtraSpacing)
[convention] 238-238: Line is too long. [192/120]
(Layout/LineLength)
[convention] 239-239: Line is too long. [138/120]
(Layout/LineLength)
[convention] 240-240: Line is too long. [134/120]
(Layout/LineLength)
[convention] 241-241: Line is too long. [192/120]
(Layout/LineLength)
[convention] 242-242: Line is too long. [192/120]
(Layout/LineLength)
[convention] 286-286: Line is too long. [175/120]
(Layout/LineLength)
[convention] 300-300: Line is too long. [178/120]
(Layout/LineLength)
[warning] 347-347: Useless assignment to variable - draft_response.
(Lint/UselessAssignment)
spec/requests/api/v1/grades_controller_spec.rb
[convention] 366-366: Line is too long. [125/120]
(Layout/LineLength)
[convention] 388-388: Line is too long. [125/120]
(Layout/LineLength)
[convention] 4-573: Block has too many lines. [448/120]
(Metrics/BlockLength)
🔇 Additional comments (18)
app/models/student_task.rb (4)
4-47: LGTM!
54-73: LGTM!
104-135: LGTM!
137-195: LGTM!spec/models/student_task_spec.rb (5)
7-27: LGTM!Also applies to: 29-64, 66-106
108-149: LGTM!
151-232: LGTM!
234-295: LGTM!
297-427: LGTM!spec/routing/student_tasks_routing_spec.rb (1)
31-41: LGTM!spec/requests/api/v1/grades_controller_spec.rb (4)
564-571: Pin the expected status to404rather than accepting both.This is a
Grades APIspec file, but the test exercisesstudent_tasks/show. The controller returns404when the participant is not found (id 999999) and403only when a participant exists but belongs to a different user. Usingbe_in([404, 403])masks regressions if the controller behavior changes. As per coding guidelines, request specs should have meaningful expectations and avoid shallow assertions.Source: Coding guidelines
348-404: LGTM!
454-460: LGTM!
462-558: LGTM!app/controllers/grades_controller.rb (4)
21-36: LGTM!
279-298: LGTM!
344-397: LGTM!
446-473: LGTM!
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
1 similar comment
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
|
🚨 RSpec Tests Report |
1 participant
This PR implements the StudentTask backend for the reimplementation, mirroring the functionality of the old Expertiza student_task_controller. It introduces student task listing, timeline data, teammate discovery, and revision tracking logic.
StudentTasksController
Grades Controller
StudentTask model
DueDate model
AssignmentParticipant model
AssignmentTeam model
Response
ExpertizaConstants
Tests Added:
Danger Policy and Code Review Quality Assessment
Pull Request Scope Issues
CRITICAL: Excessive PR Size & Multi-Concern Scope
Test Coverage & Quality Patterns
Shallow RSpec Patterns
.allow().and_return()mocking without integration verification (31 RSpec examples with 5+ stub chains per spec)StudentTask#get_timeline_data,#teamed_students, and#revise?are heavily mocked in tests rather than exercising real database queries and associations (e.g.,allow(ReviewResponseMap).to receive(:where).and_return([]))DueDate,SignedUpTeam,ReviewResponseMap, andFeedbackResponseMapthroughout student_task_spec.rb may hide integration issuesDueDate,Response, andFeedbackResponseMaprecords to verify real associations workInconsistent Test Depth
teamed_studentstest creates realAssignmentTeamand team members (better), butget_timeline_dataandrevise?are 100% mockedGradesControllertests expanded by 173 lines but largely assert response shape without verifying the underlying data aggregation logicAuthorization & Security
Positive: User ID Validation
StudentTasksController#showcorrectly checksparticipant.user_id != current_user.idand returns403TeamsParticipantsController#list_participantsadds proper authorization guard for non-TA usersConcern: Authorization Added to Wrong Place
TeamsParticipantsController#action_allowed?now handles bothupdate_dutyandlist_participants, but authorization logic forlist_participantsis scattered betweenaction_allowed?and the action body; this mixes concerns and makes the logic harder to followCode Duplication & Coupling
Potential Duplication
StudentTask#get_timeline_databuilds a timeline by merging due dates, peer reviews, and author feedbackGradesController#get_heatgrid_data_foralso builds score-view timelines with similar logic (sorting, round grouping)Response#questionnaireadded fallback logic for single-round assignments, but this same logic may be needed elsewhere ifget_heatgrid_data_forhandles single-round assignments differentlyTight Coupling
StudentTask#create_from_participantdepends onDueDate.current_stage_for,DueDate.next_due_date,SignedUpTeam,assignment.course, and timezone preference lookup — a many-dependency initializer makes testing and refactoring fragileStudentTask#get_timeline_datadirectly queriesReviewResponseMapandFeedbackResponseMap; adding a new timeline source requires modifying this methodTesting Coverage Gaps
StudentTasksController#showcorrectly populatesdue_datesfrom real DueDate and Response recordsGradesController#inject_section_headersis a new private helper with no dedicated spec; tested only indirectly viaget_heatgrid_data_forAssignmentTeam#submitted_filesadded as a stub returning[]with no tests explaining its intended use or future implementationResponse#questionnairefallback behavior tested, but no test covers the case where both lookups fail andnilis returnedConfiguration & Schema Changes
get :view, addingget 'show/:id'andget :team), but these changes are bundled with large logic changesCode Quality Issues
Complex Initialization Logic
StudentTask#initializenow accepts 10+ fields via a hash (assignment, course, current_stage, participant, stage_deadline, topic, permission_granted, due_dates, active_round, revise, not_started); large initializer signature is hard to maintainMulti-Step Dependency Resolution
StudentTask.create_from_participantspans ~20 lines with inline calls toDueDate.next_due_date,SignedUpTeam.find_by,project_topic, timezone lookup, and then assignment of computedrevise?andnot_started?flags; this would benefit from smaller private helper methodsPrivate Method Inconsistency
StudentTask.parse_stage_deadlineis a class method but defined in the private section; should clarify intent (used by class method, so could be public or moved outside private block)Summary of Violations Against Danger Policy
putsfound in other specs)Recommendations for Revision
StudentTask#get_timeline_data,#teamed_students,#revise?, and#not_started?GradesController#get_heatgrid_data_forandStudentTask#get_timeline_datashould share logic or remain independentcreate_from_participantinto smaller methods for each responsibility (resolve topic, determine stage, compute deadline)