Add Cart system

+1

+1

Lets please do this as early as possible. Please also take a look at this for further cleanup issues for the UI part of TMGMT: #1420644: Move all the overview views out of the core UI module

Agree, separate module.

This patch is the code written by Byron Sorrels, published on https://github.com/byronsorrells/tmgmt_cart.

We agreed that we will continue the development here, so posting this patch as a starting point.

I reviewed it and identified the major elements that need to be resolved to make this generally useful and not just for the limited use case that they had. Looks like they did a bit of work since I did that, so need to review the changes but I think it's still accurate.

A better way to inject the card handling into the various checkout forms. there are also the translate tabs, the upcoming potx integration and so on. tmgmt_cart can't manually hack itself into all those places.
This either requires some API that's part of tmgmt_ui and cart can hook into, or we'd just make the cart part of tmgmt_ui. Not sure yet, but we need some sort of standardization for source plugin UI's so that they can easily support both immediate checkout and add to cart wherever they create job items/jobs

We need to correctly detect the source language, including the fact that there could be multiple source languages.
This really means that instead of tracking an arbitrary array, we need to track job items in the cart. This is a bit of an overhead (I'd imagine we create and save them and then store the ID's in the session and need to load them when we need them), but it solves a whole bunch of problems and results in better UX. a) The unreliable (source types are not unique across plugins) source map shuffling can be dropped. b) We can display the same label and information in the block as on the checkout page. It will also require some changes/additionals to job items, for example, we need a way to ask them what their source language is, right now, there is no way to ask for this information. Possibly additional things that I can't think of right now. That API would also help with 1. as we could standardize more of the logic in the plugin specific job/item creation code
When we have job items and can get their source language, we can loop over them, and create as many jobs with the corresponding source language as we have to. In your case, this should give the same result, a single job with source en, assuming you only selected those sources.

This needs: #2155203: Extend source plugins with methods to get source language and existing translations

The checkout will need to go through the checkout API's
Instead of the hardcoded checkout, we need to go through the helper functions, so that we can direct users through the checkout process if they need to select/configure stuff/have multiple translators.

We will also want to support translating into multiple target languages with multiple jobs. This can wait for a later phase/iteration.
I'd leave this out for now, but we could then add a multi-value target language selector and then create jobs for every necessary source -> target combination, excluding those that are already in the desired target language(s). A simple version of this shouldn't be too hard, but there are a lot of fancy ideas of extend it, like giving the user more control if he e.g. wants to use an existing translation to translate something into another language and so on. But I think the above would already be pretty cool for a first version (a first post-commit update version ;))

In our use case, we exclusively use the /admin/tmgmt/sources UI to request translations. So far, our biggest need to is to provide labels for the items in the cart. After adding several items from different sources and across pagination, it starts to get very difficult to know what you've added to the cart. I attached a screen shot to show the cart UI.

Should we prioritize the items listed in #10?

Regarding multiple target languages, we did implement that a simple way. In our case, 99.99% of the time we want to translate something into all enabled languages. I used the language neutral constant as the target language, created a single parent job, and then created a "sub job" for each language code that's enabled (with a reference to the parent job and an indication that it's a subjob. This allows us to poll for one job instead of 35. We ingest translations for all 35 languages with that one request and then iterate over it.

Assigning this to me. Need to play with this a bit and think about how to continue, the source language issue referenced above has been committed.

First step - integrate with our new job item language API; use job items in cart

First, quick review.

1. +++ b/sources/node/tmgmt_node.plugin.inc
   @@ -18,7 +18,7 @@ class TMGMTNodeSourcePluginController extends TMGMTDefaultSourcePluginController
        $type = node_type_get_type($node);
        // Get all the fields that can be translated and arrange their values into
        // a specific structure.
   -    $structure = tmgmt_field_get_source_data('node', $node, $job_item->getJob()->source_language);
   +    $structure = tmgmt_field_get_source_data('node', $node, $job_item->getSourceLangCode());
        $structure['node_title']['#label'] = $type->title_label;
        $structure['node_title']['#text'] = $node->title;
   

   Hm, this fails because there was no job, right?

   This is problematic, because both are correct. Once there is a job, only the source language of the job is relevant, not the one of the job item/source.

   For now, I'd suggest you simply check if there is a job and fall back to the source language if not.

