Do not print the bill addr. name when there's none

[sauloperez]

What? Why?

The error

ActionView::Template::Error: undefined method `full_name' for nil:NilClass

happens a few times a day and raises exceptions we don't pay attention to. They add unnecessary noise that hides other more relevant issues.

This, however, is a symptom of a deeper data integrity problem that needs solving at some point. This is just a countermeasure.

What should we test?

Check that the invoice is correctly displayed when the billing address is set. Let me know when you are testing the opposite scenario and I'll manually remove the address. I don't know how else to reproduce it.

Release notes

Make the invoice handle the inconsistent scenario where the order has no billing address.

Changelog Category: Changed

[Matt-Yorkley]

It's pretty worrying that we have an order in production without a billing address. I wonder if this change will reduce visibility of some underlying issue. Maybe we could add a quick Bugsnag call here in the case where there is no billing address?

[RachL]

I agree that currently it is not possible to order without a billing address. This is weird. @sauloperez do you know how many case we have in production?

[luisramos0]

this is empty in live uk, fr, au and katuma:
select * from spree_orders where bill_address_id is null and state not in ('cart', 'address');

This looks like someone is trying invoice generation on an order that is not completed.
But the invoice action menu is explicitly checking for that:
links.concat(complete_order_links) if @order.complete?
🤷‍♂️

[luisramos0]

This is also empty on these 4 instances, I dont understand:
select count(*) from spree_orders o where state not in ('cart', 'address') and not exists (select 1 from spree_addresses a where a.id = o.bill_address_id);

[RachL]

btw, isn't this linked to #5405 ?

[sauloperez]

I checked in Katuma but approaching it differently

select *
  from spree_orders
  left join spree_addresses
    on spree_addresses.id = spree_orders.bill_address_id
 where state              = 'complete'
   and spree_addresses.id is null
 order by spree_orders.id desc limit 1;

but 0 results as well. Might something only affecting AU?

It must be this:

This looks like someone is trying invoice generation on an order that is not completed.

I also want to confirm whats the impact of the issue because I bet a single order causes the whole print in bulk to fail.

[sauloperez]

another example but now in orde_cycle_management report: https://app.bugsnag.com/open-food-france/open-food-france-prod/errors/5ee107d3dd8f6c0018b1c294?event_id=5ee107d3005b7d7f3d660000&i=sk&m=nw

[filipefurtad0]

Hi @sauloperez ,

I think this is possible to reproduce without manually removing the address from the DB. If one tries to print an order without a name/address, for example:

Bugnsag is triggered:

undefined method ``full_name' for nil:NilClass:
https://app.bugsnag.com/yaycode/openfoodnetwork/errors/5eb7b3af2d025e0017d4e8af?event_id=5ef3612d005b682b698e0000&i=sk&m=nw

So, I staged this PR in staging-AU (in this case, although I reproduced the scenario in three stagings, see https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1593007455366300) hoping to be able to "bulk-print" this order.

It triggered a similar error, but concerning the "full address":
undefined method `full_address' for nil:NilClass
https://app.bugsnag.com/yaycode/openfoodnetwork/errors/5ef367c30c9a67001715b9dc?event_id=5ef367c3005b7b5abac50000&i=sk&m=nw

Perhaps the next stage would be to remove the address from a healthy order, and see it bulk-printing still works - help needed here (no access to staging-au).

[filipefurtad0]

I tested this further on staging-fr and printed out the healthy order R527511207, which worked fine:

This is the order in the DB:

  id  |   number   | item_total | total |  state   | adjustment_total | user_id |         created_at         |         updated_at         |        completed_at        | bill_address_id | ship_address_id | payment_total | shipment_state | payment_state |               email               | special_instructions | distributor_id | currency | last_ip_address | order_cycle_id | customer_id | created_by_id 
------+------------+------------+-------+----------+------------------+---------+----------------------------+----------------------------+----------------------------+-----------------+-----------------+---------------+----------------+---------------+-----------------------------------+----------------------+----------------+----------+-----------------+----------------+-------------+---------------
 1198 | R527511207 |      20.00 | 21.00 | complete |             1.00 |      57 | 2020-06-08 15:42:08.362144 | 2020-06-15 20:49:26.080606 | 2020-06-15 20:49:25.619512 |            1066 |            1068 |         21.00 | ready          | paid          | [email protected] |                      |             42 | EUR      | 127.0.0.1       |             73 |          72 |            57

After that, deleted the bill adress ID with the command:
update spree_orders set bill_address_id = null where id = '1198';

