Store country band details in database #10476

danieljames-dj · 2024-12-25T04:02:17Z

This PR aims at removing some hard-coding related to dues details. This will help to make the changes quickly in case we need to make some change in dues in future.

app/controllers/country_bands_controller.rb

db/migrate/20241225031242_populate_country_band_details.rb

lib/dues_calculator.rb

gregorbg · 2025-01-14T12:25:04Z

lib/dues_calculator.rb

    country_band = CountryBand.find_by(iso2: country_iso2)&.number
+    country_band_detail = CountryBandDetail.find_by(number: country_band)


You're completely ignoring your new belongs_to association here.

Oh sorry, changed it.

gregorbg · 2025-01-14T12:25:29Z

lib/dues_calculator.rb


-    [registration_fee_dues_us_dollars, country_band_dues_us_dollars_money].max
+    # Calculation of 'country band dues'
+    country_band_dues = Money.new(country_band_detail&.due_amount_per_competitor_in_cents || 0, "USD")


Too much happening in one line

Cleaned it a bit

gregorbg · 2025-01-14T12:25:39Z

lib/dues_calculator.rb

-    # times 100 because Money require lowest currency subunit, which is cents for USD
-    country_band_dues_us_dollars_money = Money.new(country_band_dues_us_dollars * 100, "USD")
+    # Calculation of 'registration fee dues'
+    registration_fee_dues = Money.new(registration_fees * (country_band_detail&.due_percent_registration_fee.to_f || 0) / 100, "USD")


Too much happening in one line

Cleaned it a bit

gregorbg · 2025-01-14T12:27:48Z

app/models/country_band.rb

  belongs_to :country, foreign_key: :iso2, primary_key: :iso2
+  belongs_to :country_band_detail, foreign_key: :number, primary_key: :number


I don't fully understand the idea behind this CountryBandDetails model. It has a start date and an end date, but every CountryBand (which is perpetual/static) contains a column which points it to exactly one CountryBandDetail.

That means, you could point Band 5 to a CountryBandDetail from 3 years ago, which isn't even valid anymore.
I know you will probably tell me that "WFC will prevent that" but I want to challenge you to see whether you can come up with some better database design first to avoid these problems from happening in the first place

Okay, I didn't think of that case. I've now moved country_band_detail into a function, and I think this is solving the issue.

Or in other words, I'll not have tight coupling between CountryBand and CountryBandDetail, instead CountryBand table will represent the band number of a country (maybe in future we can introduce start_date and end_date as well if we need the history of bands of a country). From the CountryBandDetails table, we can get the active details of a country band.

gregorbg · 2025-02-05T10:30:52Z

app/views/country_bands/_show_band.html.erb

-    <%= t("country_bands.band_title", number: number) %>
-    (<%= t("country_bands.due_value", value: data[:value]) %>)
+    <%= t("country_bands.band_title", number: country_band_detail.number) %>
+    (<%= t("country_bands.due_value", value: country_band_detail.due_amount_per_competitor_in_cents.to_f / 100) %>)


I'm 60% sure you don't need the to_f

gregorbg · 2025-02-05T10:33:08Z

app/models/country_band_detail.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class CountryBandDetail < ApplicationRecord


You can specify a belongs_to :country_band association by providing primary_key: :number and foreign_key: :number I think.

Don't confuse these with SQL PRIMARY KEY and FOREIGN KEY. The options I'm talking about here are (easily misunderstandable) Rails AR vocabulary, which means "the key [i.e. column] where I look up information in the foreign [i.e. pointing to] model, or in myself [i.e. primary]".

Oh wait, removed it for now. But looks like the association is not used anywhere, is it still needed to have this association here?

gregorbg · 2025-02-05T10:35:18Z

app/models/country_band.rb

+  def active_country_band_detail
+    CountryBandDetail.active.find_by(number: self.number)
+  end


As per my other comment in CountryBandDetail, I think you can solve this via a has_many relation.

gregorbg · 2025-02-05T10:36:06Z

db/schema.rb

+    t.integer "number", null: false
+    t.date "start_date", null: false
+    t.date "end_date"
+    t.integer "due_amount_per_competitor_in_cents", null: false


Maybe name this column due_amount_per_competitor_us_cents to make clear that the currency is pre-defined?

gregorbg · 2025-02-05T10:38:23Z

lib/dues_calculator.rb

+
+    # Calculation of 'registration fee dues'
+    due_percent_registration_fee = country_band_detail&.due_percent_registration_fee.to_f || 0
+    registration_fee_dues = Money.new(registration_fees * due_percent_registration_fee / 100, "USD")


I think you can do calculations with the money object directly:

my_fee = Money.new(2000, "USD") some_percentage = my_fee * 0.8

gregorbg · 2025-02-05T10:38:55Z

lib/dues_calculator.rb

+    registration_fees = Money.new(base_entry_fee_lowest_denomination, currency_code).exchange_to("USD")
+
+    # Calculation of 'registration fee dues'
+    due_percent_registration_fee = country_band_detail&.due_percent_registration_fee.to_f || 0


Again, I think you don't need the to_f. Ruby is very flexible with numbers.

gregorbg · 2025-02-05T10:40:29Z

app/models/country_band.rb

  belongs_to :country, foreign_key: :iso2, primary_key: :iso2
  validates_inclusion_of :iso2, in: Country::WCA_COUNTRY_ISO_CODES
