Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shopify integration #173

Open
wants to merge 83 commits into
base: master
Choose a base branch
from
Open

Conversation

bohdan-vorona
Copy link
Contributor

This branch is extended from #155

I tried to explain everything in the file -> intro-docs/ecommerce-platfroms.md

bohdan-vorona and others added 30 commits February 21, 2023 14:19
…hookProcessingJob, initial Jobs for Shopify webhooks processing
@bohdan-vorona bohdan-vorona self-assigned this Mar 16, 2023
@bohdan-vorona bohdan-vorona linked an issue Mar 16, 2023 that may be closed by this pull request
@cgsmith cgsmith added this to the MVP milestone Mar 20, 2023
@bohdan-vorona
Copy link
Contributor Author

@samdark Applied all the requests.

@bohdan-vorona bohdan-vorona requested a review from samdark March 22, 2023 15:34
/**
* Class m230221_134343_add_shopify_mock_ecommerce_platfort
*/
class m230221_134343_add_shopify_mock_ecommerce_platform extends Migration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file name should match the class name m230221_134343_add_shopify_mock_ecommerce_platform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgsmith done

Copy link
Contributor

@cgsmith cgsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending tests.

Copy link
Collaborator

@samdark samdark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK. I'd add explicit interfaces for further integrations but that likely could be done later.

@cgsmith
Copy link
Contributor

cgsmith commented Mar 23, 2023

Received this while testing.

image

Not sure if related to the app being a draft?
image

@cgsmith
Copy link
Contributor

cgsmith commented Mar 23, 2023

Might have been related because i was requesting from localhost:30000 and then it redirected to shipwise.ngrok.io - ignore for now.

@bohdan-vorona
Copy link
Contributor Author

@cgsmith For the moment, please try to use my app credentials:

    'shopify' => [
        /**
         * Visit https://partners.shopify.com/ -> Apps -> Your App -> Overview -> Client credentials
         */
        'client_id' => '8e317f58a84c1e14d07c64e6a8e4133d',
        'client_secret' => '4437437bb4f1eaad2ee8d339922d338d',
        /**
         * You can set `https://shipwise.ngrok.io` to test it locally.
         * Otherwise, if leave the default value, the current server's domain name will be used automatically.
         */
        'override_redirect_domain' => 'https://shipwise.ngrok.io',
    ],
I will write my app state a bit later.

@bohdan-vorona
Copy link
Contributor Author

@cgsmith

Might have been related because i was requesting from localhost:30000 and then it redirected to shipwise.ngrok.io - ignore for now.

Yes, probably this is the reason. Anyway my app state:

ship-1


ship-2


ship-3


ship-4

@cgsmith
Copy link
Contributor

cgsmith commented Mar 23, 2023

Ok I have that as well.

Questions/Changes

@bohdan-vorona

  • UUID is correct on the order but the order number should match the Shopify Name (usually starts with #1001 as an order number)
  • The ecommerce_order_log table is a good idea. This would be useful to have exposed under the ecommerce-integration URI
  • Does the webhook fire for order/refund?
    image

Can you explain why we are using a cron for creating the queued items? Is it better just to queue once a webhook is received?

It also looks like Shopify has some built in notifications for if webhooks are failing. This is good because not all ecommerce platforms have this. Do you think it is better to just handle webhooks received from Shopify synchronously? If we fail at processing Shopify will provide some insights into failing webhooks. @samdark or @cebe curious on your thoughts on this specifically. Here is an example of the Shopify app dashboard:

image

Testing so far and things that need to be tested/completed before a launch

  • Created test Shopify App
  • Setup redirects to ngrok for testing
  • Create Order with valid info on Shopify, will be created in Shipwise
  • Create order on Shopify with missing info, notification will be sent from Shipwise
  • Update order on Shopify (items), items will update on Shipwise (or failure notification)
  • Update order on Shopify (shipping address), address will update on Shipwise (or failure notification)
  • Refund order on Shopify, order will ??? on Shipwise (need to think on this one)
  • Create a second Shopify integration under the same customer (customers have more than one store)
  • Check that failure logs are notifying customer who setup integration properly (maybe add notification email on integration?)
  • Pause integration from Shipwise
    • Create order on Shopify - make sure it does not send a webhook (or Shipwise ignores)
  • Delete integration from Shipwise
    • Make sure app is deleted in Shopify site
  • Update ecommerce-platforms.md if needed
  • Create production Shopify App before launch

Feel free to add others! I'll keep this list updated.

@bohdan-vorona
Copy link
Contributor Author

@cgsmith Thanks for your response

UUID is correct on the order but the order number should match the Shopify Name (usually starts with #1001 as an order number)

Ok, I will change it.

The ecommerce_order_log table is a good idea. This would be useful to have exposed under the ecommerce-integration URI

Sorry I didn't understand this request) Did you mean to display the data on the page /ecommerce-integration on our website similar I did for Order History?

Does the webhook fire for order/refund?

I will recheck it.


Can you explain why we are using a cron for creating the queued items? Is it better just to queue once a webhook is received?

I added the logic for pulling the first initial orders from a store. Let's say we have connected a store and the store has 10 orders - these 10 orders will be pulled from Shopify in this case. Later, when a webhook is triggered (let's say order/edit), we will edit the already existing order in our system.

I can remove this logic and we can use only webhooks without initial orders pulling. Or just remove it from CRON but leave the logic itself for potential use in future. Let me know.

BTW, if yes ^, a question:

Let's imagine I've removed this initial pulling and there is an order in Shopify store. The webhook order/edit (or any other that is not order/created) is triggered, but we don't have the order in our system yet, shall I first create the order in our system? Or, in another case, shall I take into account only internal orders created by the webhook order/created?

(For myself: don't forget to update the docs)


A great list. I will go through it and maybe will add something.


What shall I do with these webhooks (see the list below)? Change the status or...? If change the status, then what accordance? Or just mark them as success and do nothing?

        'orders/fulfilled',
        'orders/partially_fulfilled',
        'orders/paid',

At the moment I just set them as successfully applied without any changes at our site.

@samdark
Copy link
Collaborator

samdark commented Mar 24, 2023

I can remove this logic and we can use only webhooks without initial orders pulling.

What's the reason for pulling data in advance?

BTW, if yes ^, a question

Pulling on demand seems to be enough.

@samdark
Copy link
Collaborator

samdark commented Mar 24, 2023

Do you think it is better to just handle webhooks received from Shopify synchronously?

What's an alterantive? Shopify expects an answer syncronously. If we'll record request and process it later via queue then in Shopify it will be marked as OK regardless of actual result on our side. I'd avoid that unless processing is heavy.

@bohdan-vorona
Copy link
Contributor Author

What's the reason for pulling data in advance?

@samdark In fact, there is no reason. It can be a way to get N last already existing orders on the Shopify side if we don't know anything about them via webhooks. I will remove it from cron.

@bohdan-vorona
Copy link
Contributor Author

@cgsmith

UUID is correct on the order but the order number should match the Shopify Name (usually starts with #1001 as an order number)

Done. But I remove the # symbol (Similar to https://github.com/CGSmith-LLC/shipwise-backend/blob/master/src/CGSMITH/Shopify.php). If you need the symbol back, let me know.

Does the webhook fire for order/refund?

Yes, it has refunds/create. I've implemented the job for this webhook. But for the moment the job does nothing, I just mark it as success. What shall I do with this webhook? (Also, see my other questions regarding some other webhooks I don't process above).

BYW, as I was testing the refunds, there can be two situations:

  • Order has several items and we make a refund for one item. In this case, the webhook orders/updated is sent to our site. The response has payment_status=partially_refunded. Also, it sends the list of refunded items. It means that we can handle partially refunded orders as well.
  • Order has one or more items and we make a refund for all the items. In this case, the webhook refunds/create is sent to our side. As I wrote, it's implemented via the mock job for the moment.

Pay attention. To make the webhook refunds/create work, you need to:

  • uninstall our App in your shop's settings
  • reconnect our App on our website

It's needed to update the webhook listeners list on Shopify side for your shop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shopify Integration [FEATURE]
3 participants