Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolving Issue 2191 - log_submit now validates for date #2251

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ogometz
Copy link

@ogometz ogometz commented Dec 8, 2024

The new additions add additional validation tot he log_submit functionality for assessments. Now, the dates are checked for validity before submitting.

Description

This pull requests has modified the log_submit method in the AssessmentsController to ensure that no assessments are being submitted without proper date validation. The code checks if the current time is before the start time of the assessment, or if the current time is beyond the end time of the assessment. If either condition is met, and error is thrown. This pull request addresses pull request number 2191. This pull request suggests on potential solution: " One possibly elegant way to do this is to create a new Submission object (but never save it). If the submission would be allowed? then it's ok to record the log entry, otherwise it should be refused." This option was initially explored. However, given the brevity of the fix, it was determined that creating a new object would likely serve to confuse and further complicate the functionality. And since the original log_submit function already has certain validations and checks, adding the date check only made sense.

Motivation and Context

The motivation for this pull request is to address an issue brought up, linked here: #2191
With these changes, the proper checks will be ran for proper code quality and coverage.

How Has This Been Tested?

Accompanied by these code changes are several new tests, which have been added to the AssessmentController test suite found here: spec/controllers/assessments_controller_spec.rb. Three new tests have been added. The first executes the log_submit functionality with validated dates. The second attempts to do the same but with the submittion date before the start date of the assessment. Lastly, the third test seeks to do the same but with a submission date after the end date. As expected, the original code causes the first test to pass, but the last two to fail. With the code change, all three tests pass.

Types of changes

  • [ X ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ X ] I have run rubocop and erblint for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Copy link
Contributor

coderabbitai bot commented Dec 8, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes in this pull request introduce validation checks for submission dates in the log_submit method of the AssessmentHandin module, ensuring that submissions occur within the assessment's defined time frame. If the current time is outside the assessment period, an error message is rendered. Additionally, error handling in the local_submit method is improved to provide appropriate feedback on submission failures. A new test suite for the log_submit action is also added, covering scenarios for valid submissions and submissions made outside the assessment period.

Changes

File Change Summary
app/controllers/assessment/handin.rb Added validation to log_submit method to check submission dates against assessment period; modified error handling in local_submit to set flash[:error].
spec/controllers/assessments_controller_spec.rb Introduced a new test suite for log_submit with contexts for valid submissions, submissions before the start date, and submissions after the end date.

Possibly related PRs

  • Added confirmation prompt for late days #2249: The changes in this PR involve modifications to the submission form related to late submissions, which may interact with the validation logic for submission dates introduced in the main PR.

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
app/controllers/assessment/handin.rb (1)

273-278: Consider enhancing the date validation error message and reusability.

While the date validation logic is correct, consider these improvements:

  1. Extract the date validation into a reusable method as this check might be needed elsewhere
  2. Provide more specific error messages to indicate whether it's too early or too late
-    # Validate assessment dates
-    current_time = Time.current
-    if current_time < @assessment.start_at || current_time > @assessment.end_at
-      err = "ERROR: Submissions are not allowed outside the assessment period"
-      render(plain: err, status: :bad_request) && return
-    end
+    # Extract to a method for reusability
+    def validate_submission_time
+      current_time = Time.current
+      if current_time < @assessment.start_at
+        "ERROR: Assessment hasn't started yet. Submissions begin at #{@assessment.start_at}"
+      elsif current_time > @assessment.end_at
+        "ERROR: Assessment has ended. Submissions closed at #{@assessment.end_at}"
+      end
+    end
+
+    # Use in log_submit
+    if (err = validate_submission_time)
+      render(plain: err, status: :bad_request) && return
+    end
spec/controllers/assessments_controller_spec.rb (2)

216-309: Test suite looks comprehensive, but could benefit from some cleanup.

The test coverage is thorough and well-structured. However, there are some opportunities for improvement:

  1. Extract common setup code to reduce duplication:
