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

make monetized_attributes a hash_with_indifferent_access #430

Closed
wants to merge 1 commit into from

Conversation

bsharpe
Copy link

@bsharpe bsharpe commented Mar 16, 2016

monetized_attributes was getting created as a normal hash in one place and as a HWIA in another. By making it always a HWIA, we don't care if the accessed attr in validator is a symbol or a string.

We noticed this when running our tests -- validation was failing because @attr.to_s was being used to look up the monetized_attribute but the key was a symbol.

@LeKristapino
Copy link

No tests? :(

@bsharpe
Copy link
Author

bsharpe commented Mar 17, 2016

ok... tried to create tests... couldn't get it to fail.

Then realized it's because we had to create a completely new version of monetize that includes an exchange date (what is $100 CAD worth in USD a year ago.)

Our version of monetize didn't call track_monetized_attribute which does create a HashWithIndifferentAccess, instead relying on the other creation of the hash in monetized_attributes which does NOT create a HWIA.

So, the bug was ours after all, but I feel that both hashes should be created the same way (as a HWIA).

Also...the complexity of the monetize method made it very difficult to add an exchange_date parameter without copy/pasting the whole thing and changing just the bits we needed to.

@antstorm
Copy link
Contributor

@bsharpe yeah, I agree, monetize method has grown too complex, but there's some work around refactoring it — #425.

Re this PR — I'm happy to merge it, but it needs some tests to make sure others know that this is intentional.

Base automatically changed from master to main February 28, 2021 20:45
@semmons99 semmons99 closed this Mar 2, 2021
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.

None yet

4 participants