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

SJ - Fixing indirect cost validation [#152641470] #1184

Merged
merged 5 commits into from
Nov 9, 2017

Conversation

Stuart-Johnson
Copy link
Collaborator

@wtholt
Copy link
Contributor

wtholt commented Nov 7, 2017

@Stuart-Johnson Looks good, but I think you can git checkout those db/schema.rb changes.

@@ -38,6 +38,8 @@
end
end

it { is_expected.to validate_numericality_of(:indirect_cost_rate).is_greater_than_or_equal_to(1) }
it { is_expected.to validate_numericality_of(:indirect_cost_rate).is_less_than_or_equal_to(1000) }
if Setting.find_by_key('use_indirect_cost').value
Copy link
Contributor

Choose a reason for hiding this comment

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

After discussing with @Stuart-Johnson, we should not use an if condition here, but instead a before :each do block where use_indirect_cost is turned on.

before :each do
  stub_config("use_indirect_cost", true)
end

@kayla-glick
Copy link
Contributor

Like @wtholt said and after discussing with @Stuart-Johnson, it seems like the schema changes should be checked out, though we may want to consider the effect of the inconsistent collation in the schema file. Is it a big deal or does it need to be updated? Schema does update itself automatically,but it only updates the information for specific tables being changed in migrations, so because a lot of older tables have not be changed in a while, the collation may be out-of-date.

@Stuart-Johnson Stuart-Johnson merged commit 93abd69 into master Nov 9, 2017
@Stuart-Johnson Stuart-Johnson deleted the sj-hotfix_for_indirect_cost branch November 9, 2017 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants