From c74f7e764003f7b9cc2087d49f63dd73ca40ac4f Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 12 Jul 2013 15:18:06 +1200 Subject: [PATCH] BUG Fixes issue where items could be deleted from a has_many relation by an entirely unrelated HasManyList calling delete on that item. --- model/HasManyList.php | 21 ++++++++++++------ tests/model/DataObjectTest.php | 26 ++++++++++++++-------- tests/model/HasManyListTest.php | 39 +++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 16 deletions(-) diff --git a/model/HasManyList.php b/model/HasManyList.php index 7bcb3412f..9dc193b7b 100644 --- a/model/HasManyList.php +++ b/model/HasManyList.php @@ -16,9 +16,8 @@ class HasManyList extends RelationList { * {@link DataList} methods. Addition arguments are used to support {@@link add()} * and {@link remove()} methods. * - * @param $dataClass The class of the DataObjects that this will list. - * @param $relationFilters A map of key => value filters that define which records - * in the $dataClass table actually belong to this relationship. + * @param string $dataClass The class of the DataObjects that this will list. + * @param string $foreignKey The name of the foreign key field to set the ID filter against. */ public function __construct($dataClass, $foreignKey) { parent::__construct($dataClass); @@ -96,6 +95,7 @@ class HasManyList extends RelationList { /** * Remove an item from this relation. * Doesn't actually remove the item, it just clears the foreign key value. + * * @param $item The DataObject to be removed * @todo Maybe we should delete the object instead? */ @@ -105,10 +105,17 @@ class HasManyList extends RelationList { E_USER_ERROR); } - $fk = $this->foreignKey; - $item->$fk = null; + // Don't remove item which doesn't belong to this list + $foreignID = $this->getForeignID(); + $foreignKey = $this->getForeignKey(); + + if( empty($foreignID) + || (is_array($foreignID) && in_array($item->$foreignKey, $foreignID)) + || $foreignID == $item->$foreignKey + ) { + $item->$foreignKey = null; + $item->write(); + } - $item->write(); } - } diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 7c98aac3a..7f5688a40 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -251,14 +251,14 @@ class DataObjectTest extends SapphireTest { * - Test the IDs on the DataObjects are set correctly */ public function testHasManyRelationships() { - $team = $this->objFromFixture('DataObjectTest_Team', 'team1'); + $team1 = $this->objFromFixture('DataObjectTest_Team', 'team1'); // Test getComponents() gets the ComponentSet of the other side of the relation - $this->assertTrue($team->Comments()->Count() == 2); + $this->assertTrue($team1->Comments()->Count() == 2); // Test the IDs on the DataObjects are set correctly - foreach($team->Comments() as $comment) { - $this->assertEquals($team->ID, $comment->TeamID); + foreach($team1->Comments() as $comment) { + $this->assertEquals($team1->ID, $comment->TeamID); } // Test that we can add and remove items that already exist in the database @@ -266,15 +266,23 @@ class DataObjectTest extends SapphireTest { $newComment->Name = "Automated commenter"; $newComment->Comment = "This is a new comment"; $newComment->write(); - $team->Comments()->add($newComment); - $this->assertEquals($team->ID, $newComment->TeamID); + $team1->Comments()->add($newComment); + $this->assertEquals($team1->ID, $newComment->TeamID); $comment1 = $this->objFromFixture('DataObjectTest_TeamComment', 'comment1'); $comment2 = $this->objFromFixture('DataObjectTest_TeamComment', 'comment2'); - $team->Comments()->remove($comment2); + $team1->Comments()->remove($comment2); - $commentIDs = $team->Comments()->sort('ID')->column('ID'); - $this->assertEquals(array($comment1->ID, $newComment->ID), $commentIDs); + $team1CommentIDs = $team1->Comments()->sort('ID')->column('ID'); + $this->assertEquals(array($comment1->ID, $newComment->ID), $team1CommentIDs); + + // Test that removing an item from a list doesn't remove it from the same + // relation belonging to a different object + $team1 = $this->objFromFixture('DataObjectTest_Team', 'team1'); + $team2 = $this->objFromFixture('DataObjectTest_Team', 'team2'); + $team2->Comments()->remove($comment1); + $team1CommentIDs = $team1->Comments()->sort('ID')->column('ID'); + $this->assertEquals(array($comment1->ID, $newComment->ID), $team1CommentIDs); } public function testHasOneRelationship() { diff --git a/tests/model/HasManyListTest.php b/tests/model/HasManyListTest.php index 5f54d4214..ccce52611 100644 --- a/tests/model/HasManyListTest.php +++ b/tests/model/HasManyListTest.php @@ -16,5 +16,44 @@ class HasManyListTest extends SapphireTest { $newTeam = new DataObjectTest_Team(); // has_many Comments $this->assertEquals(array(), $newTeam->Comments()->column('ID')); } + + /** + * Test that related objects can be removed from a relation + */ + public function testRemoveRelation() { + + // Check that expected teams exist + $list = DataObjectTest_Team::get(); + $this->assertEquals( + array('Subteam 1', 'Subteam 2', 'Subteam 3', 'Team 1', 'Team 2', 'Team 3'), + $list->sort('Title')->column('Title') + ); + + // Test that each team has the correct fans + $team1 = $this->objFromFixture('DataObjectTest_Team', 'team1'); + $team2 = $this->objFromFixture('DataObjectTest_Team', 'team2'); + $this->assertEquals(array('Bob', 'Joe'), $team1->Comments()->sort('Name')->column('Name')); + $this->assertEquals(array('Phil'), $team2->Comments()->sort('Name')->column('Name')); + + // Test that removing comments from unrelated team has no effect + $team1comment = $this->objFromFixture('DataObjectTest_TeamComment', 'comment1'); + $team2comment = $this->objFromFixture('DataObjectTest_TeamComment', 'comment3'); + $team1->Comments()->remove($team2comment); + $team2->Comments()->remove($team1comment); + $this->assertEquals(array('Bob', 'Joe'), $team1->Comments()->sort('Name')->column('Name')); + $this->assertEquals(array('Phil'), $team2->Comments()->sort('Name')->column('Name')); + $this->assertEquals($team1->ID, $team1comment->TeamID); + $this->assertEquals($team2->ID, $team2comment->TeamID); + + // Test that removing items from the related team resets the has_one relations on the fan + $team1comment = $this->objFromFixture('DataObjectTest_TeamComment', 'comment1'); + $team2comment = $this->objFromFixture('DataObjectTest_TeamComment', 'comment3'); + $team1->Comments()->remove($team1comment); + $team2->Comments()->remove($team2comment); + $this->assertEquals(array('Bob'), $team1->Comments()->sort('Name')->column('Name')); + $this->assertEquals(array(), $team2->Comments()->sort('Name')->column('Name')); + $this->assertEmpty($team1comment->TeamID); + $this->assertEmpty($team2comment->TeamID); + } }