URGENT Crash Fix: Fixed crash caused by Facebook Beta Shopify Connector

[kcharwood]

We are part of Facebook/Instagram's beta program for their connectors into Shopify. On Friday, part of our Facebook/Shopify integration was upgraded to their latest codebase. In this codebase, Facebook began sending null receipt data for all Shopify transaction objects. Note they weren't excluding the receipt key, but explicitly setting it to null

...
'receipt': null
...

This behavior led to a crash inside the canonicalize method in the Stitch Shopify connector, which has led our entire Shopify connector to be down since late Friday night in a crash loop.

Description of change

The change here simply safely handles the case where the receipt could be null. In the past, the codebase assumed at a minimum that an empty ({}) dictionary would exist for this key, or that the receipt key wouldn't exist at all, and was crashing when trying to access a key within the receipt when the receipt was null. Now, there will be no operations when the receipt is null.

I have added a test case explicitly showing the crash. If you run the test without my patch, you will see the crash. Running the test with my patch yields no crash.

QA steps

• automated tests passing

Risks

Minimum. Our entire reporting arm is down in our business until this is resolved, so urgency is extremely high for me to get this deployed today.

Rollback steps

• revert this branch

Test

I created a test to demonstrate the issue. When running the test without my patch, it causes a crash. Running with my patch passes as expected.

    def test_null_receipt_record(self):
        record = {"receipt": None}
        expected_record = {"receipt": None}
        canonicalize(record, "foo")
        self.assertEqual(record, expected_record)

Test without my patch:

nosetests tests/unittests
.EINFO Transaction (id=2) contains a receipt that has `foo` and `Foo` keys with the same value. Removing the `Foo` key.
....
======================================================================
ERROR: test_null_receipt_record (test_transaction_canonicalize.TestTransactionCanonicalize)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kevin/git/tap-shopify/tests/unittests/test_transaction_canonicalize.py", line 28, in test_null_receipt_record
    canonicalize(record, "foo")
  File "/Users/kevin/git/tap-shopify/tap_shopify/streams/transactions.py", line 26, in canonicalize
    value_lower = transaction_dict.get('receipt', {}).get(field_name)
AttributeError: 'NoneType' object has no attribute 'get'

----------------------------------------------------------------------
Ran 6 tests in 0.322s

FAILED (errors=1)

Test with my patch:

nosetests tests/unittests
..INFO Transaction (id=2) contains a receipt that has `foo` and `Foo` keys with the same value. Removing the `Foo` key.
....
----------------------------------------------------------------------
Ran 6 tests in 0.298s

OK

[cmerrick]

Hi @kcharwood, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

[cmerrick]

You did it @kcharwood!

Thank you for signing the Singer Contribution License Agreement.

[kcharwood]

This is really the only change in this PR. All the code below this is simply whitespace change from indenting the code under this if receipt check.

[kcharwood]

Running this code against current master branch will yield a crash. Running it against this PR yields a success.

[RachitKataria]

Hi Singer Team, Rachit here from the Facebook Shops team. I've been working directly with Kevin on this issue and can confirm that this is a blocker to their current Shopify orders integration with us. They're in a pretty bad state right now so the team and I would really appreciate it if you could take a look at this PR request ASAP to resolve Kevin's issues.

cc @cmerrick

[KAllan357]

This looks good. Thank you for letting us know.