2. +++ b/tmgmt_cart/tmgmt_cart.inc
   @@ -0,0 +1,118 @@
   +  public function addJobItems(array $items) {
   +    foreach ($items as $item) {
   +      $key = $item->item_type . '_' . $item->item_id;
   +      $this->cart[$key] = $item->tjiid;
   +    }
   

   $key isn't unique, I would just do cart[] =

3. +++ b/tmgmt_cart/tmgmt_cart.inc
   @@ -0,0 +1,118 @@
   +   *   If the source item is in the cart.
   +   */
   +  public function isSourceItemAdded($source_type, $source_id) {
   +    return isset($this->cart[$source_type . '_' . $source_id]);
   +  }
   +
   

   Which means this won't work anymore that easily but that's true anyway, needs the plugin, type and id.

   You will have to load the job items and check them.

   I would also recommend to use the same variable names as $job->addItem(), $plugin, $item_type and $item_id.

here it comes... no interdiff provided as with merging the tmgmt_cart/ui it is basically all changed.

- now we can request translation to multiple target languages
- removed the message about not checked out items, added info about ignoring items with same source/target
- updated add to cart message that displays the actual number of added items (not working form node source => vbo)

Looks good, nice test coverage, a few things below, but nothing too big I think.

1. +++ b/sources/entity/tmgmt_entity.plugin.inc
   @@ -30,7 +30,15 @@ class TMGMTEntitySourcePluginController extends TMGMTDefaultSourcePluginControll
          throw new TMGMTException(t('Unable to load entity %type with id %id', array('%type' => $job_item->item_type, $job_item->item_id)));
        }
   -    return tmgmt_field_get_source_data($job_item->item_type, $entity, $job_item->getJob()->source_language, TRUE);
   +
   +    if (!empty($job_item->tjid)) {
   +      $source_language = $job_item->getJob()->source_language;
   +    }
   +    else {
   +      $source_language = $job_item->getSourceLangCode();
   +    }
   +
   +    return tmgmt_field_get_source_data($job_item->item_type, $entity, $source_language, TRUE);
      }
   

   We should be able to revert this change now.

