Fix currency list on new money gem #19567
Conversation
Fryguy
changed the title
Additionally add in the money gem to the Gemfile since we use it directly now.
Fryguy
force-pushed
the
fix_currency_list_on_new_gem
branch
from
03825af
to
3e5a561
Compare
|
|
||
| it "returns supported currencies" do | ||
| expect(ChargebackRateDetailCurrency.count).to eq(164) |
There was a problem hiding this comment.
Dropped this count test since it's really just a duplicate of checking all of the currencies
| 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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah, let's do the followup after this PR is merged.
There was a problem hiding this comment.
@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!
|
Checked commits Fryguy/manageiq@77cddbf~...3e5a561 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -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 | |||
There was a problem hiding this comment.
@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.
|
@Fryguy What do we want to do for ivanchuk branch which is also failing? Downstream build is using money 6.13.4. Lock to that version or backport this PR? |
money 6.13.5 added the symbol for UZS. Previously the code silently failed validation on the blank symbol, but now it doesn't and the specs fail. This PR updates to the latest, makes the specs pass, and then adds in some better handling of blank symbols from the file instead of silently failing validation. It also adds the money gem directly to the Gemfile since ChargebackRateDetailCurrency uses it directly.
@lpichler @jrafanie Please review.