Feature/ruby 2477 wcr tech upgrade to rails 7 #1389
Conversation
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
15c00ee to
55f4ffe
Compare
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
0ac6d2e to
9282f36
Compare
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
c6964bb to
9b9c69b
Compare
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
c98ebe8 to
cd0f568
Compare
…ion rather than transient registration after update_registration
| @@ -12,7 +12,12 @@ def merge_finance_details | |||
| registration.finance_details.orders << order | |||
| end | |||
|
|
|||
| transient_payments_array = [] | |||
There was a problem hiding this comment.
Why was it necessary to break this into two steps?
There was a problem hiding this comment.
The code used to be
transient_registration.finance_details.payments.each do |payment|
registration.finance_details.payments << payment
end
This is because the each loop was actually not working properly when called on transient_registration.finance_details.payments. The loop actually does not complete and I believe it's due to transient_registration.finance_details.payments being edited by the line inside the loop (payment can only belong to one payments collection at a time). The breaking up was a way to ensure each payment actually gets iterated on. I will add a comment to that effect
There was a problem hiding this comment.
But the line inside the loop in the previous code is acting on the registration, not on the transient_registration?
There was a problem hiding this comment.
Yes the line is acting on the registration, but if you move a payment from transient_registration.finance_details.payments into registration.finance_details.payments it has edited the transient_registration.finance_details.payments array during the iteration
There was a problem hiding this comment.
I don't see from the old code snippet how that could have happened. There is a loop over all the payments in the transient_registration. The body of the loop acts on the registration, not on the transient_registration. It's not moving the payment from the transient_registration to the registration, it's copying it.
There was a problem hiding this comment.
Based on the spec that I was trying to fix, it was not copying it, but moving it instead
app/services/waste_carriers_engine/notify/registration_confirmation_email_service.rb
Outdated
Show resolved
Hide resolved
| @@ -97,7 +97,7 @@ def extend_expiry_date | |||
| end | |||
|
|
|||
| def create_order_item_logs | |||
| OrderItemLog.create_from_registration(transient_registration) | |||
| OrderItemLog.create_from_registration(registration) | |||
There was a problem hiding this comment.
Shouldn't the order_item_log be created based on the new order dat in the transient_registration, not what was previously stored on the registration?
There was a problem hiding this comment.
So the data can be different? I added a spec to check if it would be the same
There was a problem hiding this comment.
Yes - the registration should have all order items since it was created, so for example the items from the original registration order; some copy cards ordered a few months later; and a renewal order. The transient registration should have only the order items from the current session. I'm fairly sure that copying from the registration (if it has previous order items) would result in duplication of the older ones.
| @@ -19,6 +18,6 @@ | |||
| business_type: business_type, | |||
| tier: tier, | |||
| key_people: key_people, | |||
| metaData: { route: route }) | |||
| metaData: { route: nil }) | |||
There was a problem hiding this comment.
Why was it necessary to change this to nil? All registrations should have a route.
There was a problem hiding this comment.
This is because this test is trying to test the metadata route being set, but that context is setting it in an implicit way that stops the test working
iteration when merging finance details - Return order item log to being created from a transient registration rather than a registration - Allow getting to registration from finance details of a transient registration - reload finance details so orders are present - Go back to using interim array rather than cloning for correcting issue with embedded document parent not being correct - Add interim array for duplication for orders that was present for payments - Add extra order to renewal_completion_service_spec to ensure it can handle multiple orders as well as multiple payments
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
b37ee68 to
67319d8
Compare
…into feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
jjromeo
force-pushed
the
feature/RUBY-2477-wcr-tech-upgrade-to-rails-7
branch
from
8125ad5 to
52db1b5
Compare
No description provided.