2. +++ b/sources/entity/ui/tmgmt_entity_ui.pages.inc
   @@ -17,6 +17,13 @@ function tmgmt_entity_ui_translate_form($form, &$form_state, $build) {
    
   +  // Check if there is a job / job item that references this translation.
   +  list($id, $vid, $bundle) = entity_extract_ids($form_state['entity_type'], $form_state['entity']);
   
   @@ -26,8 +33,6 @@ function tmgmt_entity_ui_translate_form($form, &$form_state, $build) {
      $languages = language_list();
   -  // Check if there is a job / job item that references this translation.
   -  list($id, $vid, $bundle) = entity_extract_ids($form_state['entity_type'], $form_state['entity']);
      $entity_language = entity_language($form_state['entity_type'], $form_state['entity']);
      $items = tmgmt_job_item_load_latest('entity', $form_state['entity_type'], $id, $entity_language);
   

   The moved comment here seems a bit strange, that actually belongs to the code below, the entity_extract_ids() was just a necessary step for that. In short, don't move the comment.

3. +++ b/sources/i18n_string/tmgmt_i18n_string.test
   @@ -20,7 +20,7 @@ class TMGMTI18nStringSourceTestCase extends TMGMTBaseTestCase {
    
   -  function testI18nStringSourceTaxonomy() {
   +  function NOtestI18nStringSourceTaxonomy() {
        // Test translation of a vocabulary.
   

   More of those below :)

4. +++ b/sources/locale/tmgmt_locale.plugin.inc
   @@ -77,7 +77,14 @@ class TMGMTLocaleSourcePluginController extends TMGMTDefaultSourcePluginControll
    
   -    if ($job_item->getJob()->source_language == 'en') {
   +    if (!empty($job_item->tjid)) {
   +      $source_language = $job_item->getJob()->source_language;
   +    }
   +    else {
   +      $source_language = $job_item->getSourceLangCode();
   +    }
   +
   +    if ($source_language == 'en') {
          $query = db_select('locales_source', 'ls');
   

   Also revert :)

5. +++ b/sources/node/tmgmt_node.plugin.inc
   @@ -16,9 +16,21 @@ class TMGMTNodeSourcePluginController extends TMGMTDefaultSourcePluginController
        $type = node_type_get_type($node);
   +
   +    // If the job item belongs to a job, the source language is the one provided
   +    // by the job.
   +    if (!empty($job_item->tjiid)) {
   +      $source_language = $job_item->getJob()->source_language;
   +    }
   +    // If no job yet, the source language will be retrieved from the source
   +    // object which is going to be translated.
   +    else {
   +      $source_language = $job_item->getSourceLangCode();
   +    }
   +
        // Get all the fields that can be translated and arrange their values into
   

   And... here

6. +++ b/sources/node/ui/tmgmt_node_ui.test
   @@ -416,4 +416,35 @@ class TMGMTNodeSourceUITestCase extends TMGMTEntityTestCaseUtility {
   +    }
   ...
   +    // Test the source overview.
   +    $this->drupalPost('admin/tmgmt/sources/node', array(
   +      'views_bulk_operations[0]' => TRUE,
   +      'views_bulk_operations[1]' => TRUE,
   +    ), t('Add to cart'));
   +    $this->drupalGet('admin/tmgmt/cart');
   +    $this->assertText($nodes[0]->title);
   +    $this->assertText($nodes[1]->title);
   

   Based on what is the order here? Could this possibly lead to random fails? (sql sort is not stable if there is no sort condition or one where the node have the same value).

   maybe just create two nodes to begin and create the third afterwards to make sure.

7. +++ b/ui/includes/tmgmt_ui.pages.inc
   @@ -854,6 +854,129 @@ function tmgmt_ui_ajax_callback_translator_select($form, &$form_state) {
   +  $form['info'] = array(
   +    '#type' => 'markup',
   +    '#markup' => t('If the item\'s source language will be the same as the target language the item will be ignored.'),
   

   Wondering how it would look if we made this the description of the language selector?

8. +++ b/ui/tmgmt_ui.module
   @@ -57,6 +67,15 @@ function tmgmt_ui_menu() {
   + */
   +function tmgmt_ui_cart_title() {
   +  return t('Cart (@count)', array('@count' => count(tmgmt_ui_cart_get()->getJobItemsFromCart())));
   +}
   

   Let's add a method to return the count, then it can do that directly on the ID's, this always needs to load all those job items, including their possibly large data.

9. +++ b/ui/tmgmt_ui.module
   @@ -259,6 +286,138 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
   + */
   +function tmgmt_ui_cart_job_item_make($plugin, $item_type, $item_id) {
   +  $item = tmgmt_job_item_create($plugin, $item_type, $item_id);
   +  $item->save();
   +  return $item;
   

   Hm, how useful is this really? We really just save a single, easy line and add a whole function for it.

10. +++ b/ui/tmgmt_ui.module
    @@ -259,6 +286,138 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
    +function tmgmt_ui_add_cart_form(&$form, &$form_state, $plugin, $item_type = NULL, $item_id = NULL) {
    

    This is a bit fragile with empty ID checks, optional validates and hardcoded assumptions about where the items are stored.

    We should at least make sure that it is properly documented what the requirements for using this are.

11. +++ b/ui/tmgmt_ui.module
    @@ -259,6 +286,138 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
    +    if (!tmgmt_ui_cart_get()->isSourceItemAdded($plugin, $item_type, $source_id)) {
    +      $job_items[] = tmgmt_ui_cart_job_item_make($plugin, $item_type, $source_id);
    +    }
    +  }
    +
    +  tmgmt_ui_cart_get()->addJobItems($job_items);
    

    If we want to simplify the code here, why not do $cart->addJobItem($plugin, $item_type, $source_id) ? Maybe as a separate method.

12. +++ b/ui/tmgmt_ui.rules.inc
    @@ -0,0 +1,45 @@
    +  if (!tmgmt_ui_cart_get()->isSourceItemAdded($plugin, $item_type, $item_id)) {
    +    $job_item = tmgmt_ui_cart_job_item_make($plugin, $item_type, $item_id);
    +    tmgmt_ui_cart_get()->addJobItems(array($job_item));
    +  }
    

    We could even say that that function checks this by default.

6. - there is sort by Post date. Could not come up with anything better, will think about it.

The rest is implemented, let's see if test bot is happy.

1. +++ b/ui/includes/tmgmt_ui.cart.inc
   @@ -117,6 +148,20 @@ class TMGMTJobItemUICart {
   +  public function count() {
   +    if (empty($this->count)) {
   +      $this->count = count($this->cart);
   +    }
   +
   +    return $this->count;
   +  }
   

   Hm, that's a fun one :)

   (not actual code)

   $cart->addItem();
   $cart->count(); -> 1
   $cart->addItem();
   $cart->count(); -> 1

   :)

   See http://programmers.stackexchange.com/questions/80084/is-premature-optimi...

   Drop $this->count. It's not worth the additional complexity it would require to work reliably unless we call it hundreds of times and I don't think we do :)

2. +++ b/ui/tmgmt_ui.module
   @@ -324,13 +324,16 @@ function tmgmt_ui_cart_job_item_make($plugin, $item_type, $item_id) {
     */
   -function tmgmt_ui_add_cart_form(&$form, &$form_state, $plugin, $item_type = NULL, $item_id = NULL) {
   +function tmgmt_ui_add_cart_form(&$form, &$form_state, $plugin, $item_type, $item_id = NULL, $validator = NULL, $show_info_message = FALSE) {
      $form_state['tmgmt_cart'] = array(
   

   Hm, not sure if that is what I meant/an improvement.

   The main thing I requested is good documentation:

   There are two main cases where this can be used, and it's OK to limit it to that.

   But we need to document what it means. For example, if you use this on an overview, you need to need to ensure that the ID's will be in the correctly named form element. Also include two sample calls with in @code @endcode fragements.

   And thinking about that, what would be great is starting a new @defgroup for this, just copy an example in tmgmt.api.php and then add an @ingroup tmgmt_cart to all relevant functions and classes.

3. +++ b/ui/tmgmt_ui.test
   @@ -457,11 +457,11 @@ class TMGMTUITestCase extends TMGMTBaseTestCase {
        // Create a few job items and add them to the cart.
   -    $job_items[1] = tmgmt_ui_cart_job_item_make('test_source', 'test', 1);
   -    $job_items[2] = tmgmt_ui_cart_job_item_make('test_source', 'test', 2);
   -    $job_items[3] = tmgmt_ui_cart_job_item_make('test_source', 'test', 3);
   -    $job_items[4] = tmgmt_ui_cart_job_item_make('test_source', 'test', 4);
   -    $job_items[5] = tmgmt_ui_cart_job_item_make('test_source', 'test', 5);
   +    $job_items[1] = $this->jobItemMake('test_source', 'test', 1);
   +    $job_items[2] = $this->jobItemMake('test_source', 'test', 2);
   +    $job_items[3] = $this->jobItemMake('test_source', 'test', 3);
   +    $job_items[4] = $this->jobItemMake('test_source', 'test', 4);
   +    $job_items[5] = $this->jobItemMake('test_source', 'test', 5);
   

   Ah, I guess this and the example below are the main reasons you added helper function ;) Would a loop be easier here?

4. +++ b/sources/entity/tmgmt_entity.plugin.inc
   @@ -30,6 +30,7 @@ class TMGMTEntitySourcePluginController extends TMGMTDefaultSourcePluginControll
        if (!$entity) {
          throw new TMGMTException(t('Unable to load entity %type with id %id', array('%type' => $job_item->item_type, $job_item->item_id)));
        }
   +
        return tmgmt_field_get_source_data($job_item->item_type, $entity, $job_item->getJob()->source_language, TRUE);
      }
   

   Unnecessary empty line left-over after the revert.

5. +++ b/sources/entity/ui/tmgmt_entity_ui.pages.inc
   @@ -17,6 +17,13 @@ function tmgmt_entity_ui_translate_form($form, &$form_state, $build) {
   +
   +  $form['top_actions']['#type'] = 'actions';
   +  $form['top_actions']['#weight'] = -10;
   +  tmgmt_ui_add_cart_form($form['top_actions'], $form_state, 'entity', $build['#entity_type'], $id,
   +    'tmgmt_ui_cart_source_overview_validate', TRUE);
   +
   

   This looks wrong, the translate tab version shouldn't have this?

   See above. The most important is documenting how the function works and what the requirements for using it are. Sorry if I was unclear.

6. +++ b/sources/node/tmgmt_node.plugin.inc
   @@ -16,6 +16,7 @@ class TMGMTNodeSourcePluginController extends TMGMTDefaultSourcePluginController
        $node = node_load($job_item->item_id);
        $type = node_type_get_type($node);
   +
        // Get all the fields that can be translated and arrange their values into
   

   Same left-over here.

7. +++ b/ui/includes/tmgmt_ui.cart.inc
   @@ -0,0 +1,170 @@
   +  public function addJobItems(array $items) {
   ...
   +  public function addJobItem($plugin, $item_type, $item_id) {
   

   Using singular/plural is one way to differentiate the two methods.

   TMGMTJob does it differently, it uses addItem($plugin, ...) and addExistingItem($item). The main reason there was that we already had addItem() and needed a method name for the existing one. It might or might not be better.. but it might make sense to be consistent. Let's discuss tomorrow.

8. +++ b/ui/tmgmt_ui.module
   @@ -259,6 +286,139 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
   +
   +/**
   + * Will create and save job item.
   + *
   + * @param $plugin
   + *   The plugin name.
   + * @param $item_type
   + *   The source item type.
   + * @param $item_id
   + *   The source item id.
   + *
   + * @return TMGMTJobItem
   + *   The job item that was added to the job or FALSE if it couldn't be saved.
   + */
   +function tmgmt_ui_cart_job_item_make($plugin, $item_type, $item_id) {
   +  $item = tmgmt_job_item_create($plugin, $item_type, $item_id);
   +  $item->save();
   +  return $item;
   +}
   

   Looks like you left the function in place?

9. +++ b/ui/tmgmt_ui.module
   @@ -259,6 +286,139 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
   +  if ($show_info_message) {
   +    // Compose the cart info message for the translate tab.
   +    $count = count(tmgmt_ui_cart_get()->getJobItemsFromCart());
   

   This one still has a slow count()

10. +++ b/ui/tmgmt_ui.module
    @@ -259,6 +286,139 @@ function tmgmt_ui_source_overview_form_submit($form, &$form_state) {
    +
    +  $job_items = array();
    +  foreach ($source_items as $source_id) {
    +    if ($job_item = tmgmt_ui_cart_get()->addJobItem($plugin, $item_type, $source_id)) {
    +      $job_items[] = $job_item;
    +    }
    +  }
    +
    +  tmgmt_ui_cart_get()->addJobItems($job_items);
    +  drupal_set_message(format_plural(count($job_items), '@count content source was added into the <a href="@url">cart</a>.',
    +    '@count content sources were added into the <a href="@url">cart</a>.', array('@url' => url('admin/tmgmt/cart'))));
    

    Adding them once should be enough :)

    You still need the if () I think but just to increase a counter?

    if ($cart->add...) {
    $count++;
    }

yup, i guess that coding after few beers isnt that good idea :)

with enabled all tests

Did extensive testing of this, improved the submit function and some other places and added comments.

Yeah, too tired to be capable of reading patch names. Will address the feedback above later.

1. Yeah, not sure if we need it, the idea was that it is transparent and once called, is saved. However, now that we do this already in save() assuming there's a job makes this pointless as it actually means we start a save, then save again which makes the actual saving an update. Not related.

2. Fixed.

3. Fixed as part of 2.

Dear sirs and madams. We have a cart :)

Thanks everyone! Also made sure to credit byron sorrels as well.

@Miro. The job item order is also not sorted in any way inside a job,

Awesome!

Just applied the patch (#34) and trying it out. I love the tab idea! I'm able to successfully add five items to my cart from various sources. The tab says Cart (5), but I get an error when trying to view the cart:

Fatal error: Call to undefined method TMGMTJobItem::getSourceLangCode() in www/sites/all/modules/contrib/tmgmt/sources/locale/tmgmt_locale.plugin.inc on line 86

Are you able to reproduce?

@bsorrells the getSourceLangCode() has been added among others in #2155203: Extend source plugins with methods to get source language and existing translations. Please use the latest status from the repository.