source-shopify: Inconsistent Comparison Fix

[ahmed-buksh]

What

Shopify historic sync fails due to inconsistent comparisons of none to str
Issue 11976

How

Get updated to return instead of default None

Recommended reading order

1. airbyte-integrations/connectors/source-shopify/source_shopify/source.py

🚨 User Impact 🚨

No breaking changes.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• If new credentials are required for use in CI, add them to GSM. Instructions.
• /test connector=connectors/<name> command is passing
• New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

[marcosmarxm]

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2397893602
❌ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/2397893602
🐛 https://gradle.com/s/qnsejjwmzleb4

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
================== 1 failed, 39 passed in 3470.15s (0:57:50) ===================

[alafanechere]

Thank you @ahmed-buksh for the contribution. I allowed myself to make a change in the comparison logic. I think record.get(self.cursor_field) returns ISO strings that can be compared. To make it more explicit I setting the default value to the epoch time.

[alafanechere]

@ahmed-buksh could you share the output of the acceptance test run (with a screenshot)? I think our shopify sandbox account is missing a validation on shopify side to properly run the tests (#11334) which is why the build of this connector is broken on master. I'll publish and merge this fix if you share a proof that the acceptance tests are passing 🙏

[alafanechere]

To run the acceptance tests you need to create a config.json file with your shopify config in airbyte-integrations/connectors/source-shopify/secrets/config.json and then run the acceptance test from the root of the repo with ./gradlew :airbyte-integrations:connectors:source-shopify:integrationTest

[alafanechere]

@ahmed-buksh could you please give an update about your test run? 🙏

[ahmed-buksh]

@alafanechere just pulling the updated master and running the tests, will update you in a while.

[ahmed-buksh]

@alafanechere I am facing the failure of acceptance test due to some issues with payment methods of my store. I start getting 404 on balance and transaction stuff and further findings on it revealed that one found this issue due to unavailability of payment methods in that region.

[alafanechere]

@ahmed-buksh are these errors happening during the connection check or on read? Feel free to share the test output.

[ahmed-buksh]

@alafanechere attached are the screenshots of tests failing on read.

[alafanechere]

@ahmed-buksh Could you please attach your full test logs output?

[ahmed-buksh]

@alafanechere
test_output.log

[alafanechere]

@ahmed-buksh I'm not able to run tests, for the reason I explained above.
I checked you changes and I think they are not appropriate for all streams. Some streams are using cursors that are not timestamps but id (Collects, BalanceTransactions, OrderRisks), which will make the comparison you created not work appropriately :)
Could you please override the filter_records_newer_than_state on those streams and implement a valid comparison?

[alafanechere]

Hey @ahmed-buksh I'm closing this PR as we did not receive updates since my last change request. Feel free to open a new PR anytime with these changes!