Refactor Assessment name rules, remove config file requirement

[20wildmanj]

Summary

Summary generated by Reviewpad on 06 Nov 23 22:44 UTC

This pull request includes the following changes:

1. The file homework02-illegal-assessment-name.tar has been deleted.
2. In the file app/views/assessments/_edit_basic.html.erb, a heading <h4>Assessment Config</h4> has been added, the help_text of the f.file_field has been modified, and a <div> containing a link to reload the assessment config has been added. These changes enhance the file by providing additional information and functionality related to the assessment configuration.
3. The file spec/controllers/assessments_controller_spec.rb has been updated with test cases for creating assessments, handling invalid display names, and testing the export and import roundtrip feature. Various methods in the assessments_controller.rb file have been modified to handle assessment name validation, file cleanup, error handling, and unique names and paths for assessment config files.
4. A new file named autograde.tar has been added in the randomlab directory. The file is a binary file and its contents differ from the previous version.
5. A new file randomlab.rb has been added in the randomlab directory. The file defines the module Randomlab and includes AssessmentBase. It also contains a method assessmentInitialize and initializes the instance variable @problems with an empty array.
6. A new file autograde-Makefile has been added in the randomlab directory. The Makefile extracts the contents of the autograde.tar file, changes the directory to random, and executes a script called driver.sh.
7. The file randomlab/randomlab.rb has been modified to include a require statement for "AssessmentBase.rb" and define the Randomlab module with a new method assessmentInitialize. The method calls the super method from AssessmentBase with two arguments.
8. A new file driver.sh has been added in the randomlab/random directory. The file is a bash script that generates random numbers and prints them in a JSON format.
9. The file app/controllers/assessments_controller.rb has been modified to update error messages, handle file cleanup, load and reload config files, and validate assessment names and special characters.
10. The file app/models/user.rb has been modified to remove the return keyword before authentication&.user if authentication&.user in several methods, ensuring that the methods always return the value of authentication.user if it exists.
11. The file app/helpers/assessment_autograde_core.rb has been modified to raise an AutogradeError for missing problems and handle the requirement of the assessment config file path.
12. The file app/models/course.rb has been modified to update the validation syntax for validates :requested_at and validates_associated :oauth_application.
13. The file app/controllers/assessment/autograde.rb has been modified to dynamically require configuration files based on assessment and course names.

These are the main changes across the diff. Let me know if you need further assistance or if there are any specific areas you would like to focus on.

Description

• Refactor Assessment naming rules so that assessment names can contain hyphens and underscores
  • previously, only lowercase alphabetical characters and numbers were allowed
  • rules now essentially match Ruby Identifier rules, and also allows hyphens
• Remove requirement for config files to be included in importing assessments
  • they now will be automatically generated when no config file is found
• No longer require module names to match the assessment name during importing
  • automatically rename module identifier to be "#{sanitized_name}_#{id}".camelize, so that assessment config files are unique among all assessments
• Backwards compatibility: add code to migrate existing config files when config files are executed, while preserving the original source file, to prevent assessments from breaking if this commit is rolled back
• Relax constraints on display name during assessment creation in assessments/new
  • previously, the display name here had the same naming rules as assessment names, which is inconsistent with editing display names, where there are no restrictions
• update and add tests for naming requirements
• add documentation outlining naming requirements, as well as a guide to troubleshooting importing errors

Motivation and Context

• Address the needs of 15-213 and 15-122, while making naming rules more transparent and make more sense
• Streamline assessment import by removing config file existence and naming requirements

Closes #1943
Closes #1393

How Has This Been Tested?

This PR requires Autolab to be restarted in order to work

New assessment:

• Go to a course->"Install Assessment"->"Assessment Builder"
• Create assessments with display names as follows:
  • valid:
    • .hello, ...test, test-test, test_test, TEST1-test, TEST2!, test111!@#$%^&*()
  • invalid:
    • ../, ---, ____, ////, //---__44, _-4
• See that assessment config file is created in courses/$course_name/$assessment_name/ and that config file has module name of "#{sanitized_name}_#{id}".camelize

Existing assessment:

