Merge pull request #45 from oat-sa/legacy-qti-identifiable-fix

Fixing issues with QtiIdentifiableCollection from master.
oat-sa · Aug 2, 2016 · 0b6808f · 0b6808f
2 parents e32d5b6 + 12e1dcb
commit 0b6808f
Show file tree

Hide file tree

Showing 4 changed files with 166 additions and 34 deletions.
diff --git a/composer.json b/composer.json
@@ -2,7 +2,7 @@
 	"name": "qtism/qtism",
 	"description": "OAT QTI Software Module Library",
 	"type": "library",
-	"version": "0.10.5",
+	"version": "0.10.6",
 	"authors": [
 		{
 			"name": "Open Assessment Technologies S.A.",

diff --git a/qtism/data/QtiIdentifiableCollection.php b/qtism/data/QtiIdentifiableCollection.php
@@ -27,6 +27,7 @@
 use \ReflectionClass;
 use \InvalidArgumentException;
 use \OutOfRangeException;
+use \UnexpectedValueException;
 use \SplObserver;
 use \SplSubject;
 
@@ -147,6 +148,7 @@ public function offsetUnset($offset) {
 		if (gettype($offset) === 'string') {
 			$data = &$this->getDataPlaceHolder();
 			if (isset($data[$offset])) {
+                $data[$offset]->detach($this);
 				unset($data[$offset]);
 			}
 		}
@@ -155,6 +157,50 @@ public function offsetUnset($offset) {
 			throw new OutOfRangeException($msg);
 		}
 	}
+
+    /**
+     * Replace an $object in the collection by another $replacement $object.
+     *
+     * @param mixed $object An object to be replaced.
+     * @param mixed $replacement An object to be used as a replacement.
+     * @throws \InvalidArgumentException If $object or $replacement are not compliant with the current collection typing.
+     * @throws \UnexpectedValueException If $object is not contained in the collection.
+     */
+    public function replace($object, $replacement)
+    {
+        $this->checkType($object);
+        $this->checkType($replacement);
+
+        if (($search = array_search($object, $this->dataPlaceHolder, true)) !== false) {
+
+            $objectKey = $search;
+            $replacementKey = $replacement->getIdentifier();
+
+            if ($objectKey === $replacementKey) {
+                // If they share the same key, just replace.
+                $this->dataPlaceHolder[$objectKey] = $replacement;
+            } else {
+                // Otherwise, we have to insert the $replacement object at the appropriate offset (just before $object),
+                // and then remove the former $object.
+                $objectOffset = array_search($objectKey, array_keys($this->dataPlaceHolder));
+
+                $this->dataPlaceHolder = array_merge(
+                    array_slice($this->dataPlaceHolder, 0, $objectOffset),
+                    array($replacementKey => $replacement),
+                    array_slice($this->dataPlaceHolder, $objectOffset, null)
+                );
+
+                $this->offsetUnset($objectKey);
+            }
+
+            $replacement->attach($this);
+            $object->detach($this);
+
+        } else {
+            $msg = "The object you want to replace could not be found.";
+            throw new UnexpectedValueException($msg);
+        }
+    }
 
 	/**
 	 * Implementation of SplObserver::update.
@@ -164,13 +210,7 @@ public function offsetUnset($offset) {
 	public function update(SplSubject $subject) {
 		// -- case 1 (QtiIdentifiable)
 		// If it is a QtiIdentifiable, it has changed its identifier.
-		$data = &$this->getDataPlaceHolder();
-		foreach (array_keys($data) as $k) {
-			if ($data[$k] === $subject && $k !== $subject->getIdentifier()) {
-				unset($data[$k]);
-				$this->offsetSet(null, $subject);
-			}
-		}
+		$this->replace($subject, $subject);
 	}
 
 	public function __clone() {

diff --git a/qtism/data/storage/xml/XmlDocument.php b/qtism/data/storage/xml/XmlDocument.php
@@ -392,8 +392,7 @@ public function includeAssessmentSectionRefs($validate = false)
                             $collection = $parent->getSectionParts();
                         }
 
-                        $collection->detach($assessmentSectionRef);
-                        $collection->attach($sectionRoot);
+                        $collection->replace($assessmentSectionRef, $sectionRoot);
                     }
                 }
 

diff --git a/test/qtism/data/QTIIdentifiableCollectionTest.php b/test/qtism/data/QTIIdentifiableCollectionTest.php
@@ -7,27 +7,120 @@
 
 class QtiIdentifiableCollectionTest extends QtiSmTestCase {
 
-	public function testWithWeights() {
-
-		$weight1 = new Weight('weight1', 1.0);
-		$weight2 = new Weight('weight2', 1.1);
-		$weight3 = new Weight('weight3', 1.2);
-		$weights = new WeightCollection(array($weight1, $weight2, $weight3));
-
-		$this->assertTrue($weights['weight1'] === $weight1);
-		$this->assertTrue($weights['weight2'] === $weight2);
-		$this->assertTrue($weights['weight3'] === $weight3);
-
-		$this->assertTrue($weights['weightX'] === null);
-
-
-		// Can I address the by identifier?
-		$this->assertTrue($weights['weight2'] === $weight2);
-
-		// What happens if I change the identifier of an object.
-		// Is it adressable with the new identifier?
-		$weight2->setIdentifier('weightX');
-		$this->assertTrue($weights['weightX'] === $weight2);
-		$this->assertFalse(isset($weights['weight2']));
-	}
-}
+    public function testWithWeights() {
+
+        $weight1 = new Weight('weight1', 1.0);
+        $weight2 = new Weight('weight2', 1.1);
+        $weight3 = new Weight('weight3', 1.2);
+        $weights = new WeightCollection(array($weight1, $weight2, $weight3));
+
+        $this->assertTrue($weights['weight1'] === $weight1);
+        $this->assertTrue($weights['weight2'] === $weight2);
+        $this->assertTrue($weights['weight3'] === $weight3);
+
+        $this->assertTrue($weights['weightX'] === null);
+        $this->assertFalse(isset($weights['weightX']));
+
+        // Can I address the by identifier?
+        $this->assertTrue($weights['weight2'] === $weight2);
+
+        // What happens if I change the identifier of an object.
+        // Is it adressable with the new identifier?
+        $weight2->setIdentifier('weightX');
+        $this->assertTrue($weights['weightX'] === $weight2);
+        $this->assertFalse(isset($weights['weight2']));
+        $this->assertTrue(isset($weights['weightX']));
+
+        // What happens if I remove an object?
+        unset($weights['weightX']);
+        $this->assertFalse(isset($weights['weightX']));
+    }
+
+    /**
+     * @depends testWithWeights
+     */
+    public function testReplace()
+    {
+        $weight1 = new Weight('weight1', 1.0);
+        $weight2 = new Weight('weight2', 1.1);
+        $weight3 = new Weight('weight3', 1.2);
+        $weights = new WeightCollection(array($weight1, $weight2, $weight3));
+
+        // Let's replace weight2 with another Weight object having the same identifier.
+        $this->assertSame($weight2, $weights['weight2']);
+
+        $weightBis = new Weight('weight2', 2.0);
+        $weights->replace($weight2, $weightBis);
+
+        $this->assertFalse($weight2 === $weights['weight2']);
+        $this->assertCount(3, $weights);
+
+        // Is the order still respected?
+        $this->assertSame(
+            array('weight1', 'weight2', 'weight3'),
+            $weights->getKeys()
+        );
+
+        // Let's replace (the new) weight2 with another Weight object having different identifiers.
+        $weight4 = new Weight('weight4', 1.4);
+        $weights->replace($weights['weight2'], $weight4);
+
+        $this->assertCount(3, $weights);
+        $this->assertFalse(isset($weights['weight2']));
+
+        // Now check the order of things, let's get the keys and compare them.
+        $this->assertSame(
+            array('weight1', 'weight4', 'weight3'),
+            $weights->getKeys()
+        );
+    }
+
+    /**
+     * @depends testWithWeights
+     */
+    public function testEventsUnset()
+    {
+        $weight1 = new Weight('weight1', 1.0);
+        $weight2 = new Weight('weight2', 1.2);
+        $weights = new WeightCollection(array($weight1, $weight2));
+
+        $this->assertCount(2, $weights);
+        $this->assertTrue(isset($weights['weight1']));
+        $this->assertTrue(isset($weights['weight2']));
+
+        $weight1->setIdentifier('weightX');
+        $this->assertCount(2, $weights);
+        $this->assertFalse(isset($weights['weight1']));
+        $this->assertTrue(isset($weights['weight2']));
+        $this->assertTrue(isset($weights['weightX']));
+
+        unset($weights['weightX']);
+        $this->assertCount(1, $weights);
+        $this->assertFalse(isset($weights['weightX']));
+        $this->assertTrue(isset($weights['weight2']));
+
+        $weight1->setIdentifier('weight2');
+        $this->assertFalse($weight1 === $weights['weight2']);
+    }
+
+    public function testRenamingOrder()
+    {
+        $weight1 = new Weight('weight1', 1.0);
+        $weight2 = new Weight('weight2', 1.2);
+        $weight3 = new Weight('weight3', 1.2);
+        $weights = new WeightCollection(array($weight1, $weight2, $weight3));
+
+        // If weight2 gets a new identifier "weight4", it should still be in second position in the collection.
+        $this->assertSame(
+            array('weight1', 'weight2', 'weight3'),
+            $weights->getKeys()
+        );
+
+        $weight2->setIdentifier('weight4');
+
+        $this->assertSame(
+            array('weight1', 'weight4', 'weight3'),
+            $weights->getKeys()
+        );
+    }
+}