smartsendio / woocommerce

Smart Send module for WooCommerce
GNU General Public License v3.0
0 stars 2 forks source link

Add new order statuses and new label logic #7

Closed bilfeldt closed 5 years ago

bilfeldt commented 5 years ago

We would very much like to implement more automation into the plugin and this issue is meant to make that possible to implement in the future.

Statuses

The idea is that a WooCommerce order already can have the following statuses:

We are missing some steps between the Processing and Completed steps. In order to deal with fullfillment then we would like to add these statuses:

We found inspiration for this from how Shopify does the fulfillment handling - see the field status

Manual handling of single order

Manual handling of a single order - when we are clicking Generate label from the Order View page. Here we skip the SmartSend-Pending status and instantly make an instant processing request. The immidiate response is either success or failure and hence the status is changed to SmartSend-Completed or SmartSend-Error

Manual handling of less than five orders

This is when we select Generate label from the Order list page while having selected maximum 5 orders. Here we loop over the orders and process them immidiatly like in the Manual handling of single order point above.

Manual handling of more than five orders

This is when we select Generate label from the Order list page while having selected more than 5 orders. Here we make an API call containing all orders. We will get an instance response with a status for each order. We then loop over the orders and update them to either SmartSend-Processing (we succesfully queued the order for handling) or SmartSend-Error (something failed our validation for the order).

Our system will then handle the orders and make API requests to WooCommerce for each order once the order is handled. The order will be updated to either SmartSend-Completed or SmartSend-Failed depending on the result.

Question: How should we save information about the error that occured?

Automated handling

Automated handling is when the setup some kind of trigger which will automatically queue the order for handling. It might for example be a rule that once the order gets the status Processing then the order should be handled. Such rules can either be added to our plugin or they can be done by the webshop owner himself.

When the order status is changed to SmartSend-Pending then it's our cue to start the processing. We need to make an API request so that Smart Send can start handling the request. It would be best if the request could be async so that we do not slow down other processes. There are some WooCommerce classes for that.

Challenge

If WooCommerce has some good way of handling queues then it would make sense to simple add a WooCommerce queue job to make an API request to Smart Send and then handle the response right away. So making a request like the one in section Manual handling of single order

If WooCommerce does not have a good way of handling queues, then it would make sense to instantly when something is triggered make an API call to Smart Send but an async API call meaning an API call saying "please handle this order soon, but not now". That way the API call should be fast and Smart Send will then make an API call to WooCommerce once the order is handled.

Basically the question is: Should the queue handling be done at WooCommerce or at Smart Send?

shadimanna commented 5 years ago

It might be best to have a call to discuss this task to make sure it's clear.

bilfeldt commented 5 years ago

@shadimanna Okay. When would be a suitable time for you?

shadimanna commented 5 years ago

How about Thursday at 10am?

bilfeldt commented 5 years ago

@shadimanna Tomorrow at 10am CET is a deal.

bilfeldt commented 5 years ago

One important thing I forgot: If we use WP Cron then we are using the same endpoint as for single label generation. This means that we cannot quite so easily limit the feature to customers on a pro subscription. This is a rather big disadvantage.

WP Cron Smart Send Queue
Require work on WooCommerce yes yes
Require work on Smart Send no yes
Can be limited to Pro subscription no yes
Require WooCommerce custom hook no yes
Distribute load on Smart Send server no yes
bilfeldt commented 5 years ago

After having looked more into the pros and cons of handling queue on either WooCommerce or Smart Send, then I have decided that we should look more into queueing on Smart Send.

Process for queuing orders

  1. Mark X orders
  2. Press "Generate labels"
  3. Make a Shipment obejct for each order
  4. Make an API request to Smart Send containing: A. An array of X shipment objects B. Callback url for success and errors

The response will contain a reply for each order - either success or error. For each order then we should do either of the following depending on the result of that order:

Successful response

A. Add the shipment_id from the response to the _ss_shipping_shipment_id meta field B. Update the order status to Smart Send Queue C. Fire evet smart_send_shipping_label_queued

Failed response

