Add UUID to StoreCredit#generate_authorization_code #4060
Conversation
spaghetticode
force-pushed
the
spaghetticode/store-credit-authorization_code
branch
2 times, most recently
from
b6008c7 to
563da62
Compare
| @@ -150,7 +150,7 @@ def can_void?(payment) | |||
| end | |||
|
|
|||
| def generate_authorization_code | |||
| "#{id}-SC-#{Time.current.utc.strftime('%Y%m%d%H%M%S%6N')}" | |||
| "#{id}-SC-#{Time.current.utc.strftime('%Y%m%d%H%M%S%6N')}-#{SecureRandom.uuid}" | |||
There was a problem hiding this comment.
This has enough segments in it now that it might be nicer to write it more like this, but either way is fine by me.
| "#{id}-SC-#{Time.current.utc.strftime('%Y%m%d%H%M%S%6N')}-#{SecureRandom.uuid}" | |
| [ | |
| id, | |
| "SC", | |
| Time.current.utc.strftime('%Y%m%d%H%M%S%6N'), | |
| SecureRandom.uuid | |
| ].join("-") |
spaghetticode
force-pushed
the
spaghetticode/store-credit-authorization_code
branch
from
563da62 to
341bcdc
Compare
|
|
||
| describe "#generate_authorization_code" do | ||
| it "doesn't rely on time for uniqueness" do | ||
| expect(Time).to receive(:current).twice { Time.zone.parse("2019/09/05 06:30:00") } |
There was a problem hiding this comment.
Should we use the Rails helpers for stubbing time here, so the test is not so closely coupled with the implementation?
There was a problem hiding this comment.
I actually wanted to explicitly couple it with the implementation, because the problem is with it. The moment we stop using the timestamp, this specific spec implementation may not make much sense anymore (I mean the line after this one), surely it won't make sense its text description.
There was a problem hiding this comment.
What I meant is that there's a ton of different ways to get the current time in Ruby/Rails: Time.current, Time.now, Time.zone.now, DateTime.current, etc. In the test, we shouldn't worry about what method is being called exactly in the implementation.
A quick way to solve the problem would be to use the helpers Rails provides for stubbing time.
Does that make sense?
There was a problem hiding this comment.
Oh, sure! I think I knew about those helpers at a certain point in my life, but I forgot about them since then.
The value returned by `StoreCredit#generate_authorization_code` depends on time, which means that it may not be unique when a store credit is saved more than once at the same moment. This is not very likely to happen, but not impossible at all. By introducing a UUID into the code we can be sure that this code will end up being unique. The previous format of the code is retained, since there may be applications relying on it.
spaghetticode
force-pushed
the
spaghetticode/store-credit-authorization_code
branch
from
341bcdc to
a4a3b57
Compare
Fixes #4041
The value returned by
StoreCredit#generate_authorization_codedepends on time, which means that it may not be unique when a store credit is saved more than once at the same moment. This is not very likely to happen, but not impossible at all.By introducing a UUID into the code we can be sure that this code will end up being unique. The previous format of the code is retained, since there may be applications relying on it.
Checklist: