From 754c955fc95995c35b756377af1c26a2825c80fd Mon Sep 17 00:00:00 2001 From: Augusto Callejas Date: Fri, 8 Jun 2018 07:49:13 -0400 Subject: [PATCH] Only allow non-negative points refs OUT-2240 test plan: - enable the non-scoring rubric feature flag if not enabled - load the accounts rubrics page - click the "Learning Mastery" tab - confirm that when attempting to save ratings with negative points, an error message is displayed below the points field Change-Id: Ic41b013cbc6499e3c1783fed2283fe6ed02ba2f0 Reviewed-on: https://gerrit.instructure.com/153069 Reviewed-by: Michael Brewer-Davis Reviewed-by: Matt Berns Tested-by: Jenkins QA-Review: Michael Brewer-Davis Product-Review: Sidharth Oberoi --- app/jsx/rubrics/ProficiencyTable.js | 10 ++++- .../__tests__/ProficiencyTable.test.js | 42 +++++++++++++++++++ app/models/outcome_proficiency_rating.rb | 2 +- .../models/outcome_proficiency_rating_spec.rb | 1 + 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/app/jsx/rubrics/ProficiencyTable.js b/app/jsx/rubrics/ProficiencyTable.js index 1398f0c7885..00a46467bc0 100644 --- a/app/jsx/rubrics/ProficiencyTable.js +++ b/app/jsx/rubrics/ProficiencyTable.js @@ -142,7 +142,7 @@ export default class ProficiencyTable extends React.Component { handlePointsChange = _.memoize((index) => (value) => { const parsed = NumberHelper.parse(value) let rows = this.state.rows - if (!this.invalidPoints(parsed)) { + if (!this.invalidPoints(parsed) && parsed >= 0) { rows = rows.removeIn([index, 'pointsError']) } rows = rows.setIn([index, 'points'], parsed) @@ -164,7 +164,7 @@ export default class ProficiencyTable extends React.Component { }) isStateValid = () => !this.state.rows.some(row => - this.invalidPoints(row.get('points')) || this.invalidDescription(row.get('description'))) + this.invalidPoints(row.get('points')) || row.get('points') < 0 || this.invalidDescription(row.get('description'))) stateToConfig = () => ({ @@ -208,6 +208,12 @@ export default class ProficiencyTable extends React.Component { r = r.set('focusField', 'points') firstError = false } + } else if (row.get('points') < 0) { + r = r.set('pointsError', I18n.t('Negative points')) + if (firstError) { + r = r.set('focusField', 'points') + firstError = false + } } return r }) diff --git a/app/jsx/rubrics/__tests__/ProficiencyTable.test.js b/app/jsx/rubrics/__tests__/ProficiencyTable.test.js index 1a7a2c1615e..f0d56189384 100644 --- a/app/jsx/rubrics/__tests__/ProficiencyTable.test.js +++ b/app/jsx/rubrics/__tests__/ProficiencyTable.test.js @@ -73,6 +73,42 @@ describe('default proficiency', () => { }) }) + it('setting blank description sets error', () => { + const wrapper = mount() + return promise.then(() => { + wrapper.instance().handleDescriptionChange(0)("") + wrapper.find('Button').last().simulate('click') + expect(wrapper.find('ProficiencyRating').first().prop('descriptionError')).toBe('Missing required description') + }) + }) + + it('setting blank points sets error', () => { + const wrapper = mount() + return promise.then(() => { + wrapper.instance().handlePointsChange(0)("") + wrapper.find('Button').last().simulate('click') + expect(wrapper.find('ProficiencyRating').first().prop('pointsError')).toBe('Invalid points') + }) + }) + + it('setting invalid points sets error', () => { + const wrapper = mount() + return promise.then(() => { + wrapper.instance().handlePointsChange(0)("1.1.1") + wrapper.find('Button').last().simulate('click') + expect(wrapper.find('ProficiencyRating').first().prop('pointsError')).toBe('Invalid points') + }) + }) + + it('setting negative points sets error', () => { + const wrapper = mount() + return promise.then(() => { + wrapper.instance().handlePointsChange(0)("-1") + wrapper.find('Button').last().simulate('click') + expect(wrapper.find('ProficiencyRating').first().prop('pointsError')).toBe('Negative points') + }) + }) + it('sends POST on submit', () => { const postSpy = jest.spyOn(axios,'post').mockImplementation(() => Promise.resolve({status: 200})) const wrapper = mount() @@ -154,3 +190,9 @@ it('invalid rating points leaves state invalid', () => { wrapper.instance().handlePointsChange(0)("1.1.1") expect(wrapper.instance().isStateValid()).toBe(false) }) + +it('negative rating points leaves state invalid', () => { + const wrapper = shallow() + wrapper.instance().handlePointsChange(0)("-1") + expect(wrapper.instance().isStateValid()).toBe(false) +}) diff --git a/app/models/outcome_proficiency_rating.rb b/app/models/outcome_proficiency_rating.rb index 865cfd44cf9..ed10ad45715 100644 --- a/app/models/outcome_proficiency_rating.rb +++ b/app/models/outcome_proficiency_rating.rb @@ -20,7 +20,7 @@ class OutcomeProficiencyRating < ApplicationRecord belongs_to :outcome_proficiency, inverse_of: :outcome_proficiency_ratings validates :description, presence: true - validates :points, presence: true + validates :points, presence: true, numericality: { greater_than_or_equal_to: 0 } validates :color, presence: true, format: /\A([A-Fa-f0-9]{6})\z/i def as_json(_options={}) diff --git a/spec/models/outcome_proficiency_rating_spec.rb b/spec/models/outcome_proficiency_rating_spec.rb index 339d1754deb..9fe3773a098 100644 --- a/spec/models/outcome_proficiency_rating_spec.rb +++ b/spec/models/outcome_proficiency_rating_spec.rb @@ -26,6 +26,7 @@ describe OutcomeProficiencyRating, type: :model do describe 'validations' do it { is_expected.to validate_presence_of :description } it { is_expected.to validate_presence_of :points } + it { is_expected.to validate_numericality_of(:points).is_greater_than_or_equal_to(0) } it { is_expected.to allow_value('0F160a').for(:color) } it { is_expected.not_to allow_value('#0F160a').for(:color) } it { is_expected.not_to allow_value('').for(:color) }