A. Update the order status to whatever is set in the setting Change status to X if label generation fails B. Add error as order comment C. Fire evet smart_send_shipping_label_failed

Webhook

Then we need to implement some methods that can handle a webhook call to the callback url and then update the order accordingly:

Validation

  1. Find the order from the shipment_internal_id from the webhook payload
  2. Check that the order has a meta-field _ss_shipping_shipment_id (NEW FIELD) matching the value of the shipment_id from the webhook payload

Handling successful webhook

This webhook will receive a payload identical to the response received when creating a single label through API (as we do now).

We should apply the same logic as implemented in create_label_for_single_order(). We need to refactor the code, so that we are not implementing the same code twice.

  1. Save the new the shipment_id from the webhook payload in the meta field _ss_shipping_label_id
  2. Save label locally if the setting is enabled
  3. Save tracking_number in Shipment tracking
  4. Save tracking_number and PDF link as order comment
  5. Change status to whatever is set in the setting Change status to X when a label is generated
  6. Change order status if the setting is enabled (example: set the status to completed)
  7. do_action smart_send_shipping_label_created

Handling failed webhook

This webhook will receive a payload identical to the error response received when creating a single label through API (as we do now) and an error occurred

We should do exactly the same as in the section Failed response above

shadimanna commented 5 years ago

We can definitely add a call back URL which can handle the response from your server, with get parameters.

shadimanna commented 5 years ago

I will do some further thinking regarding Smart Sending handling the queueing and what is involved on my end and get back to you beginning of next week.

shadimanna commented 5 years ago

I have reviewed the requirements and have a good idea of effort now. Let me know how we should proceed.

bilfeldt commented 5 years ago

@shadimanna Please post your proposed approach here, so that we can discuss further.

Once we agree how to do this, then I would like a quote from you (how many hours for each task, hourly rate and estimated delivery date).

Thanks, I look forward to hearing from you.

shadimanna commented 5 years ago

The assumption is we are going with Smart Send Queue.

Let me know if anything was missed.

bilfeldt commented 5 years ago

I think this is a really good approach

I have updated my first description of the project here can you please read that and let me know if you agree with the process described there?

bilfeldt commented 5 years ago

Here is inspiration for creating a custom REST API endpoint:

shadimanna commented 5 years ago

Can you please let me know which new functions were created to support the queueing of orders via the API?

shadimanna commented 5 years ago

Also, I don't recall if this new feature replaces the "Bulk" actions on the orders page or is in addition to it? If it replaces is it, then how do you anticipate we will create the combined labels PDF file?

bilfeldt commented 5 years ago

For bulk order handling then we will use the logic:

For less than 6 orders

We loop over each order and make an API call for each. We will handle each API call like described in Handling successful webhook above Then once all 5 API calls a made, then we use the newly saved shipment_id's to make an API call to combine the PDF label.

For more than 5 orders

We use the new queuing functionality.

Does it make sense? I will make a new mockup function, which you can use for testing the queued label endpoint.

shadimanna commented 5 years ago

Sounds good.

Let me know when you have the new function for testing in the SDK.

bilfeldt commented 5 years ago

I have created a new method in the API:

SS_SHIPPING_WC()->get_api_handle()->createShipmentAndLabelsAsync(array($this->shipment), 'https://example.com');

You will find this on the branch feature-async-labels. As you can see, then you can toggle a successful or a failing response on line 397 of smart-send-logistics/includes/lib/Smartsend/Client.php

$SIMULATE_SUCCESS = true;// TRUE: simulate successful api call, FALSE: simulate error

This mock up the request. We also need to make a mockup API callback. I will make that as a simply cURL php script which you can run. This will call a given URL with either a success or an error.

shadimanna commented 5 years ago

Has the mockup API callback been created?

Also, can we ensure that the returned array to the callback has the same structure as a single call i.e. "createShipmentAndLabels"?

bilfeldt commented 5 years ago

Hi Shadi

The cURL example can be found below. There is an example of a shipment where label generation failed and one where label generation passed. As you can see, then the format is idential to the one you would get using createShipmentAndLabels() for a single shipment.

Successful label generation

$endpoint = 'http://example.com/api/smartsend';
$pdf_link = 'https://smartsend.dk/files/Label-PostNord-Test.pdf';
$pdf_base64 = base64_encode(file_get_contents($pdf_link));
$payload = [
    "data" => [
        "type" => "label",
        "shipment_id" => "ad4fe940-5c3b-11e9-907e-7de39f1ae779",
        "shipment_internal_id" => "19990101120035",
        "shipment_internal_reference" => "shipment-0123456-A",
        "carrier_code" => "gls",
        "carrier_name" => "GLS",
        "pdf" => [
            "link" => $pdf_link,
            "base_64_encoded" => $pdf_base64,
        ],
        "parcels" => [
            [
                "parcel_id" => "ad5cef40-5c3b-11e9-962a-d9cfc896df47",
                "parcel_internal_id" => "00100025556",
                "parcel_internal_reference" => "ABC12345678",
                "carrier_code" => "gls",
                "carrier_name" => "GLS",
                "tracking_code" => "DEMO989687279826727973",
                "tracking_link" => "https://staging.smartsend.io/track/gls/DEMO989687279826727973"
            ],
            [
                "parcel_id" => "ad6cea50-5c3b-11e9-a177-77ae8aa30f74",
                "parcel_internal_id" => "00100025556",
                "parcel_internal_reference" => "ABC12345678",
                "carrier_code" => "gls",
                "carrier_name" => "GLS",
                "tracking_code" => "DEMO596108818371417370",
                "tracking_link" => "https://staging.smartsend.io/track/gls/DEMO596108818371417370"
            ],
        ]
    ]
];

// Make request
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $endpoint);
curl_setopt($ch, CURLOPT_HTTPHEADER, array(
    'Accept: application/json',
    'Content-Type: application/json',
));
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($ch, CURLINFO_HEADER_OUT, true);
curl_setopt($ch, CURLOPT_POST, true);
curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode($payload));
$response = curl_exec($ch);
curl_close($ch);

// Request
echo "<pre>";
print_r(json_encode($payload));
echo "</pre>";

// Response
echo "<pre>";
print_r($response);
echo "</pre>";

Failing label generation

$endpoint = 'http://example.com/api/smartsend';
$payload = [
    "message" => "The given data was invalid.",
    "errors" => [
        "shipping_date" => [
            "validation.after_or_equal",
        ],
    ],
    "links" => [
        "about" => "https://smartsend.io/errors#ValidationException",
        "status" => "http://status.smartsend.io/",
    ],
    "id" => "UUID-1234-5678-8900",
    "code" => "ValidationException",
];

// Make request
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $endpoint);
curl_setopt($ch, CURLOPT_HTTPHEADER, array(
    'Accept: application/json',
    'Content-Type: application/json',
));
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($ch, CURLINFO_HEADER_OUT, true);
curl_setopt($ch, CURLOPT_POST, true);
curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode($payload));
$response = curl_exec($ch);
curl_close($ch);

// Request
echo "<pre>";
print_r(json_encode($payload));
echo "</pre>";

// Response
echo "<pre>";
print_r($response);
echo "</pre>";
shadimanna commented 5 years ago

So will the endpoint be called for each individual shipment in the queue or will there be an array returned with success and failures as previously mentioned?

bilfeldt commented 5 years ago

It will be called for each individual shipment. So posting three shipments A, B and C will return in 3 API calls to Wordpress:

shadimanna commented 5 years ago

Alright, that makes sense.

shadimanna commented 5 years ago

I have been researching the POST callback approach and I do not believe it is possible to use POST to send the data via a callback URL, we can only use GET. Due to this, I believe we have two options:

  1. Use the WooCommerce API; Each merchant would need to set the credentials on their Smart Send account, which would then be used by Smart Send to POST the queue response as per your examples. We would need to create a custom endpoint for this as well.
  2. Use the GET callback; This would simply send the "order_key" to a custom callback URL via GET parameter, which would then trigger an API call to Smart Send that checks the status of this order and updates it accordingly. We would NOT need a custom endpoint or for the merchant to configure anything in WooCommerce or the Smart send account.

