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

New features #143

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

vidmantaskiro
Copy link
Contributor

No description provided.

@vidmantaskiro vidmantaskiro requested a review from a team as a code owner December 18, 2024 07:42
defined( 'ABSPATH' ) || die( 'no direct access' );

/**
* Omnisend Batch class. It should be used with Omnisend Client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Category class

defined( 'ABSPATH' ) || die( 'no direct access' );

/**
* Omnisend Batch class. It should be used with Omnisend Client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Product class

}

if ( $this->sku !== null && strlen( $this->sku ) > 100 ) {
$error->add( 'default_image_url', 'SKU must be under 100 characters' );
Copy link
Contributor

Choose a reason for hiding this comment

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

SKU, not default_image_url

}

if ( ! empty( $this->variants ) ) {
$arr['variants'] = $this->variants;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you use productVariant method to_array here? Also, product validation should validate variants and reuse productVariant validation, because I think you will be sending and creating products and variant is a child of a product and you dont want to call separate methods and deal with it

Or perhaps I am missing something here? How are you planing to use them from client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Products can have multiple variants, and if I use productVariant function "to_array" inside product, then I also need "save/reset" function for when I assign variant values to make sure no information is re-assigned from variant#2 to variant#1, if something like "StrikeThroughPrice" wasn't provided due to "if" condition.

If I pass an array of multiple variant classes to product class which then uses "to_array" in a loop it solves this issue, but something I'd prefer to avoid, because class should be initialized once within construct.

I think current solution could also use "save/reset" function that would be triggered, after using "to_array", to reset properties and avoid using within "cycle" $omnisend_product = new Product();

Also expected usage for variant would be:

foreach ($products as $product) {
$omnisend_product->set_sku($product->get_sku());
$omnisend_product->set_variants($this->get_variants($product->get_variants()));

if ( $omnisend_product->validate()->has_errors() ){
continue; // or break + log message
}

$batch[] = $omnisend_product->to_array();
}

function get_variants($variants){
foreach ($variants as $variant) {
$omnisend_variant->set_sku($variant->get_sku());
$omnisend_variant->set_something();

    if ($variant->special_price) {
       $variant->set_strike_through_price(..);
    }
    ... validate
    $batch[] = $omnisend_variant->to_array();

}

return $batch;
}

*
* @return void
*/
public function set_items( $items ): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

How are you planing to validate items? Because they can be product, category etc and as I understand batch methods will be used to create all of these items.
How would you create a batch of products and send them via this method? Would to_array generate correct request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Batch should be used with Product/Event/Category class - they return single item and user only needs to save this item into array and set it with set_items e.g.

$data[] = $product->to_array();

$data[] = $category->to_array();

$batch->set_items($data);

or I could implement previously suggested "save/reset", and handle "$data[] = $product->to_array()" for end user. I only need to know when user starts assigning another product values within loop.

Copy link
Collaborator

@nerijuszaniauskas nerijuszaniauskas Jan 8, 2025

Choose a reason for hiding this comment

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

How new developer who first time see our SDK will know following things:

  1. What you can send with batch items?
  2. That you need to pass array of items from objects Product/Event/Category and use function "to_array();" it looks so not intuitive and complicated. I would say event impossible if you do not know Omnisend API. SDK goal is ensure easy usage for anyone who see SDK first time without knowing actual Omnisend API.
  3. How SDK user will know if he set all required properties to Product/Event/Category class objects correctly? You implemented function "validate". It is hard for SDK user and he will not likely call "validate" function on each object and handle errors? So these functions with your suggested usage is useless.

Here is my suggestion:

  1. Instead of "set_items" create function "add_item" that will accept only object of Product/Event/Category class.
  2. In "Batch" function "validate" validate each object using function "validate"
  3. In "Batch" function "to_array" iterate and call each object method "to_array" ("to_array" and "validate" is internal objects functions and SDK user should not care and use them.

With this changes usage is very simple ("to_array" and "validate" used automatically + types defined):

$batch->add_item($product1);
$batch->add_item($product2);
// or 
$batch->add_item($event1);
$batch->add_item($event2);

Also please:

  1. validate if all added items are same type (can not be different)
  2. remove "endpoint" property. This can be automatically calculated by added items
  3. apply same logic for product variant as well

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

Successfully merging this pull request may close these issues.

3 participants