+ let(:base_params) do
+   {
+     course_name: course_hash[:course].name,
+     user: course_hash[:students_cud].first.email,
+     result: "test result"
+   }
+ end

+ def create_test_assessment(name:, start_at:, due_at:, end_at:)
+   assessment_path = Rails.root.join("courses/#{course_hash[:course].name}/#{name}")
+   FileUtils.mkdir_p(assessment_path)
+   FileUtils.mkdir_p("#{assessment_path}/handin")
+   FactoryBot.create(:assessment,
+     name: name,
+     course: course_hash[:course],
+     start_at: start_at,
+     due_at: due_at,
+     end_at: end_at,
+     allow_unofficial: true)
+ end
  1. Use the extracted helpers in the tests:
- let!(:valid_assessment) do
-   assessment_path = Rails.root.join("courses/#{course_hash[:course].name}/assessment_valid")
-   FileUtils.mkdir_p(assessment_path)
-   FileUtils.mkdir_p("#{assessment_path}/handin")
-   assessment = FactoryBot.create(:assessment,
-     name: "assessment_valid",
-     course: course_hash[:course],
-     start_at: 2.days.ago,
-     due_at: 1.day.from_now,
-     end_at: 2.days.from_now,
-     allow_unofficial: true)
-   assessment.save!
-   assessment
- end
+ let!(:valid_assessment) do
+   create_test_assessment(
+     name: "assessment_valid",
+     start_at: 2.days.ago,
+     due_at: 1.day.from_now,
+     end_at: 2.days.from_now
+   )
+ end
🧰 Tools
🪛 rubocop (1.68.0)

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 233-233: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 247-247: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 251-251: Line is too long. [105/100]

(Layout/LineLength)


[convention] 255-260: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 264-264: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 275-275: Line is too long. [103/100]

(Layout/LineLength)


[convention] 286-291: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 295-295: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 306-306: Line is too long. [103/100]

(Layout/LineLength)


223-229: Fix alignment of method arguments per rubocop.

-        assessment = FactoryBot.create(:assessment,
-                                        name: "assessment_valid",
-                                        course: course_hash[:course],
-                                        start_at: 2.days.ago,
-                                        due_at: 1.day.from_now,
-                                        end_at: 2.days.from_now,
-                                        allow_unofficial: true)
+        assessment = FactoryBot.create(
+          :assessment,
+          name: "assessment_valid",
+          course: course_hash[:course],
+          start_at: 2.days.ago,
+          due_at: 1.day.from_now,
+          end_at: 2.days.from_now,
+          allow_unofficial: true
+        )

Also applies to: 255-260, 286-291

🧰 Tools
🪛 rubocop (1.68.0)

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fe44b53 and c410098.

