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

Store country band details in database #10476

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danieljames-dj
Copy link
Member

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.

@danieljames-dj danieljames-dj force-pushed the country_band_details branch 15 times, most recently from d02c82a to 9c50e1a Compare January 4, 2025 10:05
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 danieljames-dj force-pushed the country_band_details branch 2 times, most recently from bf747ae to b4fe1d2 Compare January 8, 2025 04:25
Comment on lines 22 to +23
country_band = CountryBand.find_by(iso2: country_iso2)&.number
country_band_detail = CountryBandDetail.find_by(number: country_band)
Copy link
Member

Choose a reason for hiding this comment

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

You're completely ignoring your new belongs_to association here.


[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")
Copy link
Member

Choose a reason for hiding this comment

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

Too much happening in one line

# 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")
Copy link
Member

Choose a reason for hiding this comment

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

Too much happening in one line

belongs_to :country, foreign_key: :iso2, primary_key: :iso2
belongs_to :country_band_detail, foreign_key: :number, primary_key: :number
Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants