With normal mappers like the price one, we have no issues in replacing values, but the product reference one is slightly different because it relates the node (display) with the product id from commerce_product in a sepparate field.
If you want to load product references, the first time is just fine, but for updates you need to send all the product references again for adding a new one, which in some cases could be a pain.

Maybe it would make sense to have two mappers, one that is quicker and straightforward, replaces the field contents and that's it, another one would be "acumulative", and would not delete product ids but add them to the reference field.

I have a proof of concept working, if it makes sense I'll post the code here.

CommentFileSizeAuthor
#14 1188994_acumulative_mapper-14.patch5.01 KBpcambra
#10 1188994_acumulative_mapper.patch3.7 KBpcambra

Comments

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

I don't quite understand this. Can you give an example of how it works?

summit’s picture

summit commented

Subscribing, greetings, Martijn

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

Example with code:


  $info = field_info_field($target);
  list($field_name, $sub_field) = explode(':', $target);

  // We get the previous values from the field.
  $field = $entity->{$field_name};

  $i = isset($field[LANGUAGE_NONE]) ? count($field[LANGUAGE_NONE]) : 0;

  // We only check if the number of elements is > 0 because the process is slower.
  $check = TRUE;
  if ($i == 0) {
    $check = FALSE;
  }

  foreach ($value as $v) {
    if (!is_array($v) && !is_object($v)) {
      if (strstr($target, 'product_id')) {
        $product_id = $v;
      }
      elseif (strstr($target, 'sku')) {
        if ($product = commerce_product_load_by_sku($v)) {
          $product_id = $product->product_id;
        }
        else {
          drupal_set_message(t('A product with SKU %sku could not be found. Please check that the product exists or import it first.', array('%sku' => $v)), 'error');
        }
      }
      if ($product_id) {
        $delta = $i;
        // If there are already some elements in the field, we search for it to reuse them.
        if ($check) {
          foreach($field[LANGUAGE_NONE] as $key=>$id) {
            if ($id['product_id'] == $product_id) {
              $delta = $key;
              break;
            }
          }
        }
        $field[LANGUAGE_NONE][$delta]['product_id'] = $product_id;
      }
    }
    if ($info['cardinality'] == 1) {
      break;
    }
    $i++;
  }
  $entity->{$field_name} = $field;
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

I think an accumulative product reference is a really great idea... However, it makes a mess of normal updates, where the update might be coming from an external source and might be done over and over. Would we be closing the door on iterative updates?

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

You're right, we shouldn't assume that users want to always do update. But I'm also worried about the likely confusion we could create having 2 different mappings for apparently the same thing.

Maybe we could keep one mapper and check the import mode, if we're creating/replacing we run the current mapping code, but when we're updating we run the acumulative one.

if ($this->config['update_existing'] == FEEDS_UPDATE_EXISTING) {
// Acumulative routines.
}
else {
// REPLACE/CREATE
// Normal way
}
smokinggoat’s picture

smokinggoat commented

Hey Pedro -
I get an error on my test. I imported products using one Importer, then I created a new Display importer that attempts add several products to a single node (using the *same* product CSV - so one product per line). I used the module you sent me - and I get the following error:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=9&op=do StatusText: OK ResponseText: ( ! ) Fatal error: Call to undefined function dpm() in /projects/saloncommerce/www/sites/all/modules/commerce_feeds/mappers/commerce_product_reference.inc on line 46 Call Stack #TimeMemoryFunctionLocation 10.0002107288{main}( )../index.php:0 20.385336344280menu_execute_active_handler( )../index.php:21 30.392037682544call_user_func_array ( )../menu.inc:503 40.392137682760system_batch_page( )../menu.inc:0 50.393437682856_batch_page( )../system.admin.inc:2284 60.393737683448_batch_do( )../batch.inc:80 70.393737683448_batch_process( )../batch.inc:161 80.614437830648call_user_func_array ( )../batch.inc:284 90.614537830648feeds_batch( )../batch.inc:0 100.649841581344FeedsSource->import( )../feeds.module:170 110.696241734672FeedsProcessor->process( )../FeedsSource.inc:349 120.720941739600FeedsProcessor->map( )../FeedsProcessor.inc:127 130.737942698392commerce_product_reference_feeds_set_target( )../FeedsProcessor.inc:398

Here is the export of my Importer:

$feeds_importer = new stdClass;
$feeds_importer->disabled = FALSE; /* Edit this to true to make a default feeds_importer disabled initially */
$feeds_importer->api_version = 1;
$feeds_importer->id = 'pedro_test2_display';
$feeds_importer->config = array(
  'name' => 'Pedro test2 Display',
  'description' => '',
  'fetcher' => array(
    'plugin_key' => 'FeedsFileFetcher',
    'config' => array(
      'allowed_extensions' => 'txt csv tsv xml opml',
      'direct' => FALSE,
    ),
  ),
  'parser' => array(
    'plugin_key' => 'FeedsCSVParser',
    'config' => array(
      'delimiter' => ',',
      'no_headers' => 0,
    ),
  ),
  'processor' => array(
    'plugin_key' => 'FeedsNodeProcessor',
    'config' => array(
      'content_type' => 'product_display',
      'expire' => '-1',
      'author' => '1',
      'mappings' => array(
        0 => array(
          'source' => 'Model',
          'target' => 'guid',
          'unique' => 1,
        ),
        1 => array(
          'source' => 'Description',
          'target' => 'body',
          'unique' => FALSE,
        ),
        2 => array(
          'source' => 'Model',
          'target' => 'title',
          'unique' => FALSE,
        ),
        3 => array(
          'source' => 'SKU',
          'target' => 'field_product:sku',
          'unique' => FALSE,
        ),
      ),
      'update_existing' => '2',
      'input_format' => 'plain_text',
    ),
  ),
  'content_type' => '',
  'update' => 0,
  'import_period' => '-1',
  'expire_period' => 3600,
  'import_on_create' => 1,
  'process_in_background' => 0,
);
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

@smokinggoat, he just left a debug (dpm) statement in there. Enable devel module and the error will go away.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

Oops sorry about that, thanks Randy!

smokinggoat’s picture

smokinggoat commented

Yeah! Alright, it works! It properly accumulates the product references!

So Pedro, if you get an updated version with the conditional "accumulate" or "override" function, I'll test both cases. :-)

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new 1188994_acumulative_mapper.patch3.7 KB

Ok, here's a patch that considers the update_existing setting for the node processor and if it's set to update it uses the accumulative mapper, and if it's set to create or replace, it uses the usual mapper.

twistor’s picture

twistor commented

You might want to check this against #996808: Update existing doesn't reset targets that have real_target set.. I think you might depending on behavior that's actually a bug. The $field that's attached to $entity is supposed to have been reset before the mapping starts. The reason for copying it off the entity is to support multiple mappings to the same target.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Title: Acumulative product reference mapper » Accumulative product reference mapper
Status: Needs review » Needs work

Ok, reviewed what twistor says (finally) and it seems that our approach for accumulative processor is based in an "unexpected" behavior of Feeds, that seems that will disappear eventually as the majority of use cases are different of this one.

Normally one would expect to pass all the field values all over so you can delete references, i.e.

$node->field_values = array(1,2,3)

$node->field_values = array(1,2,4)

Will end with values: 1, 2 and 4

But we're dealing with an use case where we'll end with 1,2,3 and 4.

Probably we'll want to implement a FeedsProductDisplay plugin on top of FeedsProcessor to add a fourth option: add, replace, update and accumulate which will behave as we expect.

Setting back to needs work.

twistor’s picture

twistor commented

Couldn't you just set real_target to a non-existent attribute? Feeds would never reset it, and you would get the behavior you're looking for.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 1188994_acumulative_mapper-14.patch5.01 KB

Great idea twistor, thanks!

Here's a patch that changes the real target so that it's never reseted and just for the use case of updating (not replacing) it keeps the old records.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Fixed

Let's commit this!

Thanks to everyone :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

summit’s picture

summit commented

Hi, sorry to ask, but is this commiitted?
greetings, Martijn

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

It was committed to -dev version on Jan 13 as you can see in #15

summit’s picture

summit commented

Hi, Thanks for telling! Greetings, Martijn

chadmkidner’s picture

chadmkidner commented

Hello, I'm trying to understand whether my 'use case' is the same as the one solved with this patch or what changes I would need to make to have it work for my case.

I am understanding the idea behind accumulating values for the product references which is perfect but does this method of accumulate target this value specifically or effect all possible fields?

For example, if you have a product that comes in Small, Medium, and Large but also have other attributes.. say color is one. Would this method allow to accumulate for the unique value but update for non-unique attributes?

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

This mapper option is for acumulating the product reference field in the product display, exactly as you can see in #12

$node->field_values = array(1,2,3)

$node->field_values = array(1,2,4)

Will end with values: 1, 2 and 4

But we're dealing with an use case where we'll end with 1,2,3 and 4.

This mapper option has anything to do with the product import itself but with the relationship between product display and product.

Isostar’s picture

Isostar commented
Status: Closed (fixed) » Active

There is a problem with the accumulate in the case that in the import file some SKU's are removed from the product display.
In this case these product stay referenced.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Active » Closed (fixed)

Please don't revive issues that have been committed, open a new one instead

vbard’s picture

vbard commented
Version: 7.x-1.x-dev » 7.x-1.3
Component: Miscellaneous » Documentation
Category: task » support
Status: Closed (fixed) » Active

Hi!
Sorry, maybe I should create another issue, I'm new here... The question is whether this patch is already in 1.3 version of the module? If so, how can I use the feature discussed above? I came here from https://drupal.org/node/135116 and I'm trying to understand how can this feature help me to import multiple products into one product display node.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Component: Documentation » Miscellaneous
Category: support » task
Status: Active » Closed (fixed)

Please do not reopen issues after a year of being closed.
Feature entered long ago and is available in the module.