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

Jump to bottom

Fix currency list on new money gem #19567

Merged
merged 3 commits into from
Dec 2, 2019
Merged

Fix currency list on new money gem #19567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77cddbf
Add support for new UZS currency in money 6.13.5
Fryguy Dec 2, 2019
e46b614
Check for blank symbols directly instead of silently failing validation
Fryguy Dec 2, 2019
3e5a561
Cleanup the spec file
Fryguy Dec 2, 2019
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
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ gem "manageiq-password", "~>0.3", :require => false
gem "manageiq-postgres_ha_admin", "~>3.1", :require => false
gem "memoist", "~>0.15.0", :require => false
gem "mime-types", "~>3.0", :path => File.expand_path("mime-types-redirector", __dir__)
gem "money", "~>6.13.5", :require => false
Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy I think the Money class was coming from the money-rails gem here - https://github.com/ManageIQ/manageiq-consumption/blob/c7198b50c03944497b00fd28284caf723a88ff1b/manageiq-consumption.gemspec#L20, not the money gem. I'm not sure that it matters since we only use the gem for its list of currencies, but I thought I should mention it. I had added the money-rails gem to the PR that decouples us from manageiq-consumption and noticed this what I tried to rebase it with this change.

gem "more_core_extensions", "~>3.7"
gem "net-ldap", "~>0.16.1", :require => false
gem "net-ping", "~>1.7.4", :require => false
Expand Down
11 changes: 9 additions & 2 deletions app/models/chargeback_rate_detail_currency.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require "money"

class ChargebackRateDetailCurrency < ApplicationRecord
belongs_to :chargeback_rate_detail

Expand Down Expand Up @@ -42,15 +44,20 @@ def self.seed
if File.exist?(currency_file_source)
fixture_mtime_currency = File.mtime(currency_file_source).utc
currencies.each do |currency|
if currency[:symbol].blank?
_log.info("Skipping [#{currency[:code]}] due to missing symbol")
next
end

rec = db_currencies[currency[:code]]
if rec.nil?
_log.info("Creating [#{currency[:code]}] with symbols=[#{currency[:symbol]}]")
ChargebackRateDetailCurrency.create(currency)
ChargebackRateDetailCurrency.create!(currency)
elsif fixture_mtime_currency > rec.created_at
rec.attributes = currency
if rec.changed?
_log.info("Updating [#{currency[:code]}] with symbols=[#{currency[:symbol]}]")
rec.update(:created_at => fixture_mtime_currency)
rec.update!(:created_at => fixture_mtime_currency)
end
end
end
Expand Down
13 changes: 6 additions & 7 deletions spec/models/chargeback_rate_detail_currency_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,30 @@
describe ChargebackRateDetailCurrency do
describe '.seed' do
before do
it "returns supported currencies" do
ChargebackRateDetailCurrency.seed
end