-  validates_inclusion_of :number, in: BANDS.keys.freeze


Can you hard-code the range from 0 to 5 instead of just completely deleting the line?
I know hard-coding is not very pretty, but effectively it was already hard-coded before (through an intermediate step which was getting the keys from the hard-coded hash)

gregorbg · 2025-02-05T10:41:16Z

spec/models/country_band_spec.rb

@@ -15,6 +15,6 @@

  it "invalidates band with invalid band id" do
    cb = CountryBand.new(number: 6, iso2: "HELLO")
-    expect(cb).to be_invalid_with_errors(number: ["is not included in the list"])
+    expect(cb).to be_invalid_with_errors(country: ["must exist"])


Regarding my other comment above, feel free to re-add the previous "number not included in the list" validation.

Also, why did the country validation that you're testing now not exist/fail before?

Added back that error. I just realized the country validation test was already there in the unit test just before this.

danieljames-dj

@gregorbg Thanks for your review, I've partially attended your comments. But since some changes are clashing with changes in #10767, I'll wait for that PR to get merged before attending the remaining.

danieljames-dj · 2025-02-06T06:00:16Z

app/models/country_band.rb

  belongs_to :country, foreign_key: :iso2, primary_key: :iso2
  validates_inclusion_of :iso2, in: Country::WCA_COUNTRY_ISO_CODES
-  validates_inclusion_of :number, in: BANDS.keys.freeze


danieljames-dj · 2025-02-06T06:01:25Z

app/models/country_band_detail.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class CountryBandDetail < ApplicationRecord


danieljames-dj · 2025-02-06T06:03:11Z

app/models/country_band.rb

+  def active_country_band_detail
+    CountryBandDetail.active.find_by(number: self.number)
+  end


danieljames-dj · 2025-02-06T06:06:16Z

app/models/country_band_detail.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class CountryBandDetail < ApplicationRecord


Oh wait, removed it for now. But looks like the association is not used anywhere, is it still needed to have this association here?

danieljames-dj · 2025-02-06T06:16:21Z

spec/models/country_band_spec.rb

@@ -15,6 +15,6 @@

  it "invalidates band with invalid band id" do
    cb = CountryBand.new(number: 6, iso2: "HELLO")
-    expect(cb).to be_invalid_with_errors(number: ["is not included in the list"])
+    expect(cb).to be_invalid_with_errors(country: ["must exist"])


Added back that error. I just realized the country validation test was already there in the unit test just before this.

github-actions bot assigned danieljames-dj Dec 25, 2024

danieljames-dj force-pushed the country_band_details branch 15 times, most recently from d02c82a to 9c50e1a Compare January 4, 2025 10:05

gregorbg requested changes Jan 4, 2025

View reviewed changes

app/controllers/country_bands_controller.rb Outdated Show resolved Hide resolved

db/migrate/20241225031242_populate_country_band_details.rb Outdated Show resolved Hide resolved

lib/dues_calculator.rb Outdated Show resolved Hide resolved

lib/dues_calculator.rb Outdated Show resolved Hide resolved

danieljames-dj force-pushed the country_band_details branch 4 times, most recently from 91c0294 to e28d6cc Compare January 9, 2025 12:23

danieljames-dj requested a review from gregorbg January 14, 2025 12:17

gregorbg requested changes Jan 14, 2025

View reviewed changes

danieljames-dj force-pushed the country_band_details branch from a47e47d to c8162fe Compare January 29, 2025 14:50

danieljames-dj added 3 commits January 30, 2025 08:35

Store country band details in database

69a5d1b

Review changes

a51ced8

Review changes

47591c4

danieljames-dj force-pushed the country_band_details branch from c8162fe to 47591c4 Compare January 30, 2025 03:05

gregorbg requested changes Feb 5, 2025

View reviewed changes

Review changes

ad860a9

danieljames-dj commented Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store country band details in database #10476

Store country band details in database #10476

danieljames-dj commented Dec 25, 2024

gregorbg Jan 14, 2025

danieljames-dj Jan 29, 2025

gregorbg Jan 14, 2025

danieljames-dj Jan 29, 2025

gregorbg Jan 14, 2025

danieljames-dj Jan 29, 2025

gregorbg Jan 14, 2025

danieljames-dj Jan 29, 2025

danieljames-dj Jan 29, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

gregorbg Feb 5, 2025

gregorbg Feb 5, 2025

danieljames-dj Feb 6, 2025

danieljames-dj left a comment

danieljames-dj Feb 6, 2025

danieljames-dj Feb 6, 2025

danieljames-dj Feb 6, 2025

danieljames-dj Feb 6, 2025

danieljames-dj Feb 6, 2025

		country_band = CountryBand.find_by(iso2: country_iso2)&.number
		country_band_detail = CountryBandDetail.find_by(number: country_band)

		belongs_to :country, foreign_key: :iso2, primary_key: :iso2
		belongs_to :country_band_detail, foreign_key: :number, primary_key: :number

		@@ -0,0 +1,6 @@
		# frozen_string_literal: true

		class CountryBandDetail < ApplicationRecord

Store country band details in database #10476

Are you sure you want to change the base?

Store country band details in database #10476

Conversation

danieljames-dj commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljames-dj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment