Create/Update products sync to facebook with Batch API #2672
Conversation
rahulraina7
changed the title
|
Thank you, @rahulraina7 for the PR. I'll do a review now. |
There was a problem hiding this comment.
Thank you, @rahulraina7, for your patience. The changes look good to me. I've tested the simple product, variable product creation and update. They work as expected. I also looked into the logs and saw items_batch being called instead of the Graph ID.
However, there is only one blocker related to the code: the usage of update_product_item within the update_fb_visibility method. Please verify if we should also switch this to the batch API call.
I've added some minor questions and suggestions. They are not blockers.
I've approved the PR in advance as it works as described. I'll trust you to resolve the blocker and merge the PR.
| @@ -1175,7 +1178,7 @@ public function on_simple_product_publish( $wp_id, $woo_product = null, &$parent | |||
|
|
|||
| if ( $fb_product_item_id ) { | |||
| $woo_product->fb_visibility = Products::is_product_visible( $woo_product->woo_product ); | |||
| $this->update_product_item( $woo_product, $fb_product_item_id ); | |||
| $this->update_product_item_batch_api( $woo_product, $fb_product_item_id ); | |||
There was a problem hiding this comment.
Are we replacing all calls to update_product_item with update_product_item_batch_api? In that case, we still have update_product_item call being made here: https://github.com/woocommerce/facebook-for-woocommerce/blob/b8c26af50a7655f30ce9cb8e45a2f4ec92fb9681/facebook-commerce.php#L2996
Can you please verify if this needs to be replaced?
There was a problem hiding this comment.
@ibndawood - thank you for pointing this out. I am of the opinion that lets leave this function as it is, as this is merely changing the visibility of the product. Even though we could possible migrate to the new API, but that prepares an entire product again with a new Woo Product class and makes the code a bit more verbose IMO, because the current way of preparing a product for sending batch updates sends the entire product, instead of just say the visibility in this case.
Let me know what you think ?
There was a problem hiding this comment.
I understand. We can proceed without changing this function.
Code format Co-authored-by: Kader Ibrahim S <[email protected]>
|
@rahulraina7 is this all or are you working on some things still? @ibndawood how does this look after the last updates? |
|
@budzanowski - Looks good to me. I've already approved. |
Hello @budzanowski - yes all is good, I am just waiting for the holiday period to be over. To have no surprise issues, I will merge this in first week of January. I am not sure of the release cycles of the package how frequent it is done. |
|
We release every week ( Tuesday/Wednesday ) if there is something to release. Please ping after the merge. |
ibndawood
added
the
changelog: add
ibndawood
added
changelog: none
Changes proposed in this Pull Request:
[1/2]
Currently the video syncing is only supported by batch-api / feed upload
This pull requests adds the following
flows are affected
Tests
Changelog entry