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.

Sign up for GitHub

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

Add condition to fix deletion of Default Container Image Rate #16792

Merged
merged 2 commits into from
Jan 24, 2018
Merged

Add condition to fix deletion of Default Container Image Rate #16792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad0476e
Add condition to fix deletion of Default Container Image Rate
Jan 10, 2018
65ddd88
Add spec test for Default Container Image Rate
Jan 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/chargeback_rate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def ensure_unassigned
end

def ensure_nondefault
if default?
if default? || description == 'Default Container Image Rate'
Copy link
Contributor

Choose a reason for hiding this comment

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

@hstastna few questions, can't description be edited, or have same chargeback rate with same description, or is description unique among chargeback rates?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gtanzillo @lpichler if "Default Container Image Rate" is a default rate that gets seeded from yml, why dont we change yml to populate default field as true here https://github.com/ManageIQ/manageiq/blob/master/db/fixtures/chargeback_rates.yml#L155

Copy link
Author

@hstastna hstastna Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@h-kataria If I click on Edit this Chargeback rate and try to change the description of 'Default Container Image Rate' and click on Save, the flash message occurs:
Can not change description of 'Default Container Image Rate'
If I try to add a new rate with the same description, the flash message occurs:
Description has already been taken
so I think this all works properly. I hope I understood your question properly 👼

Copy link
Contributor

Choose a reason for hiding this comment

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

@hstastna i see that change was made here ManageIQ/manageiq-ui-classic#269 to not allow edit of Default Container Image chargeback rate. I still think it should have been marked as default rate to begin with. @gtanzillo @lpichler @dclarizio any comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

@h-kataria we added it here #13063 and what I remember we needed to distinguish this specific rate and it was possible only according to name. But yes it isn't the reason why we didn't set ChargebackRate#default to true in yaml. Maybe it was causing issues. Can you remember @zeari ?

Copy link

Choose a reason for hiding this comment

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

What we needed was a default-fallback rate, that if no rate is assigned that one is used.
We also needed the ability to edit it which wasnt possible for default rates.

Since default rates were not editable we made a special case of out "Default container image rate". The only place where that is used is in Chargeback for container images.

errors.add(:rate, "default rate cannot be deleted")
throw :abort
end
Expand Down
8 changes: 8 additions & 0 deletions spec/models/chargeback_rate_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@
expect(cbr.errors.first).to include("default rate cannot be deleted")
end

it "when non-default with default description" do
cbr = FactoryGirl.create(:chargeback_rate, :description => "Default Container Image Rate", :default => false)
cbr.destroy
expect(cbr).to_not be_destroyed
expect(cbr.errors.count).to be(1)
expect(cbr.errors.first).to include("default rate cannot be deleted")
end

it "when non-default" do
cbr = FactoryGirl.create(:chargeback_rate, :description => "Non-default", :default => false)
cbr.destroy
Expand Down