Which seems to have worked (hooray!):

  id  |   number   | item_total | total |  state   | adjustment_total | user_id |         created_at         |         updated_at         |        completed_at        | bill_address_id | ship_address_id | payment_total | shipment_state | payment_state |               email               | special_instructions | distributor_id | currency | last_ip_address | order_cycle_id | customer_id | created_by_id 
------+------------+------------+-------+----------+------------------+---------+----------------------------+----------------------------+----------------------------+-----------------+-----------------+---------------+----------------+---------------+-----------------------------------+----------------------+----------------+----------+-----------------+----------------+-------------+---------------
 1198 | R527511207 |      20.00 | 21.00 | complete |             1.00 |      57 | 2020-06-08 15:42:08.362144 | 2020-06-15 20:49:26.080606 | 2020-06-15 20:49:25.619512 |                 |            1068 |         21.00 | ready          | paid          | [email protected] |                      |             42 | EUR      | 127.0.0.1       |             73 |          72 |            57

After that, I tried repeating the bulk print-job and got the expected error:

This looks related to the findings from @RachL while testing PR #5353.

After staging the PR in staging-fr, the result does not change:

Editing this (complete) order and printing the invoice generates an error 500:

Perhaps this PR needs a second look @sauloperez ? Should something else be tested here, for now?

[sauloperez]

Yes, I see the billing addres is being referenced a couple more times. Thanks for the testing @filipefurtad0 !

[sauloperez]

I fixed the missing parts and now I can confirm it works on my machine while before didn't @filipefurtad0

[filipefurtad0]

Hey @sauloperez ,

Re-staged the PR, and got the same results as before on order R527511207:

Bulk printing (this order), while on /admin/orders:

Editing the order and printing the order, i.e., the link:

https://staging.openfoodfrance.org/admin/orders/R527511207/print

triggers error 500, as before.

Moving back to In Dev.

[sauloperez]

It seems there's an issue the staging server and it has nothing to do with this PR. This is what I seen in the logs

I, [2020-07-01 10:49:43#19750]  INFO -- : Started GET "/admin/orders/R527511207/print" for 109.69.11.78 at 2020-07-01 10:49:43 +0200
I, [2020-07-01 10:49:43#19750]  INFO -- : Processing by Spree::Admin::OrdersController#print as HTML
I, [2020-07-01 10:49:43#19750]  INFO -- :   Parameters: {"id"=>"R527511207"}                                                                                                                    
I, [2020-07-01 10:49:43#19750]  INFO -- : Completed 500 Internal Server Error in 37ms  
I, [2020-07-01 10:49:43#19750]  INFO -- [Bugsnag]: Notifying https://notify.bugsnag.com of I18n::InvalidLocale                                                                                  
F, [2020-07-01 10:49:43#19750] FATAL -- :                                                       
I18n::InvalidLocale ("en_GB" is not a valid locale):                          
  app/helpers/i18n_helper.rb:14:in `set_locale'      

I'll see if I manage to fix it and back to you @filipefurtad0

[luisramos0]

that locale must have been active at some point and then removed.
I fixed it locally manually now:
update spree_users set locale = 'en_FR' where locale = 'en_GB';

[sauloperez]

did it actually update any row? because I was just chechking select id, email, locale from spree_users where locale not in ('fr', 'it', 'en_FR'); and got nothing. I assume it was you.

[sauloperez]

no sorry, that was meant for @luisramos0 . I'm redeploying this branch again now.

[filipefurtad0]

I deleted my comment, but was too late :-)

[sauloperez]

Pretty good testing @filipefurtad0 👏 . I totally missed that we have two invoice templates that can be configured from /admin/invoice_settings/edit. Now this invoice renders as well.

[luisramos0]

did it actually update any row? because I was just chechking select id, email, locale from spree_users where locale not in ('fr', 'it', 'en_FR'); and got nothing. I assume it was you.

yes Pau, 3 users had en_GB.

[filipefurtad0]

Thank you @sauloperez ! 🚀

Did you mean to move it to All the things? I would think this is ready to be tested now, right?

[sauloperez]

Oh yes, I don't know when I moved it back. I must have mixed it with another one.

[filipefurtad0]

Hey @sauloperez ,
this works now, no more error 422 on the console! 💪

Both print options - bulk or under /order//edit - now work and yield the respective invoices according to the set template:

Ready to go! 🎉

[sauloperez]

Oh, finally! thanks a lot for thorough testing @filipefurtad0 !