Let me know if this makes sense or if there are questions on it.

bilfeldt commented 5 years ago

I understand completely.

Solution 1 is not an ideal solution as it will require more complexity on the central system (authentication will need to be saved and used in the callbacks).

Solution 2 might be the best option. We could simply support two different types of callbacks:

Why is POST not an option in WooCommerce/Wordpress

Can you please describe why a POST request is not possible? I would prefer this solution but it might be an option to go with a GET request.

shadimanna commented 5 years ago

This is the only WC Callback option I know and it is a GET option - https://docs.woocommerce.com/document/wc_api-the-woocommerce-api-callback/. To POST you would need authentication which would take us back to "Solution 1".

bilfeldt commented 5 years ago

I just had a look and actually, I really like the idea of only allowing external calls to our plugin saying

Please check the results for shipment XXX which have just been processed

Instead of saying

Here are the info about shipment XXX

It will be good for security as nobody can post anything to the plugin. The plugin will always take the information from us.

We need to update the createShipmentAndLabelsAsync method then:

public function createShipmentAndLabelsAsync($shipments, $ping_url)
{
    $data = array(
        'notification' => array(
            'ping' => array(
                'url' => $ping_url,//this should include a :shipmentId tag which will be replaced by the id of the shipment. Example: https://example.com/api/:shipmentId or https://example.com/api?shipmentId=:shipmentId
            ),
        ),
        'shipments' => $shipments,
    );
    return $this->httpPost('shipments/labels/async', array(), array(), $data);
}

I just renamed from callback to ping as we might also implement a callback method later on.

shadimanna commented 5 years ago

OK, but it is best to use the "ping" URL with the "order_key" or "order_id" otherwise I would need to loop all orders to find the "shipment_id".

Also, can you create new APIs to check the status of a shipment on the Smart Send server, that returns the same structure you created?

bilfeldt commented 5 years ago

Do you need to loop all orders? Can't you find the meta field with that value and then use that to find the order?

Otherwise, then use a $ping_url like this:

$ping_url = 'https://example.com/api?order_id=:internalId';

