Correctly record last_ip_address on order #1658
Conversation
There was a problem hiding this comment.
I have a concern with this in which simple_current_order can now cause some very unexpected side effects as we're calling the update on the memoized object.
it 'is called twice' do
order1 = controller.simple_current_order
order1.email = "[email protected]"
order2 = controller.simple_current_order
expect(order2.email).to_not eq('[email protected]')
endThe second simple_current_order returns the same (modified) memoized object, which will call .update and persist that email change to the database.
Maybe we can look at shifting when we make the method call to update the current IP address to take all the modification out of this read method?
|
@cbrunsdon I added a spec that checks for persistence when |
|
@bbuchalter doesn't subject memoize as well? I don't believe your spec addition actually runs |
|
@bbuchalter hey, my apologies, the return in simple_current_order means if its memoized its only going to be executed once, so right you are. |
|
I have objections to the existence of |
|
@cbrunsdon would you like me to rebase the fixup or remove it? |
| @@ -21,10 +21,10 @@ def simple_current_order | |||
| @simple_current_order = find_order_by_token_or_user | |||
|
|
|||
| if @simple_current_order | |||
| @simple_current_order.last_ip_address = ip_address | |||
| @simple_current_order.update(last_ip_address: ip_address) | |||
There was a problem hiding this comment.
One though before merging: should this be update!? We don't check the result so it probably should.
There was a problem hiding this comment.
I agree this should be an update!. Also before merge, there is this question about my fixup commit in this PR: #1658 (comment)
There was a problem hiding this comment.
The test in the fixup looks good to me (it can't hurt to have). I'd like it rebased please :)
bbuchalter
force-pushed
the
save-ip-address
branch
from
88a6a59 to
59a4dc6
Compare
|
Done. |
|
Test fail does not seem legit. I can't seem to trigger a rebuild. Thoughts? |
|
I triggered a rebuild. Here's a link to the odd failure: https://circleci.com/gh/solidusio/solidus/4985 I was not able to reproduce it locally. |
|
Poke. |
The assignment of last_ip_address was not persisted where it was set. When this module is included in a custom storefront, the assignment of last_ip_address is lost.
bbuchalter
force-pushed
the
save-ip-address
branch
from
59a4dc6 to
1b99c9b
Compare
|
I have rebased this PR on master. Given that |
|
Thanks! |
The assignment of last_ip_address was not persisted where it was set.
When this module is included in a custom storefront, the assignment of
last_ip_address is lost. There was also a case in simple_current_order
where the last_ip_address was never assigned.