📒 Files selected for processing (2)
  • app/controllers/assessment/handin.rb (1 hunks)
  • spec/controllers/assessments_controller_spec.rb (1 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
spec/controllers/assessments_controller_spec.rb

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 233-233: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 247-247: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 251-251: Line is too long. [105/100]

(Layout/LineLength)


[convention] 255-260: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 264-264: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 275-275: Line is too long. [103/100]

(Layout/LineLength)


[convention] 286-291: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 295-295: Trailing whitespace detected.

(Layout/TrailingWhitespace)


[convention] 306-306: Line is too long. [103/100]

(Layout/LineLength)


[convention] 310-310: Final newline missing.

(Layout/TrailingEmptyLines)

🔇 Additional comments (2)
spec/controllers/assessments_controller_spec.rb (2)

233-245: LGTM! Valid submission test case is well implemented.

The test properly verifies that submissions within the assessment period are accepted.

🧰 Tools
🪛 rubocop (1.68.0)

[convention] 233-233: Trailing whitespace detected.

(Layout/TrailingWhitespace)


265-277: LGTM! Invalid date test cases are comprehensive.

The tests properly verify that submissions are rejected both before the start date and after the end date.

Also applies to: 296-308

🧰 Tools
🪛 rubocop (1.68.0)

[convention] 275-275: Line is too long. [103/100]

(Layout/LineLength)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
spec/controllers/assessments_controller_spec.rb (4)

223-229: Style: Improve argument alignment for better readability

The method arguments should be aligned consistently.

-        assessment = FactoryBot.create(:assessment,
-                                        name: "assessment_valid",
-                                        course: course_hash[:course],
-                                        start_at: 2.days.ago,
-                                        due_at: 1.day.from_now,
-                                        end_at: 2.days.from_now,
-                                        allow_unofficial: true)
+        assessment = FactoryBot.create(
+          :assessment,
+          name: "assessment_valid",
+          course: course_hash[:course],
+          start_at: 2.days.ago,
+          due_at: 1.day.from_now,
+          end_at: 2.days.from_now,
+          allow_unofficial: true
+        )
🧰 Tools
🪛 rubocop (1.68.0)

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


242-242: Style: Use keyword arguments for HTTP method calls

Update the HTTP method call to use keyword arguments as per Rails conventions.

-        post :log_submit, params
+        post :log_submit, params: params
🧰 Tools
🪛 rubocop (1.68.0)

[convention] 242-242: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


251-254: Style: Improve string concatenation in path construction

The path construction can be simplified using string interpolation.

-        assessment_path = Rails.root.join(
-          "courses/#{course_hash[:course].name}/assessment_invalid_date"
-          )
+        assessment_path = Rails.root.join("courses", course_hash[:course].name, "assessment_invalid_date")
🧰 Tools
🪛 rubocop (1.68.0)

[convention] 253-253: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


286-287: Fix: Remove unnecessary string concatenation and line break

The path string is unnecessarily split across lines.

-        assessment_path = Rails.root.join("courses/#{course_hash[:course].name}
-        /assessment_expired")
+        assessment_path = Rails.root.join("courses", course_hash[:course].name, "assessment_expired")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c410098 and e9c5ad4.

📒 Files selected for processing (1)
  • spec/controllers/assessments_controller_spec.rb (1 hunks)
🧰 Additional context used
🪛 rubocop (1.68.0)
spec/controllers/assessments_controller_spec.rb

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 242-242: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 253-253: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 257-262: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 275-275: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 279-279: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 291-296: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 309-309: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)

🔇 Additional comments (2)
spec/controllers/assessments_controller_spec.rb (2)

216-316: LGTM! Comprehensive test coverage for date validation

The test suite thoroughly covers all three scenarios for assessment submission timing:

  1. Valid submission within the assessment period
  2. Rejection of early submissions (before start date)
  3. Rejection of late submissions (after end date)

The test cases are well-structured with appropriate assertions and error messages.

🧰 Tools
🪛 rubocop (1.68.0)

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 242-242: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 253-253: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 257-262: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 275-275: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 279-279: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 291-296: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 309-309: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


216-316: Verify: Test coverage matches PR objectives

The test suite successfully addresses Issue 2191 by verifying that:

  1. The log_submit action validates submission dates
  2. Submissions are only accepted within the assessment period
  3. Appropriate error messages are returned for invalid submission times
✅ Verification successful

Test coverage correctly validates date constraints

The test suite accurately reflects the implementation in app/controllers/assessment/handin.rb, where:

  • Date validation checks if current_time is within start_at and end_at
  • The exact error message "Submissions are not allowed outside the assessment period" is used
  • All test scenarios (valid period, before start, after end) align with the implementation logic
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the implementation matches the test expectations

# Check for the error message string in the implementation
rg -l "Submissions are not allowed outside the assessment period" app/

Length of output: 109


Script:

#!/bin/bash
# Let's check the implementation in handin.rb to verify the date validation logic
rg -A 10 "Submissions are not allowed outside the assessment period" app/controllers/assessment/handin.rb

Length of output: 561


Script:

#!/bin/bash
# Let's check the date validation logic implementation
rg -B 10 "Submissions are not allowed outside the assessment period" app/controllers/assessment/handin.rb

Length of output: 643

🧰 Tools
🪛 rubocop (1.68.0)

[convention] 224-229: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 242-242: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 253-253: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 257-262: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 275-275: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)


[convention] 279-279: Indent ) to column 8 (not 10)

(Layout/ClosingParenthesisIndentation)


[convention] 291-296: Align the arguments of a method call if they span more than one line.

(Layout/ArgumentAlignment)


[convention] 309-309: Use keyword arguments instead of positional arguments for http call: post.

(Rails/HttpPositionalArguments)

@Nita242004
Copy link
Contributor

Effectively enhances the log_submit function by adding robust date validation, improving the system's reliability and preventing errors from invalid inputs. Would be beneficial to include comprehensive tests for edge cases like leap years and diverse time zones to ensure the feature works universally.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
spec/controllers/assessments_controller_spec.rb (3)

217-247: Test coverage for valid submission looks good!

The test properly validates the happy path with appropriate assertions. However, there's a minor style issue with the closing brace placement in the assessment creation.

Consider adjusting the brace placement:

        assessment = FactoryBot.create(
          :assessment,
          name: "assessment_valid",
          course: course_hash[:course],
          start_at: 2.days.ago,
          due_at: 1.day.from_now,
          end_at: 2.days.from_now,
-         allow_unofficial: true)
+         allow_unofficial: true
+        )
🧰 Tools
🪛 rubocop (1.69.1)

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


249-249: Consider a more descriptive context name

The current context name could be more specific about the timing scenario being tested.

-    context "when the submission is before the start date" do
+    context "when attempting to submit before the assessment start date" do

216-319: Consider adding edge case tests

As suggested in the PR comments, consider adding tests for the following scenarios:

  • Submission exactly at start time
  • Submission exactly at end time
  • Submissions across different time zones
  • Submissions during DST transitions

Would you like assistance in implementing these additional test cases?

🧰 Tools
🪛 rubocop (1.69.1)

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 299-299: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9c5ad4 and 13a1cee.

📒 Files selected for processing (1)
  • spec/controllers/assessments_controller_spec.rb (1 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/controllers/assessments_controller_spec.rb

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 299-299: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)

🔇 Additional comments (1)
spec/controllers/assessments_controller_spec.rb (1)

216-216: Well-structured test organization!

The test suite follows RSpec best practices with proper describe/context blocks for different scenarios.

spec/controllers/assessments_controller_spec.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
spec/controllers/assessments_controller_spec.rb (4)

223-231: Fix the closing brace style

The closing brace should be on a new line after the last argument.

         assessment = FactoryBot.create(
           :assessment,
           name: "assessment_valid",
           course: course_hash[:course],
           start_at: 2.days.ago,
           due_at: 1.day.from_now,
           end_at: 2.days.from_now,
-          allow_unofficial: true)
+          allow_unofficial: true
+        )
🧰 Tools
🪛 rubocop (1.69.1)

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


257-266: Fix the closing brace style

The closing brace should be on a new line after the last argument.

         assessment = FactoryBot.create(
           :assessment,
           name: "assessment_invalid_date",
           course: course_hash[:course],
           start_at: 1.day.from_now,
           due_at: 2.days.from_now,
           end_at: 3.days.from_now,
-          allow_unofficial: true)
+          allow_unofficial: true
+        )
🧰 Tools
🪛 rubocop (1.69.1)

[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


293-302: Fix the closing brace style

The closing brace should be on a new line after the last argument.

         assessment = FactoryBot.create(
           :assessment,
           name: "assessment_expired",
           course: course_hash[:course],
           start_at: 3.days.ago,
           due_at: 2.days.ago,
           end_at: 1.day.ago,
-          allow_unofficial: true)
+          allow_unofficial: true
+        )
🧰 Tools
🪛 rubocop (1.69.1)

[convention] 300-300: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


216-320: Consider adding edge case tests