Then our system will make a GET request to `https://example.com/api?order_id=1234'

I will create another endpoint you can use for this, but for now, if you just make a new API call like you do when creating one shipment, then you will get the same response. So you can use that for mockup.

shadimanna commented 5 years ago

OK great. So I will assume you will ping WC one by one and the status will be checked this way by making a call e.g. "getShipmentStatus" which will return the same responses you sent above.

shadimanna commented 5 years ago

I have made a commit which I think creates the bulk of the work for the queue feature. Once we have the actual SDK working correctly I can test further.

The other point I have not coded is checking whether a client has a Pro account or not.

bilfeldt commented 5 years ago

@shadimanna what branch have you merged the changes to, and have you implemented a new method which you call to get the Shipment Status?

p.s. If you reference the issue in the Git commit using a # then we can follow the development in this issue. Something like this:

Updating XXX #7

shadimanna commented 5 years ago

I added this to the "feature-async-label" you created and did add #7 on the initial commit.

I added a function called a function "getShipmentStatus" but placed in a comment since it would need to be added in the SDK.

bilfeldt commented 5 years ago

Okay, interesting that it's not mentioned in here then.

So what you need from me to continue is adding a method which returns a mockup of a success/error here?

//smart-send-logistics/includes/class-ss-shipping-wc-order-bulk.php
//Line 470-471

// MAKE API CALL TO GET STATUS
$response = SS_SHIPPING_WC()->get_api_handle()->getShipmentStatus( $order_id )
shadimanna commented 5 years ago

Yes, plus some way to test if the user has a pro account or not.

When do you plan to develop the actual functionality?

bilfeldt commented 5 years ago

You will not need to test the pro account feature. That is already tested when you post an array of shipments to the server. If the user is not on a pro subscription then they will get a HTTP 403 error.

The actual functionality will not be implemented right away, so we need to make this work based on mocked-up responses.

I will remove the old mockup method and implement this new mockup method for you to test. What is missing once this is implemented?

shadimanna commented 5 years ago

It is not clear how to check for the 403 error, is there a function to get the HTTP status returned or should I use "isSuccessful()" etc.?

Once we have the new mockup method, we can try testing the new code with more than 5 orders selected in the bulk actions page.

bilfeldt commented 5 years ago

You will get exactly the same response as you would if you try to make a single label with an invalid API Token.

I am almost done with the mockup, so you should have something to test with tomorrow.

bilfeldt commented 5 years ago

I have now implemented two mockup methods in smart-send-logistics/includes/lib/Smartsend/Api.php:

You will see in each of these methods that you can uncomment a line this will get that specific result.

For this to work, then you should change the endpoint using the following filter

function smart_send_api_endpoint_callback( $endpoint ) {
    return 'https://smartsendio.proxy.beeceptor.com/api/v1/';
}
add_filter( 'smart_send_api_endpoint', 'smart_send_api_endpoint_callback' );

Can you please let me know, if you are able to use this mockup?

shadimanna commented 5 years ago

When "createShipmentAndLabelsAsync()" is called now it returns an error - https://www.dropbox.com/s/xbvbhog8qlgvgt0/Screenshot%202019-04-19%2011.23.36.png?dl=0

shadimanna commented 5 years ago

Also, I wanted to note that when the callback is made it should include the "order_id" and whether the call is for a "return" label or not. Currently I only check if "$_GET['return']" is set or not.

bilfeldt commented 5 years ago

There were a bug in the endpoint name I can see. I just pushed a change. Can yoy please try again now?

The callback will include the order_id if you put the placeholder :internal_id into the callback url somewhere - example: https://example.com/webhook/order?order_id=:internal_id.

Whether or not is's a return has to be determined by the shipping_method field which will always start with "return" for return shipments.

shadimanna commented 5 years ago

But I have to know whether to get the "shipment_id" for "order_id" of a return label or a normal one, so this has to be included in the callback request, otherwise how would I know which one it is?

bilfeldt commented 5 years ago

You can ude the pladeholder ‘id’ which is the smipment_id saved in the meta fields. Thar Way you her both the id of the order and the shipment.

fre. 19. apr. 2019 kl. 12.28 skrev Shadi Manna notifications@github.com:

But I have to know whether to get the "shipment_id" for "order_id" of a return label or a normal one, so this has to be included in the callback request, otherwise how would I know which one it is?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/smartsendio/woocommerce/issues/7#issuecomment-484844074, or mute the thread https://github.com/notifications/unsubscribe-auth/AHGUCR4V72T55EUEY5JSZ6DPRGNENANCNFSM4G3QGVGQ .

shadimanna commented 5 years ago

Your message wasn't clear can you explain again?

In terms of the shipment data, what I send is both label and return label to the queue so I was thinking the same would be returned one by one. Does this not make sense?

bilfeldt commented 5 years ago

If you use a callback link with the two placeholders id and inernal_id as so, then it will work:

$pring_url = 'https://example.com/webhook/shipment?order_id=:internal_id&shipment_id=:id'
//Example ping: GET https://example.com/webhook/shipment?order_id=12345&shipment_id=a9b7755e-a209-4859-b4b1-102c1c6635c6

You simply use the $_GET['order_id'] to find the correct order and you check if it has a meta field _ss_shipping_shipment_id or _ss_shipping_return_shipment_id which matched $_GET['shipment_id']

Does it make sense?

shadimanna commented 5 years ago

Yes, that should work.

shadimanna commented 5 years ago

But I do not believe that the "shipment_id" is saved in post meta when creating the queue payload, it is only saved AFTER the label is created, isn't that so?

bilfeldt commented 5 years ago

No we are both changing order status and saving the shipment id to meta fields as soon as the asynchronous request is accepted by our system. See the process here

shadimanna commented 5 years ago

OK that make sense now, I make the "createShipmentAndLabelsAsync" and save the returned "ship_id" for label and return label then, right?