Skip to content

Commit

Permalink
Merge pull request #423 from creative-commoners/pulls/1.13/remove-imp…
Browse files Browse the repository at this point in the history 
…licitly-added-item

FIX Remove implicitly added item
  • Loading branch information
Maxime Rainville authored Nov 1, 2023
2 parents 302aa4c + d614204 commit 59299b6
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 5 deletions.
16 changes: 16 additions & 0 deletions src/ChangeSet.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,22 @@ public function removeObject(DataObject $object)
// TODO: Handle case of implicit added item being removed.

$item->delete();

// Get the implicitly included items for this ChangeSet
$implicit = $this->calculateImplicit();

foreach ($this->Changes()->filter(['Added' => ChangeSetItem::IMPLICITLY]) as $changeSetItem) {
$objectKey = $this->implicitKey($changeSetItem);

// If a ChangeSetItem exists, but isn't in $implicit, it's no longer required, so delete it
if (!array_key_exists($objectKey, $implicit)) {
$changeSetItem->delete();
} else {
// Otherwise it is required, so update ReferencedBy and remove from $implicit
$changeSetItem->ReferencedBy()->setByIDList($implicit[$objectKey]['ReferencedBy']);
unset($implicit[$objectKey]);
}
}
}

$this->sync();
Expand Down
118 changes: 113 additions & 5 deletions tests/php/ChangeSetTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ public function testRepeatedSyncIsNOP()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand All @@ -172,6 +173,7 @@ public function testRepeatedSyncIsNOP()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand All @@ -196,6 +198,7 @@ public function testSync()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand All @@ -214,6 +217,7 @@ public function testSync()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand Down Expand Up @@ -255,6 +259,7 @@ public function testSync()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand Down Expand Up @@ -301,6 +306,7 @@ public function testIsSynced()
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
Expand All @@ -319,6 +325,7 @@ public function testIsSynced()
[
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
);
Expand All @@ -334,7 +341,7 @@ public function testCanPublish()
$base = $this->objFromFixture(ChangeSetTest\BaseObject::class, 'base');
$changeSet->addObject($base);
$changeSet->sync();
$this->assertEquals(5, $changeSet->Changes()->count());
$this->assertEquals(6, $changeSet->Changes()->count());

// Test un-authenticated user cannot publish
$this->logOut();
Expand Down Expand Up @@ -378,7 +385,7 @@ public function testCanPublishNested()
$changeSet->addObject($base);
$changeSet->addObject($mid1);
$changeSet->sync();
$this->assertEquals(5, $changeSet->Changes()->count());
$this->assertEquals(6, $changeSet->Changes()->count());

// With model publish permissions only publish is allowed
$this->assertTrue($changeSet->canPublish());
Expand Down Expand Up @@ -434,7 +441,7 @@ public function testCanEdit()
$base = $this->objFromFixture(ChangeSetTest\BaseObject::class, 'base');
$changeSet->addObject($base);
$changeSet->sync();
$this->assertEquals(5, $changeSet->Changes()->count());
$this->assertEquals(6, $changeSet->Changes()->count());

// Check canEdit
$this->logOut();
Expand Down Expand Up @@ -465,7 +472,7 @@ public function testCanDelete()
$base = $this->objFromFixture(ChangeSetTest\BaseObject::class, 'base');
$changeSet->addObject($base);
$changeSet->sync();
$this->assertEquals(5, $changeSet->Changes()->count());
$this->assertEquals(6, $changeSet->Changes()->count());

// Check canDelete
$this->logOut();
Expand All @@ -485,7 +492,7 @@ public function testCanView()
$base = $this->objFromFixture(ChangeSetTest\BaseObject::class, 'base');
$changeSet->addObject($base);
$changeSet->sync();
$this->assertEquals(5, $changeSet->Changes()->count());
$this->assertEquals(6, $changeSet->Changes()->count());

// Check canView
$this->logOut();
Expand Down Expand Up @@ -617,6 +624,7 @@ public function testUnlinkDisassociated()
[
ChangeSetTest\BaseObject::class . '.base' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
);
Expand Down Expand Up @@ -735,4 +743,104 @@ public function testIsSyncedCanBeSkipped()

$this->assertFalse($changeset->isSyncCalled, 'isSynced is skipped when providing truthy argument to publish');
}

public function testRemoveObject()
{
$this->publishAllFixtures();

$mid1 = $this->objFromFixture(ChangeSetTest\MidObject::class, 'mid1'); // Item mid2 reference Item end2

$changeset = new ChangeSet();
$changeset->write();
$changeset->addObject($mid1);
$changeset->publish();

$this->assertChangeSetLooksLike(
$changeset,
[
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
]
);

$changeset->removeObject($mid1); // Remove mid1

$this->assertChangeSetLooksLike(
$changeset,
[]
);
}

public function testRemoveObjectsWithReferencesToOneItem()
{
$this->publishAllFixtures();

$mid2 = $this->objFromFixture(ChangeSetTest\MidObject::class, 'mid2'); // Item mid2 reference Item end2
$mid3 = $this->objFromFixture(ChangeSetTest\MidObject::class, 'mid4'); // Item mid4 reference Item end2

$changeset = new ChangeSet();
$changeset->write();
$changeset->addObject($mid2);
$changeset->addObject($mid3);
$changeset->publish();

$this->assertChangeSetLooksLike(
$changeset,
[
ChangeSetTest\MidObject::class . '.mid2' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY,
]
);

$changeset->removeObject($mid2); // Remove mid2

$this->assertChangeSetLooksLike(
$changeset,
[
ChangeSetTest\MidObject::class . '.mid4' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\EndObject::class . '.end2' => ChangeSetItem::IMPLICITLY, // Item end2 is still in ChangeSet
]
);

$changeset->removeObject($mid3); // Remove mid3

$this->assertChangeSetLooksLike(
$changeset,
[]
);
}

public function testRemoveObjectWithImplicitlyAddedItemOnly()
{
$this->publishAllFixtures();

$mid1 = $this->objFromFixture(ChangeSetTest\MidObject::class, 'mid1'); // Item mid1 reference Item end1
$end1 = $this->objFromFixture(ChangeSetTest\EndObject::class, 'end1'); // Item end1

$changeset = new ChangeSet();
$changeset->write();
$changeset->addObject($mid1);
$changeset->addObject($end1); // Item end1 explicitly added to ChangeSet
$changeset->publish();

$this->assertChangeSetLooksLike(
$changeset,
[
ChangeSetTest\MidObject::class . '.mid1' => ChangeSetItem::EXPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::IMPLICITLY,
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::EXPLICITLY
]
);

$changeset->removeObject($mid1); // Remove mid1

$this->assertChangeSetLooksLike(
$changeset,
[
ChangeSetTest\EndObject::class . '.end1' => ChangeSetItem::EXPLICITLY // Item end1 is still in ChangeSet
]
);
}
}
4 changes: 4 additions & 0 deletions tests/php/ChangeSetTest.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ SilverStripe\Versioned\Tests\ChangeSetTest\MidObject:
End: =>SilverStripe\Versioned\Tests\ChangeSetTest\EndObject.end2
mid3:
Base: =>SilverStripe\Versioned\Tests\ChangeSetTest\BaseObject.base2
mid4:
Bar: 3
Base: =>SilverStripe\Versioned\Tests\ChangeSetTest\BaseObject.base
End: =>SilverStripe\Versioned\Tests\ChangeSetTest\EndObject.end2
SilverStripe\Versioned\Tests\ChangeSetTest\UnversionedObject:
unversioned1:
Title: 'object'
Expand Down

0 comments on commit 59299b6

Please sign in to comment.