-
Notifications
You must be signed in to change notification settings - Fork 0
Zach/support built in logical types #22
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, I noticed this repo does not have all of the required topics applied to it. In order to organize repos at Acima, all repos need to have several topics that describe the nature of the repo. Please apply the topics as specified by the confluence article here: https://acima.atlassian.net/wiki/spaces/SEC/pages/313622545/Why+did+I+get+comments+on+my+PR You are missing the following topics:
|
| title: 'VP of Engineering', | ||
| language: 'Haskell' | ||
| language: 'Haskell', | ||
| password: 'password123!' |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| { first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby' }, | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' } | ||
| { first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby', password: 'rubyroxx' }, | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' } |
Check failure
Code scanning / CodeQL
Hard-coded credentials
|
|
||
| let(:developer_attrs) do | ||
| employee_attrs.merge language: 'Haskell' | ||
| employee_attrs.merge language: 'Haskell', password: 'password123!' |
Check failure
Code scanning / CodeQL
Hard-coded credentials
|
|
||
| let(:dev2_attrs) do | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' } | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' } |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| { first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby' }, | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' } | ||
| { first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby', password: 'rubyroxx' }, | ||
| { first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' } |
Check failure
Code scanning / CodeQL
Hard-coded credentials
|
The history of using currency was that decimal was not available on the ruby side whent he code was built. So I added |
|
One change I'm seeing that I didn't expect, and could break things, is changing |
a86c2a5 to
8047df4
Compare
This PR:
decimalwhich requiresprecisionandscalearguments. It adds this support in a future-proof way. See the README change in this PR for example usage.date,datetime, ortimewould need to be updated like thisrequired :created_at, :time, avro: { logical_type: 'timestamp-millis' }. Because of this, I bumped the major version of the gem. It is probably possible to make this change in a backward compatible way. Let me know if that's desirable and I can do it. I made a list of affected projects at Acima by doing a code search. Each project just has a handful of updates it would have to make.contract-funding,lms,virtual-bank, andonboarding_modelsare affected.Reasoning for this PR
I'm working on removing the field_struct_avro_schema dependency from merchant-portal. The only reason it depends on FSAS is because of onboarding_models. The onboarding_models gem does have important data validations in it, which are active model compatible, so the idea is to split those validation functions into modules that can be included in both FSAS-based models and in anything else that supports ActiveModel, like avromatic.
During this work, I discovered the
:currencydata type. I assume this data type was created to get around using thedecimallogical type, which is supported everywhere. To support:currencythe FSAS gem must do a special pre-serialization and post-deserialization conversion process where it multiplies or divides the value currently in the model by 100. This makes the shared use of validations which want to callto_son those values impractical. E.g. if I use FSAS and call.errors.full_messagesI might get something like "Amount must be greater than 25.76". But if I make that same.errors.full_messagescall on a model not using FSAS, I will get something like "Amount must be greater than 2576".The solution here is to ditch
:currencyand just use the built-in avro type that is meant for decimal encoding.