The test suite provides good coverage of the main scenarios. However, as suggested in the PR comments by Nita242004, consider adding tests for edge cases:

  • Submissions exactly at start_at and end_at times
  • Submissions across different time zones
  • Date transitions (e.g., DST changes, leap years)

Would you like me to help generate additional test cases for these scenarios?

🧰 Tools
🪛 rubocop (1.69.1)

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 300-300: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13a1cee and d9f82bf.

📒 Files selected for processing (1)
  • spec/controllers/assessments_controller_spec.rb (1 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/controllers/assessments_controller_spec.rb

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


[convention] 300-300: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)

🔇 Additional comments (3)
spec/controllers/assessments_controller_spec.rb (3)

217-246: LGTM! Well-structured test for valid submission

The test case effectively validates the happy path scenario with appropriate date ranges and assertions.

🧰 Tools
🪛 rubocop (1.69.1)

[convention] 230-230: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


249-282: LGTM! Comprehensive test for early submission

The test case effectively validates the early submission scenario with appropriate future dates and error assertions.

🧰 Tools
🪛 rubocop (1.69.1)

[convention] 264-264: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)


285-318: LGTM! Thorough test for late submission

The test case effectively validates the late submission scenario with appropriate past dates and error assertions.

🧰 Tools
🪛 rubocop (1.69.1)

[convention] 300-300: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

(Layout/MultilineMethodCallBraceLayout)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
spec/controllers/assessments_controller_spec.rb (3)

253-255: Fix the assessment path construction

The path construction could be simplified and made more robust.

-        assessment_path = Rails.root.join(
-          "courses/#{course_hash[:course].name}/assessment_invalid_date"
-        )
+        assessment_path = Rails.root.join("courses", course_hash[:course].name, "assessment_invalid_date")

250-285: Consider reducing test setup duplication

The setup code is duplicated across contexts. Consider using shared contexts or let blocks for common setup.

Example refactor:

shared_context "assessment setup" do |assessment_name, dates|
  let!(:assessment) do
    assessment_path = Rails.root.join("courses", course_hash[:course].name, assessment_name)
    FileUtils.mkdir_p(assessment_path)
    FileUtils.mkdir_p(File.join(assessment_path, "handin"))
    
    FactoryBot.create(
      :assessment,
      name: assessment_name,
      course: course_hash[:course],
      start_at: dates[:start_at],
      due_at: dates[:due_at],
      end_at: dates[:end_at],
      allow_unofficial: true
    ).tap(&:save!)
  end
end

216-323: Consider adding edge case tests

While the current test coverage is good, consider adding tests for:

  1. Submission exactly at start time
  2. Submission exactly at end time
  3. Submission with different time zones
  4. Submission near DST transitions

These edge cases would ensure robust date validation across all scenarios.

Example edge case test:

context "when submitting exactly at start time" do
  let!(:edge_case_assessment) do
    start_time = Time.current.beginning_of_minute
    FactoryBot.create(
      :assessment,
      start_at: start_time,
      due_at: 1.day.from_now,
      end_at: 2.days.from_now,
      # ... other attributes
    )
  end

  it "allows submission at exact start time" do
    travel_to edge_case_assessment.start_at do
      # test submission
    end
  end
end
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9f82bf and 99e408f.

📒 Files selected for processing (1)
  • spec/controllers/assessments_controller_spec.rb (1 hunks)
🔇 Additional comments (2)
spec/controllers/assessments_controller_spec.rb (2)

216-248: LGTM! Well-structured test setup for valid submission case

The test setup is clean and follows best practices:

  • Clear context description
  • Appropriate use of FactoryBot
  • Well-defined date ranges using Rails time helpers
  • Proper isolation of test data

290-292: Fix the assessment path construction

The path construction issue is repeated here and should be fixed similarly.

-        assessment_path = Rails.root.join(
-          "courses/#{course_hash[:course].name}/assessment_expired"
-        )
+        assessment_path = Rails.root.join("courses", course_hash[:course].name, "assessment_expired")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants