smartsendio / woocommerce

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

Change where to show the agent_no and how to change it #1

Closed bilfeldt closed 5 years ago

bilfeldt commented 5 years ago

The meta field ss_shipping_order_agent_no should be shown in the Smart Send box as text:

image

We remove the input box since the agent number is rarely changed and this will simplify the ajax call made when creating labels (right now it start by updating the agent number if need be). It is still possible to change the agent number from the Custom Fields section on the View Order page:

We need to listen for an update of this value. If the user tried to update the agent number, then we should use the API to get information about the provided agent and: image

bilfeldt commented 5 years ago

because we implemented this functionality + the functionality of sorting the items into parcels, then we have complicated everything.

Especially the method create_label_for_single_order() is overly complicated. It creates a variable $ss_args and applies a filter smart_send_shipping_label_args. But the problem is that it's then calling the method make_single_shipment_api_call which in turn is then calling make_single_shipment_api_payload. This method should use the data from the provided $ss_args, but it uses both values from that, and values taken directly from the order.

I suggest that we simply remove $ss_args and retrieve all necessary information directly from the make_single_shipment_api_payload method. Then we can add relevant filters to that method.

The only real change this will require is that if an item/parcels configuration is provided when making an API call, then the new configuration is saved as meta data first and the order is refreshed.

shadimanna commented 5 years ago

Regarding the last comment, I understand that the code isn't the "cleanest" however class "SS_Shipping_Shipment" is not aware of the class "SS_Shipping_WC_Order" so it cannot populate $ss_args accordingly.

bilfeldt commented 5 years ago

But if the class SS_Shipping_Shipment simply pulls everything directly from the WC Order object and applied the correct filters, then there is no info from SS_Shipping_WC_Order that must be retrieved from SS_Shipping_Shipment.

The only info that could get passed is the arrangement of items into parcels (this is a form on the order details page). Here we simply need to save it to the correct meta field before we build the payload using SS_Shipping_Shipment.

Does it make sense?

shadimanna commented 5 years ago

It is not as simple as reading meta information, there is logic involved within retrieving some of the fields that is abstracted within "SS_Shipping_WC_Order". I could pass the object to "SS_Shipping_Shipment", but I want to understand the motivation for this change. Is it to have one filter that can be overridden within "make_single_shipment_api_payload()" or multiple or something else?

bilfeldt commented 5 years ago

@shadimanna please explain what information you would like to pass from SS_Shipping_WC_Order to SS_Shipping_Shipment?

As I see it, then they should be completely decoupled and have the following capabilities:

SS_Shipping_WC_Order

Responsible for:

SS_Shipping_Shipment

This controller is used to:

bilfeldt commented 5 years ago

I have looked at the latest changes you have made, and I have a few questions:

  1. Why are we returning false here? This will also be returned when we were able to find the agent and update the meta field _ss_shipping_order_agent
  2. Minor cleanup needs to be done like:
  3. What is the difference between the methods save_meta_box and save_meta_box_ajax in smart-send-logistics/includes/class-ss-shipping-wc-order.php? They have very similar names, so maybe we need a rename here.
shadimanna commented 5 years ago

I have made another commit which addresses the mentioned issue above.

bilfeldt commented 5 years ago

It seems to me that 1 and 3 are done. But 2 is not done right?

bilfeldt commented 5 years ago

Testing result:

shadimanna commented 5 years ago

I can add the delete hook, but I don't see an easy way to add messages unfortunately. Since most people will not use this it seems like overkill for the time that will need to be invested.

bilfeldt commented 5 years ago

It seems that it's not possible to add an error message by default, so let us skip that - see docs.

We should function to delete the _ss_shipping_order_agent meta field when the ss_shipping_order_agent_no meta field is disabled.

I noticed that we are using a filter called update_post_metadata_by_mid but that does not seem to exist in the documentation. Why are we not just using update_post_metadata? See here.

shadimanna commented 5 years ago

I was not able to get it to work with "update_post_metadata".

I have committed the latest changes which includes "delete" and "add" meta, as well as docblock.