Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-6631: Enriched TrashItem with removedLocationContentIdMap #388

Merged
merged 4 commits into from
Oct 19, 2023
Merged

IBX-6631: Enriched TrashItem with removedLocationContentIdMap #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfc6389
IBX-6631: Enriched `TrashItem` with `removedLocationContentIdMap`
barw4 Oct 11, 2023
94b7703
IBX-6631: Simplified the solution
barw4 Oct 12, 2023
8c7f28f
IBX-6631: Removed unnecessary assert
barw4 Oct 12, 2023
48730c1
IBX-6631: Added an integration test
barw4 Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions eZ/Publish/API/Repository/Tests/TrashServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,6 @@ public function testLoadTrashItem()
$trashItemReloaded->pathString
);

$this->assertEquals(
$trashItem,
$trashItemReloaded
);

$this->assertInstanceOf(
DateTime::class,
$trashItemReloaded->trashed
Expand Down Expand Up @@ -1041,6 +1036,31 @@ public function testDeleteThrowsNotFoundExceptionForNonExistingTrashItem()
));
}

public function testTrashProperlyAssignsRemovedLocationContentMapToTrashItem(): void
{
$repository = $this->getRepository();
$trashService = $repository->getTrashService();
$locationService = $repository->getLocationService();

$folder1 = $this->createFolder(['eng-GB' => 'Folder1'], 2);
$folder2 = $this->createFolder(['eng-GB' => 'Folder2'], $folder1->contentInfo->getMainLocationId());
$folder3 = $this->createFolder(['eng-GB' => 'Folder2'], $folder2->contentInfo->getMainLocationId());

$folderLocation = $locationService->loadLocation($folder1->contentInfo->getMainLocationId());

$trashItem = $trashService->trash($folderLocation);
$removedLocationContentMap = $trashItem->getRemovedLocationContentIdMap();

self::assertSame(
[
$folderLocation->id => $folder1->id,
$folder2->contentInfo->getMainLocationId() => $folder2->id,
$folder3->contentInfo->getMainLocationId() => $folder3->id,
],
$removedLocationContentMap,
);
}

/**
* @return array
*/
Expand Down
11 changes: 11 additions & 0 deletions eZ/Publish/API/Repository/Values/Content/TrashItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,15 @@ abstract class TrashItem extends Location
* @var \DateTime
*/
protected $trashed;

/** @var array<int, int> */
protected $removedLocationContentIdMap = [];

/**
* @return array<int, int>
*/
public function getRemovedLocationContentIdMap(): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://github.com/ezsystems/date-based-publisher/pull/267 I see that you are using this map to create content deletion notifications, and perform actual deletions.

Note that Content can have multiple locations, which means the same Content ID might occur in this map multiple times. Is this your intent?

Copy link
Member Author

@barw4 barw4 Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Steveb-p Yes, it can appear multiple times. Why would that be a problem? If one schedule entry is deleted, the second one won't be found and hence, won't be deleted. No exception is being thrown.

Copy link
Contributor

@Steveb-p Steveb-p Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is the case, then it is ok. I assumed it was a case that was not taken into account and the same action was taken for the same Content ID more than once.

You may consider swapping the map order. By having it be array<ContentID, array<LocationID>> (array with ContentID as keys and an array of LocationIDs as value) you would have a similar effect, with Location IDs being grouped together. Only a suggestion though, of course.

{
return $this->removedLocationContentIdMap;
}
}
11 changes: 10 additions & 1 deletion eZ/Publish/Core/Persistence/Legacy/Content/Location/Trash/Handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public function trashSubtree($locationId)
$locationRows = $this->locationGateway->getSubtreeContent($locationId);
$isLocationRemoved = false;
$parentLocationId = null;
$removedLocationsContentMap = [];

foreach ($locationRows as $locationRow) {
if ($locationRow['node_id'] == $locationId) {
Expand All @@ -117,6 +118,7 @@ public function trashSubtree($locationId)

if ($this->locationGateway->countLocationsByContentId($locationRow['contentobject_id']) == 1) {
$this->locationGateway->trashLocation($locationRow['node_id']);
$removedLocationsContentMap[(int)$locationRow['node_id']] = (int)$locationRow['contentobject_id'];
} else {
if ($locationRow['node_id'] == $locationId) {
$isLocationRemoved = true;
Expand All @@ -143,7 +145,14 @@ public function trashSubtree($locationId)
$this->locationHandler->markSubtreeModified($parentLocationId, time());
}

return $isLocationRemoved ? null : $this->loadTrashItem($locationId);
if ($isLocationRemoved === true) {
return null;
}

$trashItem = $this->loadTrashItem($locationId);
$trashItem->removedLocationContentIdMap = $removedLocationsContentMap;

return $trashItem;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion eZ/Publish/Core/Repository/TrashService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,10 @@ protected function buildDomainTrashItemObject(Trashed $spiTrashItem, Content $co
'depth' => $spiTrashItem->depth,
'sortField' => $spiTrashItem->sortField,
'sortOrder' => $spiTrashItem->sortOrder,
'trashed' => isset($spiTrashItem->trashed) ? new DateTime('@' . $spiTrashItem->trashed) : new DateTime('@0'),
'trashed' => isset($spiTrashItem->trashed)
? new DateTime('@' . $spiTrashItem->trashed)
: new DateTime('@0'),
'removedLocationContentIdMap' => $spiTrashItem->removedLocationContentIdMap,
'parentLocation' => $this->proxyDomainMapper->createLocationProxy($spiTrashItem->parentId),
]
);
Expand Down
3 changes: 3 additions & 0 deletions eZ/Publish/SPI/Persistence/Content/Location/Trashed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,7 @@ class Trashed extends Location
* @var mixed Trashed timestamp.
*/
public $trashed;

/** @var array<int, int> Location ID to a Content ID map of removed items */
public $removedLocationContentIdMap = [];
}
Loading