BUG Fix duplicate tag creation

BUG Fix isMultiple not being initialised BUG Fix tag rendering as a 100px wide container
silverstripe · Nov 4, 2015 · 2077c9d · 2077c9d
1 parent 847a237
commit 2077c9d
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 22 deletions.
diff --git a/code/StringTagField.php b/code/StringTagField.php
@@ -33,9 +33,9 @@ class StringTagField extends DropdownField {
 	protected $record;
 
 	/**
-	 * @var boolean
+	 * @var bool
 	 */
-	protected $isMultiple;
+	protected $isMultiple = true;
 
 	/**
 	 * @param string $name

diff --git a/code/TagField.php b/code/TagField.php
@@ -35,9 +35,9 @@ class TagField extends DropdownField {
 	protected $titleField = 'Title';
 
 	/**
-	 * @var string
+	 * @var bool
 	 */
-	protected $isMultiple;
+	protected $isMultiple = true;
 
 	/**
 	 * @param string $name
@@ -260,8 +260,6 @@ public function saveInto(DataObjectInterface $record) {
 
 		$source = $this->getSource();
 
-		$dataClass = $source->dataClass();
-
 		$values = $this->Value();
 
 		if(!$values) {
@@ -287,10 +285,8 @@ public function saveInto(DataObjectInterface $record) {
 					continue;
 				}
 
-				$record = new $dataClass();
-				$record->{$titleField} = $value;
-				$record->write();
-
+				// Get or create record
+				$record = $this->getOrCreateTag($value);
 				$values[$i] = $record->ID;
 			}
 		}
@@ -302,6 +298,31 @@ public function saveInto(DataObjectInterface $record) {
 		$relation->setByIDList(array_filter($values));
 	}
 
+	/**
+	 * Get or create tag with the given value
+	 *
+	 * @param string $term
+	 * @return DataObject
+	 */
+	protected function getOrCreateTag($term) {
+		// Check if existing record can be found
+		$source = $this->getSource();
+		$titleField = $this->getTitleField();
+		$record = $source
+			->filter($titleField, $term)
+			->first();
+		if($record) {
+			return $record;
+		}
+
+		// Create new instance if not yet saved
+		$dataClass = $source->dataClass();
+		$record = Injector::inst()->create($dataClass);
+		$record->{$titleField} = $term;
+		$record->write();
+		return $record;
+	}
+
 	/**
 	 * Returns a JSON string of tags, for lazy loading.
 	 *
@@ -334,28 +355,24 @@ protected function getTags($term) {
 
 		$titleField = $this->getTitleField();
 
-		$term = Convert::raw2sql($term);
-
 		$query = $source
 			->filter($titleField . ':PartialMatch:nocase', $term)
 			->sort($titleField)
 			->limit($this->getLazyLoadItemLimit());
 
+		// Map into a distinct list
 		$items = array();
-
+		$titleField = $this->getTitleField();
 		foreach($query->map('ID', $titleField) as $id => $title) {
-			if(!in_array($title, $items)) {
-				$items[] = array(
-					'id' => $id,
-					'text' => $title
-				);
-			}
+			$items[$title] = array(
+				'id' => $id,
+				'text' => $title
+			);
 		}
 
-		return $items;
+		return array_values($items);
 	}
 
-
 	/**
 	 * DropdownField assumes value will be a scalar so we must
 	 * override validate. This only applies to Silverstripe 3.2+

diff --git a/css/TagField.css b/css/TagField.css
@@ -1,6 +1,7 @@
 .select2-container {
     position: relative;
     max-width: 416px !important;
+    width: 100%;
 }
 
 .select2-selection {

diff --git a/tests/TagFieldTest.php b/tests/TagFieldTest.php
@@ -49,7 +49,17 @@ protected function getNewTagFieldTestBlogPost($name) {
 	 * @param TagFieldTestBlogPost $record
 	 */
 	protected function compareExpectedAndActualTags(array $expected, TagFieldTestBlogPost $record) {
-		$actual = array_values($record->Tags()->map('ID', 'Title')->toArray());
+		$this->compareTagLists($expected, $record->Tags());
+	}
+
+	/**
+	 * Ensure a source of tags matches the given string tag names
+	 *
+	 * @param array $expected
+	 * @param DataList $actualSource
+	 */
+	protected function compareTagLists(array $expected, DataList $actualSource) {
+		$actual = array_values($actualSource->map('ID', 'Title')->toArray());
 
 		sort($expected);
 		sort($actual);
@@ -103,6 +113,44 @@ public function testItSavesLinksToExistingTagsOnExistingRecords() {
 		);
 	}
 
+	/**
+	 * Ensure that {@see TagField::saveInto} respects existing tags
+	 */
+	public function testSaveDuplicateTags() {
+		$record = $this->getNewTagFieldTestBlogPost('BlogPost2');
+		$record->write();
+		$tag2ID = $this->idFromFixture('TagFieldTestBlogTag', 'Tag2');
+
+		// Check tags before write
+		$this->compareExpectedAndActualTags(
+			array('Tag1', 'Tag2'),
+			$record
+		);
+		$this->compareTagLists(
+			array('Tag1', 'Tag2'),
+			TagFieldTestBlogTag::get()
+		);
+		$this->assertContains($tag2ID, TagFieldTestBlogTag::get()->column('ID'));
+
+		// Write new tags
+		$field = new TagField('Tags', '', new DataList('TagFieldTestBlogTag'));
+		$field->setValue(array('Tag2', 'Tag3'));
+		$field->saveInto($record);
+
+		// Check only one new tag was added
+		$this->compareExpectedAndActualTags(
+			array('Tag2', 'Tag3'),
+			$record
+		);
+
+		// Ensure that only one new dataobject was added and that tag2s id has not changed
+		$this->compareTagLists(
+			array('Tag1', 'Tag2', 'Tag3'),
+			TagFieldTestBlogTag::get()
+		);
+		$this->assertContains($tag2ID, TagFieldTestBlogTag::get()->column('ID'));
+	}
+
 	function testItSuggestsTags() {
 		$field = new TagField('Tags', '', new DataList('TagFieldTestBlogTag'));