let(:expected_currencies) do
%w(AED AFN ALL AMD ANG AOA ARS AUD AWG AZN BAM BBD BDT BGN BHD BIF BMD BND BOB BRL BSD BTN BWP BYN BYR BZD CAD CDF CHF CLF CLP CNY COP CRC CUC CUP CVE CZK DJF DKK DOP DZD EGP ERN ETB EUR FJD FKP GBP GEL GHS GIP GMD GNF GTQ GYD HKD HNL HRK HTG HUF IDR ILS INR IQD IRR ISK JMD JOD JPY KES KGS KHR KMF KPW KRW KWD KYD KZT LAK LBP LKR LRD LSL LYD MAD MDL MGA MKD MMK MNT MOP MRO MUR MVR MWK MXN MYR MZN NAD NGN NIO NOK NPR NZD OMR PAB PEN PGK PHP PKR PLN PYG QAR RON RSD RUB RWF SAR SBD SCR SDG SEK SGD SHP SKK SLL SOS SRD SSP STD SVC SYP SZL THB TJS TMT TND TOP TRY TTD TWD TZS UAH UGX USD UYU VES VND VUV WST XAF XAG XAU XCD XDR XOF XPD XPF XPT YER ZAR ZMK ZMW)
end
expected_currencies = %w[AED AFN ALL AMD ANG AOA ARS AUD AWG AZN BAM BBD BDT BGN BHD BIF BMD BND BOB BRL BSD BTN BWP BYN BYR BZD CAD CDF CHF CLF CLP CNY COP CRC CUC CUP CVE CZK DJF DKK DOP DZD EGP ERN ETB EUR FJD FKP GBP GEL GHS GIP GMD GNF GTQ GYD HKD HNL HRK HTG HUF IDR ILS INR IQD IRR ISK JMD JOD JPY KES KGS KHR KMF KPW KRW KWD KYD KZT LAK LBP LKR LRD LSL LYD MAD MDL MGA MKD MMK MNT MOP MRU MUR MVR MWK MXN MYR MZN NAD NGN NIO NOK NPR NZD OMR PAB PEN PGK PHP PKR PLN PYG QAR RON RSD RUB RWF SAR SBD SCR SDG SEK SGD SHP SKK SLL SOS SRD SSP STD SVC SYP SZL THB TJS TMT TND TOP TRY TTD TWD TZS UAH UGX USD UYU UZS VES VND VUV WST XAF XAG XAU XCD XDR XOF XPD XPF XPT YER ZAR ZMK ZMW]
Copy link
Member Author

@Fryguy Fryguy Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hard to tell in the PR diff (easier to see in the first commit's diff), but there are 2 changes here...

  • Added UZS
  • Renamed MRO to MRU

The latter is interesting because it seems seed doesn't delete old currencies, nor does it have a way to "update" old currencies where the code has changed. I'm not sure if that's a good thing or a bad thing.

@lpichler Thoughts? Should we have a data migration moving MRO to MRU, and then delete old currencies? Or should we just leave it the way it is, which is technically backwards compatibile?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with keeping this for now but this test feels brittle. We'll have to change this whenever money adds/removes/renames currencies, right? Maybe we should be looking for specific subset of newer currencies we want to test against?

I do like the dropping of the currency count test though. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the test is brittle, and a subset is the only thing I can think of as well. I think that's better as a follow up PR though.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, let's do the followup after this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lpichler Thoughts? Should we have a data migration moving MRO to MRU, and then delete old currencies? Or should we just leave it the way it is, which is technically backwards compatibile?

@Fryguy Change MRO to MRU is basically: removing one old currency and adding new.
There are different they have different rate.

So if anybody used MRO - I prefer to keep MRO as he selected it.

If anybody created report/chargeback rate/services/,,, with MRO and we will change it to MRU without conversions of prices and are changing meaning of it - so I suggest don't change it.

If there will be change in symbol because of typo,.. we can create migration symbol is unique identification for seeding.

We'll have to change this whenever money adds/removes/renames currencies, right?

this is a reason why there is the test - to be informed about external changes.

anyway do you think that we can copy their json with currencies to our project ? (I don't know how to judge this) because we need (or we will soon) money gem just because of this file(list of currencies).

thanks!


it "returns supported currencies" do
expect(ChargebackRateDetailCurrency.count).to eq(164)
Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped this count test since it's really just a duplicate of checking all of the currencies

expect(ChargebackRateDetailCurrency.all.map(&:code)).to match_array(expected_currencies)
end
end

it "has a valid factory" do
expect(FactoryBot.create(:chargeback_rate_detail_currency)).to be_valid
end

it "is invalid without a code" do
expect(FactoryBot.build(:chargeback_rate_detail_currency, :code => nil)).not_to be_valid
end

it "is invalid without a name" do
expect(FactoryBot.build(:chargeback_rate_detail_currency, :name => nil)).not_to be_valid
end

it "is invalid without a full_name" do
expect(FactoryBot.build(:chargeback_rate_detail_currency, :full_name => nil)).not_to be_valid
end

it "is invalid without a symbol" do
expect(FactoryBot.build(:chargeback_rate_detail_currency, :symbol => nil)).not_to be_valid
end
Expand Down