• Ensure that viewing assessment still works
• Run an autograded assessment, see that autograding works (i.e. ensure that for an assessment that returns a non-zero score, it should still return a non-zero score)
• See that in assessmentConfig folder, there should exist "course-assessment.rb" and "course-assessment-${id}.rb" files, and "course-assessment-${id}.rb" should have a module name of "${assessment name}${id}"

Import Assessment:

• from the documentation, those with valid names should be created successfully, those with invalid names should fail to create (try also names such as "../", see that creation fails)

• tar import: Import the tar file below, see that it successfully imports, generates a new assessment config file in the assessment folder and it also generated in the assessmentConfig folder
  bareminimum.tar.zip

• Try importing the tar file below, see that it fails to import
  badasmt.tar.zip
  

• Take an existing assessment, copy the folder, rename the folder / assessment.yml / name field in assessment.yml to a name that is valid, see that it imports successfully

• Take an existing assessment, copy the folder, rename the folder / assessment.yml / name field in assessment.yml to a name that is invalid, see that it cannot be imported
  

• run docs locally, see that documentation looks good

Types of changes

• 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 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

[coderabbitai]

Walkthrough

Walkthrough

Walkthrough

The changes in this pull request primarily focus on improving the handling and validation of assessment names and configuration files in an Autolab application. The changes allow for more flexibility in assessment names, including the use of hyphens and underscores. The configuration files for each assessment are now uniquely named, and the code handles exceptions when loading these files. The user interface and documentation have been updated to reflect these changes.

Changes

File Path	Change Summary
app/controllers/assessment/autograde.rb	Introduced a conditional statement to determine the path of the required configuration file based on the value of @assessment.use_unique_module_name.
app/controllers/assessments_controller.rb	Updated to enforce valid assessment names, handle exceptions when reloading the course config file, use a unique config file path for each assessment, and delete unique config files when destroying an assessment.
app/controllers/scoreboards_controller.rb	Removed an empty line in the show method.
app/helpers/assessment_autograde_core.rb	Updated the saveAutograde and extend_config_module methods to handle unique module names and raise exceptions if the assessment config file cannot be found.
app/models/assessment.rb	Introduced new methods for generating unique file paths and module names based on the assessment's name and ID. Updated the logic for constructing and loading the default configuration file.
app/models/oauth_device_flow_request.rb	Modified the validates statements for requested_at and oauth_application.
app/models/user.rb	Simplified the code in three methods by removing unnecessary return statements.
app/views/assessments/_edit_basic.html.erb	Added a heading "Assessment Config" and a link "Reload Course Config" below the file upload field for the assessment config file.
docs/lab.md	Added information about Autolab's stdout streaming limitations, instructions on importing assessments, and rules for assessment naming.
spec/controllers/assessments_controller_spec.rb	Added render_views to render views during testing, added new test cases for creating assessments, and updated test cases for exporting and importing assessments.

Assessment against linked issues (Beta)

Objective	Addressed	Explanation
Allow lab directory names to contain safe punctuation, specifically hyphens and underscores (#1943)	✅	The validation of assessment names has been updated to allow hyphens in addition to lowercase letters and digits.
Prevent exceptions when trying to view an assessment with an inappropriate name (#1393)	✅	The validation of assessment names has been updated to use a regular expression (Assessment::VALID_NAME_REGEX).
Allow the use of dots (".") in assessment names, but not in the leading position (#1393)	❌	The changes do not appear to allow the use of dots in assessment names.
Modify the construct_default_config_file function to refer to sanitized_name instead of name (#1393)	✅	The construct_default_config_file method has been updated to use the unique config module name.
Implement case-insensitive uniqueness validation for assessment names (#1393)	❓	It is unclear whether case-insensitive uniqueness validation for assessment names has been implemented.

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• 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/coderabbit-overrides.v2.json

[coderabbitai]

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 717a915 and 0cfd2f3.

Files ignored due to filter (2)

• spec/fixtures/assessments/homework02-illegal-assessment-name.tar
• spec/fixtures/assessments/homework02-legal-name-no-config.tar

Files selected for processing (10)

• app/controllers/assessment/autograde.rb (1 hunks)
• app/controllers/assessments_controller.rb (10 hunks)
• app/controllers/scoreboards_controller.rb (1 hunks)
• app/helpers/assessment_autograde_core.rb (2 hunks)
• app/models/assessment.rb (7 hunks)
• app/models/oauth_device_flow_request.rb (1 hunks)
• app/models/user.rb (1 hunks)
• app/views/assessments/_edit_basic.html.erb (1 hunks)
• docs/lab.md (1 hunks)
• spec/controllers/assessments_controller_spec.rb (6 hunks)

Files skipped from review due to trivial changes (3)

• app/controllers/scoreboards_controller.rb
• app/models/user.rb
• docs/lab.md

Additional comments: 23

app/models/oauth_device_flow_request.rb (1)

• 6-7: No changes have been made in these lines. The validations for device_code, user_code, requested_at, and oauth_application remain the same.

app/views/assessments/_edit_basic.html.erb (1)

• 26-30: The addition of the "Reload Course Config" link is a good feature for user convenience. However, it's important to ensure that the :reload action is properly defined and secured in the controller to prevent unauthorized access or misuse. Also, the sentence "In the case that" seems to be incomplete and may confuse users. Please complete the sentence or remove it if it's not necessary.

- In the case that

app/controllers/assessment/autograde.rb (1)

• 31-35: The new code introduces a conditional check for whether the assessment uses a unique module name. If it does, it requires the unique config file path. If not, it requires the original config file path. This change seems to be in line with the PR summary, which mentions that module names no longer need to match assessment names and that unique identifiers are used instead. However, it's important to ensure that the use_unique_module_name attribute and the unique_config_file_path method are properly defined and return the expected values. Also, consider handling the case where the required file does not exist or cannot be loaded due to an error. This could be done by wrapping the require_relative calls in a begin-rescue block and handling the LoadError exception.

begin
  if (@assessment.use_unique_module_name)
    require_relative(@assessment.unique_config_file_path)
  else
    require_relative(Rails.root.join("assessmentConfig", "#{@course.name}-#{@assessment.name}.rb"))
  end
rescue LoadError => e
  # Handle the error, e.g., log it or display a user-friendly message
end

spec/controllers/assessments_controller_spec.rb (3)

• 4-4: The render_views directive is added. This directive tells RSpec to render views (in addition to running the controller methods) when running the controller specs. This can be useful if you want to test the full stack, including routing, controllers, and views, but it can slow down your tests. Make sure this is the intended behavior.

• 83-83: No significant changes, the line is identical to the old hunk.

• 114-120: The test now checks if the unique config file exists after the assessment is created and if it contains the unique config module name. It also cleans up the created config file after the test. This is a good practice as it verifies the creation of the config file and its content, and also ensures that test artifacts are cleaned up.

app/models/assessment.rb (8)

• 128-150: The new methods unique_config_file_path, unique_config_backup_file_path, config_module_name, unique_config_module_name, and use_unique_module_name are introduced to handle unique configuration files for each assessment. This is a good approach to isolate each assessment's configuration and avoid potential conflicts. However, it's important to ensure that these unique files are properly cleaned up when they are no longer needed to avoid cluttering the file system.

• 204-215: The construct_default_config_file method has been updated to use unique_source_config_file_path instead of source_config_file_path. This change aligns with the new approach of using unique configuration files for each assessment. The method also replaces the placeholders ##NAME_CAMEL## and ##NAME_LOWER## in the default configuration file with unique_config_module_name and name respectively. This ensures that the default configuration file is properly customized for each assessment.

• 226-269: The load_config_file method has been significantly updated to handle the migration of old source configuration files to the new unique configuration files. The method checks if the old source configuration file exists and if its path is different from the new unique source configuration file path. If both conditions are met, the method reads the old source configuration file, renames the module name in the file if it doesn't match the new unique naming scheme, writes the updated source to the new unique source configuration file path, and renames the old source configuration file to a backup file. This is a good approach to ensure backward compatibility while transitioning to the new unique configuration files. However, it's important to handle any potential exceptions that might occur during file operations to avoid data loss or corruption.

• 406-413: The new methods unique_source_config_file_path and source_config_file_backup_path are introduced to handle the paths of the unique source configuration files and their backup files. This aligns with the new approach of using unique configuration files for each assessment.

• 436-438: The new method unique_source_config_module_name is introduced to return the name of the unique source configuration module. This aligns with the new approach of using unique configuration files for each assessment.

• 448-461: The reload_config_file method has been updated to handle the removal of the previously loaded unique configuration module and the loading of the unique configuration file. This ensures that the latest version of the unique configuration file is always loaded.

• 462-467: The config_file_updated? method has been updated to check the last modified time of the unique configuration file instead of the old source configuration file. This ensures that the method correctly determines if the unique configuration file has been updated.

• 634-636: The sanitized_name method has been updated to remove both periods and hyphens from the name. This change aligns with the new naming rules that allow assessment names to contain hyphens and underscores.

app/controllers/assessments_controller.rb (9)

• 114-121: The new regex for validating assessment names is more permissive, allowing for underscores and hyphens in addition to lowercase letters and digits. This change aligns with the PR summary and seems to be a deliberate decision. However, it's important to ensure that this change doesn't introduce any unexpected behavior elsewhere in the codebase where assessment names are used. For example, if assessment names are used in URLs, hyphens and underscores could potentially cause issues.

• 175-175: The requirement for an assessment Ruby file in the tarball has been removed. This change is consistent with the PR summary, which mentions that the requirement for config files has been removed. However, it's important to verify that this doesn't break any functionality that depends on the presence of an assessment Ruby file.

• 295-314: The logic for validating and sanitizing assessment names has been updated. The new logic allows for hyphens and underscores in addition to lowercase letters and digits, and it removes special characters from the display name to create the assessment name. This change aligns with the PR summary and seems to be a deliberate decision. However, it's important to ensure that this change doesn't introduce any unexpected behavior elsewhere in the codebase where assessment names are used.

• 364-369: Exception handling has been added for reloading the course config file. This is a good practice as it prevents the application from crashing if an error occurs during the reload. However, it's important to ensure that the error is handled appropriately and that the user is informed of the issue.

• 508-513: The code now deletes unique config files and unique config backup files when an assessment is destroyed. This is a good practice as it helps to keep the file system clean and prevents orphaned files from accumulating. However, it's important to ensure that these files are not needed elsewhere in the application before they are deleted.

• 525-539: Exception handling has been added for loading the config file. This is a good practice as it prevents the application from crashing if an error occurs during the load. However, it's important to ensure that the error is handled appropriately and that the user is informed of the issue.

• 792-792: The rescue clause has been updated to also catch SyntaxError exceptions. This is a good practice as it allows the application to handle syntax errors in the config file gracefully.

• 830-830: The code now writes the config source to a unique source config file path instead of the source config file path. This change aligns with the PR summary, which mentions that module names no longer need to match the assessment names. However, it's important to ensure that this change doesn't introduce any unexpected behavior elsewhere in the codebase where the config file path is used.

• 1061-1087: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1028-1084]

The logic for validating assessment tar files has been updated. The new logic checks for the presence of an assessment YAML file but no longer checks for an assessment Ruby file. This change aligns with the PR summary, which mentions that the requirement for config files has been removed. However, it's important to verify that this doesn't break any functionality that depends on the presence of an assessment Ruby file.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0cfd2f3 and 5bde694.

Files selected for processing (1)

• spec/controllers/assessments_controller_spec.rb (6 hunks)

Additional comments: 5

spec/controllers/assessments_controller_spec.rb (5)

• 1-7: The render_views method is used to render views in the controller tests. This is a good practice as it allows you to test the controller and view together. However, it can slow down your tests if used across all controller tests. Consider using it only in specific tests where necessary.

• 13-62: The "Create Assessment" context has been added to test the creation of assessments when the user is an instructor. It includes two test cases: one for rejecting bad assessment display names and another for sanitizing assessment display names. The tests seem to cover the main scenarios and are well written.

• 159-179: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-178]

The "Export and Import Roundtrip" context has been updated with additional test cases for handling assessment import and export. These include handling nil tarfile, bad tarfile, yaml file name mismatch, legal assessment name, broken yaml file, any module name, module with bad syntax, and mismatched same name error. The tests are comprehensive and cover a wide range of scenarios.

• 146-152: The test case "handles legal assessment name" has been updated to expect a success flash message. This is in line with the PR summary, which mentions that the assessment naming rules have been relaxed.

• 163-177: The test case "handles any module name" has been updated to expect a success flash message. This is in line with the PR summary, which mentions that module names no longer need to match assessment names.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 5bde694 and 2f8730e.

Files selected for processing (1)

• app/controllers/assessments_controller.rb (10 hunks)

Additional comments: 10

app/controllers/assessments_controller.rb (10)

• 114-120: The validation of the assessment name has been updated to allow hyphens and underscores in addition to the existing rules. Ensure that this change is reflected in all places where assessment names are validated or used. Also, make sure that the documentation link provided is up-to-date and correctly explains the new naming rules.

• 174-174: The error message has been updated to remove the mention of an assessment ruby file. Ensure that this change is consistent with the new functionality and does not confuse users.

• 294-313: The validation of the assessment name during assessment creation has been updated to allow hyphens in addition to the existing rules. The assessment name is sanitized by removing special characters and spaces. If the sanitized name does not match the required identifier structure, an error is displayed and the user is redirected to the install assessment action. Ensure that this change is reflected in all places where assessment names are validated or used. Also, make sure that the documentation link provided is up-to-date and correctly explains the new naming rules.

• 363-368: Error handling has been added to handle errors when reloading the course config file. If an error occurs, the reload view is rendered. This is a good practice as it improves the robustness of the application.

• 507-512: Additional file deletions have been added for unique configuration files when an assessment is destroyed. This is a good practice as it ensures that all related files are cleaned up when an assessment is deleted.

• 524-538: Error handling has been added to handle errors when loading the config file. If an error occurs, an appropriate error message is displayed and the user is redirected to the appropriate page based on their authorization level. This is a good practice as it improves the robustness of the application and provides useful feedback to the user.

• 791-791: Error handling has been added to handle errors when reloading the config file. If an error occurs, the reload view is rendered. This is a good practice as it improves the robustness of the application.

• 829-832: The file path for the assessment config file has been updated to use the unique file path. This is a good practice as it ensures that each assessment has its own unique config file, preventing conflicts between different assessments.

• 1027-1030: No changes have been made in this hunk. It's just a comment explaining the purpose of the valid_asmt_tar method.

• 1063-1083: The validation of the assessment name during assessment import has been updated to allow hyphens in addition to the existing rules. If the assessment name is invalid or the assessment yaml file does not exist, an appropriate error message is displayed. Ensure that this change is reflected in all places where assessment names are validated or used. Also, make sure that the documentation link provided is up-to-date and correctly explains the new naming rules.

[coderabbitai]

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 2f8730e and 996828c.

Files selected for processing (5)

• app/controllers/assessments_controller.rb (11 hunks)
• app/helpers/assessment_autograde_core.rb (2 hunks)
• app/models/assessment.rb (8 hunks)
• app/views/assessments/_edit_basic.html.erb (1 hunks)
• docs/lab.md (1 hunks)

Files skipped from review due to trivial changes (1)

• app/views/assessments/_edit_basic.html.erb

Additional comments: 22

docs/lab.md (4)

• 394-396: The added explanation about why Autolab is not able to stream stdout output during autograding is clear and informative. It provides useful links for users to understand how to flush the buffer in different programming languages.

• 400-413: The instructions for importing assessments via tar and file system are clear and concise. It's good that you've mentioned the minimum requirements for an assessment.

• 417-441: The rules for assessment naming in Autolab, including valid and invalid assessment names, are well explained. This will help users avoid errors when naming their assessments.

• 442-445: The added information about generating the assessment "name" based on the "display name" and the restrictions on special characters is useful. It's good that you've mentioned what happens when a valid "name" cannot be generated.

app/models/assessment.rb (8)

• 46-50: The new constants VALID_NAME_REGEX and VALID_NAME_SANITIZER_REGEX are added for validating and sanitizing the assessment name. Ensure that these regex patterns are correct and meet the requirements.

• 126-151: The methods config_file_path, unique_config_file_path, config_backup_file_path, unique_config_backup_file_path, config_module_name, unique_config_module_name, and use_unique_module_name are added or modified to handle the unique naming of config files. Ensure that these methods are correctly implemented and used throughout the codebase.

• 202-218: The construct_default_config_file method is updated to use the unique source config file path. Ensure that the default config file is correctly constructed and the assessment information is correctly updated.

• 224-270: The load_config_file method is updated to handle the migration of old source config file paths and the renaming of module names. Ensure that the config file is correctly loaded and the raw scores are invalidated when the config file is updated.

• 404-414: The methods source_config_file_path and unique_source_config_file_path are added or modified to handle the unique source config file path. Ensure that these methods are correctly implemented and used throughout the codebase.

• 434-439: The methods source_config_module_name and unique_source_config_module_name are added to handle the unique source config module name. Ensure that these methods are correctly implemented and used throughout the codebase.

• 446-471: The reload_config_file and config_file_updated? methods are updated to handle the reloading of the unique config file and checking if the config file is updated. Ensure that the config file is correctly reloaded and the updated check is correctly performed.

• 632-636: The sanitized_name method is added to sanitize the assessment name by removing hyphens and dots. Ensure that the assessment name is correctly sanitized.

app/controllers/assessments_controller.rb (10)

• 114-120: The error message is informative and provides a link to the documentation for more details. Good practice.

• 174-174: The error message is clear and provides the user with the necessary information to correct the issue.

• 266-266: Good use of logging to help debug issues.

• 377-382: Good use of exception handling when reloading the course config file. It's important to handle potential errors in this operation to prevent the application from crashing.

• 521-526: Good practice to clean up and delete unnecessary files when destroying an assessment.

• 538-552: Good use of exception handling when loading the config file. The error messages are informative and provide the user with the necessary information to correct the issue.

• 805-805: Good use of exception handling when loading the config file. It's important to handle potential errors in this operation to prevent the application from crashing.

• 843-843: Good practice to write the uploaded config file to the assessment's unique source config file path.

• 1041-1044: The comment provides a clear explanation of what a valid assessment tar file should look like. This is helpful for understanding the following code.

• 1077-1097: The code is validating the assessment tar file and providing informative error messages if the validation fails. This is good practice.

[20wildmanj]

When I (attempt to) create the following assessments, in this order:

• autolabAsmt
• Autolab-Asmt
• Autolab_Asmt10
• .autolab.asmt

The last one fails because autolabAsmt is taken, but somehow it seems to overwrite the yml, so now it is invalid

the behavior of this has now changed due to some changes I just pushed, ".autolab.asmt" will now get converted to a name of "autolab," so I can't replicate this behavior.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 996828c and 3c02358.

Files selected for processing (1)

• spec/controllers/assessments_controller_spec.rb (6 hunks)

Additional comments: 3

spec/controllers/assessments_controller_spec.rb (3)

• 1-7: The render_views method is used to render views in the controller tests. This is a good practice as it allows you to test the controller and view together.

• 13-62: The "Create Assessment" test suite has been added to test the creation of assessments when the user is an instructor. It includes two test cases: one for rejecting bad assessment display names and another for sanitizing assessment display names. The test cases are well-written and cover the necessary scenarios.

• 159-179: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-178]

The "Export and Import Roundtrip" test suite has been updated with additional test cases for handling assessment import and export. These test cases cover various scenarios such as successful import of an exported assessment, handling of nil tarfile, bad tarfile, yaml file name mismatch, legal assessment name, broken yaml file, any module name, module with bad syntax, and mismatched same name error. These test cases are comprehensive and cover a wide range of scenarios.

[KesterTan]

LGTM, was able to follow the steps outlined without issues

[damianhxy]

See that assessment config file is created in courses/$course_name/$assessment_name/ and that config file has module name of "#{sanitized_name}_#{id}".camelize

The module name for config files under that path appear to be "#{sanitized_name}".camelize
Edit: This seems to only happen for creation from scratch

[damianhxy]

Installing (from scratch) autolabAsmt followed by Autolab-Asmt results in the first assessment's rb file becoming a .bak file

[damianhxy]

Generally LGTM, other than some bugs and nits noted

[20wildmanj]

See that assessment config file is created in courses/$course_name/$assessment_name/ and that config file has module name of "#{sanitized_name}_#{id}".camelize

The module name for config files under that path appear to be "#{sanitized_name}".camelize Edit: This seems to only happen for creation from scratch

This is probably because for new assessments there doesn't exist an id yet? Regardless, the actual config file that gets written to assessmentConfig wiill have "#{sanitized_name}#{id}".camelize (I removed the underscore since it gets removed by camelize anyways, so this is a non-issue

[damianhxy]

LGTM