update remote_autotest_settings_id validation #7393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
donny-wong
force-pushed
the
update_remote_autotest_settings_id_validation
branch
from
1b040f4 to
4a7218b
Compare
Pull Request Test Coverage Report for Build 12998164448Details
💛 - Coveralls |
|
|
||
| # Ensure remote_autotest_settings_id is unique for a given autotester | ||
| def remote_autotest_settings_id_check | ||
| unless self.remote_autotest_settings_id.nil? |
There was a problem hiding this comment.
Instead of having the check here, use the :if argument to validate at the top
| @@ -126,4 +126,18 @@ def not_timed_and_scanned | |||
| msg = I18n.t('activerecord.errors.models.assignment_properties.attributes.is_timed.not_scanned') | |||
| errors.add(:base, msg) if is_timed && scanned_exam | |||
| end | |||
|
|
|||
| # Ensure remote_autotest_settings_id is unique for a given autotester | |||
| def remote_autotest_settings_id_check | |||
There was a problem hiding this comment.
The name "check" is a bit generic. Please rename to remote_autotest_settings_id_uniqueness
| # Ensure remote_autotest_settings_id is unique for a given autotester | ||
| def remote_autotest_settings_id_check | ||
| unless self.remote_autotest_settings_id.nil? | ||
| assign_prop = AssignmentProperties.joins(assignment: :course).where( |
There was a problem hiding this comment.
This query is good but can be improved in two ways:
- Add a
.notquery to eliminate the record withid: self.id. Note that this can be chained with the original query, i.e., you can have a.where(...).not.where(...) - Add a
.exists?at the end, this will simplify the return value to just a boolean that you can use for the condition.
| ) | ||
| if assign_prop.count == 1 && self.id != assign_prop.first.id | ||
| errors.add(:remote_autotest_settings_id, | ||
| 'remote_autotest_settings_id has already been taken by the same autotester.') |
There was a problem hiding this comment.
Make this an internationalized string. You can add it to the corresponding models YML file.
| it { is_expected.to validate_uniqueness_of(:remote_autotest_settings_id).allow_nil } | ||
| it 'should not be valid when already taken by the same autotester' do | ||
| expect(assignment2).not_to be_valid | ||
| expect(assignment2.errors.full_messages[0]).to include('remote_autotest_settings_id has already been taken') |
There was a problem hiding this comment.
refer to the internationalized string (which I asked you to add in an above comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Currently, the remote_autotest_settings_id validation on assignment_properties model is set to being unique. This would cause the unique validation error if an autotester returns an autotest_settings_id number to be set on remote_autotest_settings_id, but that number has already been taken by another autotester. We would want to allow this kind of scenario.
To fix this, we will need to update the unique validation constraint to have the remote_autotest_settings_id to be unique to an autotester.
Testing steps:
...
Screenshots of your changes (if applicable)
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)