From 33c5af6c529c6cb3f730ba8ac8d3ca8b3eebdc4f Mon Sep 17 00:00:00 2001
From: Jorrit Schippers <jorrit@161217.no-reply.drupal.org>
Date: Thu, 11 Oct 2012 21:55:21 +0200
Subject: [PATCH] Issue #1444800 by Jorrit: Fixed duplicate rows when using
 optional entity to entity relations in views.

---
 tests/relation.test                     |    1 +
 tests/relation.views.test               |  208 ++++++++++++++++++++++---------
 views/relation_handler_relationship.inc |   51 ++++----
 3 files changed, 171 insertions(+), 89 deletions(-)

diff --git a/tests/relation.test b/tests/relation.test
index 94d2f93..e74959a 100644
--- a/tests/relation.test
+++ b/tests/relation.test
@@ -59,6 +59,7 @@ class RelationTestCase extends DrupalWebTestCase {
     $this->node3 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 1, 'title' => 'Parent'));
     $this->node4 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0, 'title' => 'Child'));
     $this->node5 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0));
+    $this->node6 = $this->drupalCreateNode(array('type' => 'page', 'promote' => 0, 'title' => 'Unrelated'));
   }
 
   function createRelationUsers() {
diff --git a/tests/relation.views.test b/tests/relation.views.test
index cc58b27..e125bc9 100644
--- a/tests/relation.views.test
+++ b/tests/relation.views.test
@@ -24,13 +24,13 @@ class RelationViewsTestCase extends RelationTestCase {
   }
 
   /**
-   * Tests relations in Views.
+   * Tests views with relations as a base table.
    */
-  function testRelationViews() {
+  function testRelationsAsBaseTable() {
     if (!module_exists('views')) {
       return;
     }
-    // Relations as a base table.
+
     foreach (array('symmetric', 'directional', 'octopus') as $relation_type) {
       $view = new view;
       $view->base_table = 'relation';
@@ -84,9 +84,17 @@ class RelationViewsTestCase extends RelationTestCase {
           break;
       }
     }
+  }
+
+  /**
+   * Tests views with symmetric relations.
+   */
+  function testSymmetricRelations() {
+    if (!module_exists('views')) {
+      return;
+    }
 
-    // Symmetric.
-    foreach (array(2, 1) as $required => $count) {
+    foreach (array(FALSE, TRUE) as $required) {
       $view = new view;
       $handler = $view->new_display('default');
       $handler->display->display_options['relationships']['relation_symmetric_node']['id'] = 'relation_symmetric_node';
@@ -97,73 +105,130 @@ class RelationViewsTestCase extends RelationTestCase {
       $handler->display->display_options['fields']['nid']['table'] = 'node';
       $handler->display->display_options['fields']['nid']['field'] = 'nid';
       $handler->display->display_options['fields']['nid']['relationship'] = 'relation_symmetric_node';
+      $handler->display->display_options['fields']['nid2']['id'] = 'nid2';
+      $handler->display->display_options['fields']['nid2']['table'] = 'node';
+      $handler->display->display_options['fields']['nid2']['field'] = 'nid';
       $handler->display->display_options['arguments']['nid']['id'] = 'nid';
       $handler->display->display_options['arguments']['nid']['table'] = 'node';
       $handler->display->display_options['arguments']['nid']['field'] = 'nid';
+
+      // The result should be the same for required and non-required for
+      // argument node 1: Relation from 1 to 4.
       $view->set_arguments(array($this->node1->nid));
       $view->execute();
 
-      // For non-required, we expect NULL and 4 as results.
-      // For required, we expect just 4.
-      $match_count = 0;
-      $null_count  = 0;
-      foreach ($view->result as $result) {
-        if (isset($result->node_node_nid)) {
-          $match_count++;
-          $this->assertEqual($result->node_node_nid, $this->node4->nid);
-        }
-        else {
-          $null_count++;
-        }
+      $this->assertEqual(count($view->result), 1);
+      $result = array_pop($view->result);
+      $this->assertEqual($result->nid, $this->node1->nid);
+      $this->assertEqual($result->node_node_nid, $this->node4->nid);
+
+      $view = $view->clone_view();
+
+      // The result should be different for required and non-required for
+      // argument node 6: for required, there should be no results. For
+      // non-required, node 6 may be returned.
+      $view->set_arguments(array($this->node6->nid));
+      $view->execute();
+
+      if ($required) {
+        $this->assertEqual(count($view->result), 0);
+      }
+      else {
+        $this->assertEqual(count($view->result), 1);
+        $result = array_pop($view->result);
+        $this->assertEqual($result->nid, $this->node6->nid);
+        $this->assertEqual($result->node_node_nid, NULL);
       }
-      $this->assertEqual(count($view->result), $count);
-      $this->assertEqual($match_count, 1);
-      $this->assertEqual($null_count, $count - 1);
     }
+  }
 
-    // Directional to source, to target and to both.
-    for ($r_index = -1; $r_index < 2; $r_index++) {
-      $view = new view;
-      $handler = $view->new_display('default');
-      $handler->display->display_options['relationships']['relation_directional_node']['id'] = 'relation_directional_node';
-      $handler->display->display_options['relationships']['relation_directional_node']['table'] = 'node';
-      $handler->display->display_options['relationships']['relation_directional_node']['field'] = 'relation_directional_node';
-      $handler->display->display_options['relationships']['relation_directional_node']['required'] = 1;
-      $handler->display->display_options['relationships']['relation_directional_node']['r_index'] = $r_index;
-      $handler->display->display_options['fields']['nid']['id'] = 'nid';
-      $handler->display->display_options['fields']['nid']['table'] = 'node';
-      $handler->display->display_options['fields']['nid']['field'] = 'nid';
-      $handler->display->display_options['fields']['nid']['relationship'] = 'relation_directional_node';
-      $handler->display->display_options['arguments']['nid']['id'] = 'nid';
-      $handler->display->display_options['arguments']['nid']['table'] = 'node';
-      $handler->display->display_options['arguments']['nid']['field'] = 'nid';
-      $view->set_arguments(array($this->node3->nid));
-      $view->execute();
+  /**
+   * Tests views with directional relations to source, to target and to both.
+   */
+  function testDirectionalRelations() {
+    if (!module_exists('views')) {
+      return;
+    }
 
-      switch ($r_index) {
-        case -1:
-          // Directional, both ways.
-          $this->assertEqual(count($view->result), 2);
-          $matches = array($this->node1->nid => TRUE, $this->node4->nid => TRUE);
-          foreach ($view->result as $result) {
-            unset($matches[$result->node_node_nid]);
-          }
-          $this->assertFalse($matches);
-          break;
-        case 0:
-          // Source. This finds the 3->4 relation.
-          $this->assertEqual(count($view->result), 1);
-          $this->assertEqual($view->result[0]->node_node_nid, $this->node4->nid);
-          break;
-        case 1:
-          // Target. This finds the 1->3 relation.
+    foreach (array(FALSE, TRUE) as $required) {
+      for ($r_index = -1; $r_index < 2; $r_index++) {
+        $view = new view;
+        $handler = $view->new_display('default');
+        $handler->display->display_options['relationships']['relation_directional_node']['id'] = 'relation_directional_node';
+        $handler->display->display_options['relationships']['relation_directional_node']['table'] = 'node';
+        $handler->display->display_options['relationships']['relation_directional_node']['field'] = 'relation_directional_node';
+        $handler->display->display_options['relationships']['relation_directional_node']['required'] = $required;
+        $handler->display->display_options['relationships']['relation_directional_node']['r_index'] = $r_index;
+        $handler->display->display_options['fields']['nid_source']['id'] = 'nid';
+        $handler->display->display_options['fields']['nid_source']['table'] = 'node';
+        $handler->display->display_options['fields']['nid_source']['field'] = 'nid';
+        $handler->display->display_options['fields']['nid_target']['id'] = 'nid';
+        $handler->display->display_options['fields']['nid_target']['table'] = 'node';
+        $handler->display->display_options['fields']['nid_target']['field'] = 'nid';
+        $handler->display->display_options['fields']['nid_target']['relationship'] = 'relation_directional_node';
+        $handler->display->display_options['arguments']['nid']['id'] = 'nid';
+        $handler->display->display_options['arguments']['nid']['table'] = 'node';
+        $handler->display->display_options['arguments']['nid']['field'] = 'nid';
+
+        // First test: node that has relations. The results should be the same
+        // for required and non-required relations.
+        $view->set_arguments(array($this->node3->nid));
+        $view->execute();
+
+        switch ($r_index) {
+          case -1:
+            // Directional, both ways.
+            $this->assertEqual(count($view->result), 2);
+            $targetmatches = array($this->node1->nid => TRUE, $this->node4->nid => TRUE);
+            foreach ($view->result as $result) {
+              $this->assertEqual($result->nid, $this->node3->nid);
+              unset($targetmatches[$result->node_node_nid]);
+            }
+            $this->assertFalse($targetmatches);
+            break;
+          case 0:
+            // Source. This finds the 3->4 relation.
+            $this->assertEqual(count($view->result), 1);
+            $this->assertEqual($view->result[0]->nid, $this->node3->nid);
+            $this->assertEqual($view->result[0]->node_node_nid, $this->node4->nid);
+            break;
+          case 1:
+            // Target. This finds the 1->3 relation.
+            $this->assertEqual(count($view->result), 1);
+            $this->assertEqual($view->result[0]->nid, $this->node3->nid);
+            $this->assertEqual($view->result[0]->node_node_nid, $this->node1->nid);
+            break;
+        }
+
+        // Second test: node that has no relations. The results should be that
+        // no results are found for the required relation, and 6 / NULL
+        // for the optional relation.
+        $view = $view->clone_view(); 
+        $view->set_arguments(array($this->node6->nid));
+        $view->execute();
+
+        if ($required) {
+          $this->assertEqual(count($view->result), 0);
+        }
+        else {
           $this->assertEqual(count($view->result), 1);
-          $this->assertEqual($view->result[0]->node_node_nid, $this->node1->nid);
-          break;
+          $result = array_pop($view->result);
+          $this->assertEqual($result->nid, $this->node6->nid);
+          $this->assertEqual($result->node_node_nid, NULL);
+        }
       }
     }
+  }
+
+  /**
+   * Tests views with forward directional relations to source, to target and to
+   * both with the same entities types.
+   */
+  function testForwardDirectionalSameEntityRelations() {
+    if (!module_exists('views')) {
+      return;
+    }
 
-    // Forward directional to source, to target and to both with the same entities types.
     for ($r_index = -1; $r_index < 2; $r_index++) {
       $view = new view;
       $view->base_table = 'node';
@@ -213,8 +278,13 @@ class RelationViewsTestCase extends RelationTestCase {
           break;
       }
     }
+  }
 
-    // Reverse directional to source, to target and to both with the same entities types.
+  /**
+   * Tests views with reverse directional relations to source, to target and to
+   * both with the same entities types.
+   */
+  function testReverseDirectionalSameEntityRelations() {
     for ($r_index = -1; $r_index < 2; $r_index++) {
       $view = new view;
       $view->base_table = 'node';
@@ -264,8 +334,13 @@ class RelationViewsTestCase extends RelationTestCase {
           break;
       }
     }
+  }
 
-    // Forward directional to source, to target and to both with different entities types.
+  /**
+   * Tests views with forward directional relations to source, to target and to
+   * both with different entities types.
+   */
+  function testForwardDirectionalDifferentEntityRelations() {
     for ($r_index = -1; $r_index < 2; $r_index++) {
       $view = new view;
       $view->base_table = 'users';
@@ -315,8 +390,13 @@ class RelationViewsTestCase extends RelationTestCase {
           break;
       }
     }
+  }
 
-    // Reverse directional to source, to target and to both with different entities types.
+  /**
+   * Tests views with reverse directional relations to source, to target and to
+   * both with different entities types.
+   */
+  function testReverseDirectionalDifferentEntityRelations() {
     for ($r_index = -1; $r_index < 2; $r_index++) {
       $view = new view;
       $view->base_table = 'node';
@@ -366,6 +446,12 @@ class RelationViewsTestCase extends RelationTestCase {
           break;
       }
     }
+  }
+
+  /**
+   * Tests views deduplication.
+   */
+  function testDeduplication() {
     for ($i = 0; $i < 2; $i++) {
       $view = new view;
       $view->base_table = 'node';
diff --git a/views/relation_handler_relationship.inc b/views/relation_handler_relationship.inc
index b708af5..e26650a 100644
--- a/views/relation_handler_relationship.inc
+++ b/views/relation_handler_relationship.inc
@@ -23,11 +23,17 @@ class relation_handler_relationship extends views_handler_relationship {
   function options_form(&$form, &$form_state) {
     parent::options_form($form, $form_state);
 
-    $options = $this->options_form_summary_options();
+    // Check if this relation is entity-to-entity or entity-to-relation / relation-to-entity.
+    $endpoints_twice = isset($this->definition['entity_type_left']) && isset($this->definition['entity_type_right']);
+
     if ($this->definition['directional']) {
       $form['r_index'] = array(
         '#type' => 'select',
-        '#options' => $options,
+        '#options' => array(
+          -1 => t('Any'),
+          0 => t('Source'),
+          1 => t('Target'),
+        ),
         '#title' => t('Position of the relationship base'),
         '#default_value' => $this->options['r_index'],
         // check_plain()'d in the definition.
@@ -38,7 +44,9 @@ class relation_handler_relationship extends views_handler_relationship {
       if (isset($this->definition['entity_type_' . $key])) {
         $form['entity_deduplication_' . $key] = array(
           '#type' => 'checkbox',
-          '#title' => t('Avoid @type duplication', array('@type' => $this->definition['entity_type_' . $key])),
+          '#title' => $endpoints_twice ?
+            t('Avoid @direction @type duplication', array('@direction' => t($key), '@type' => $this->definition['entity_type_' . $key])) :
+            t('Avoid @type duplication', array('@type' => $this->definition['entity_type_' . $key])),
           '#default_value' => $this->options['entity_deduplication_' . $key],
           '#description' => t('When creating a chain of Views relationships for example from node to relation and then from relation to node (both via the same relation type) then each node will display on both ends. Check this option to avoid this kind of duplication.'),
         );
@@ -46,17 +54,6 @@ class relation_handler_relationship extends views_handler_relationship {
     }
   }
 
-  /**
-   * Return the main options, which are shown in the summary title.
-   */
-  function options_form_summary_options() {
-    return $this->definition['directional'] ? array(
-      -1 => t('Any'),
-      0 => t('Source'),
-      1 => t('Target'),
-    ) : array();
-  }
-
   function query() {
     $field = field_info_field('endpoints');
     $relation_data_table_name = _field_sql_storage_tablename($field);
@@ -82,7 +79,7 @@ class relation_handler_relationship extends views_handler_relationship {
       ),
     );
     if (isset($this->definition['entity_type_left'])) {
-      // The other table is an entity we are relating to.
+      // The left table is an entity, not a relation.
       $join->definition['field'] = $entity_id_field_name;
       $this->ensure_no_duplicate_entities($join->definition['extra'], $this->options['entity_deduplication_left'], $this->definition['relation_type'], $this->definition['entity_type_left'], $this->table_alias, $this->real_field);
       $join->definition['extra'][] = array(
@@ -91,7 +88,7 @@ class relation_handler_relationship extends views_handler_relationship {
       );
     }
     else {
-      // The other table is relation.
+      // The left table is relation.
       $join->definition['field'] = 'entity_id';
     }
     if ($this->definition['directional'] && $this->options['r_index'] > -1) {
@@ -120,6 +117,16 @@ class relation_handler_relationship extends views_handler_relationship {
           ),
         ),
       );
+
+      if ($this->definition['entity_type_left'] == $this->definition['entity_type_right']) {
+        $join->definition['extra'][] = array(
+          // This definition is a bit funny but there's no other way to tell
+          // Views to use an expression in join extra as it is.
+          'field' => $r_index_field_name . ' !=  ' . $l . '.' . $r_index_field_name . ' AND 1',
+          'value' => 1,
+        );
+      }
+
       $join->construct();
       $join->adjusted = TRUE;
       $r = $this->query->add_table($relation_data_table_name, $this->relationship, $join);
@@ -134,18 +141,6 @@ class relation_handler_relationship extends views_handler_relationship {
       'field'      => $this->definition['base field'],
       'type'       => $join_type,
     );
-    // There is no query where these conditions could be added earlier:
-    // $r might be just $l. But even if it's not there's no point in adding
-    // condition if the entity types already restrict them apart.
-    if ($endpoints_twice && $this->definition['entity_type_left'] == $this->definition['entity_type_right']) {
-      $join->definition['extra'][] = array(
-        // This definition is a bit funny but there's no other way to tell
-        // Views to use an expression in join extra as it is.
-        'table' => NULL,
-        'field' => "$r.$r_index_field_name != $l.$r_index_field_name AND 1",
-        'value' => 1,
-      );
-    }
     if (isset($this->definition['entity_type_right'])) {
       // We are finishing on an entity table.
       $join->definition['left_field'] = $entity_id_field_name;
-- 
1.7.9.5