-
Notifications
You must be signed in to change notification settings - Fork 0
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
Zach/support built in logical types #22
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:
|
@@ -157,7 +170,8 @@ | |||
first_name: 'John', | |||
last_name: 'Max', | |||
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
@@ -31,7 +32,7 @@ | |||
end | |||
|
|||
let(:developer_attrs) do | |||
employee_attrs.merge language: 'Haskell' | |||
employee_attrs.merge language: 'Haskell', password: 'password123!' |
Check failure
Code scanning / CodeQL
Hard-coded credentials
end | ||
|
||
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:
decimal
which requiresprecision
andscale
arguments. It adds this support in a future-proof way. See the README change in this PR for example usage.date
,datetime
, ortime
would 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_models
are 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
:currency
data type. I assume this data type was created to get around using thedecimal
logical type, which is supported everywhere. To support:currency
the 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_s
on those values impractical. E.g. if I use FSAS and call.errors.full_messages
I might get something like "Amount must be greater than 25.76". But if I make that same.errors.full_messages
call on a model not using FSAS, I will get something like "Amount must be greater than 2576".The solution here is to ditch
:currency
and just use the built-in avro type that is meant for decimal encoding.