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.

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-8418: Remove draft when trashing its parent or ancestor location #398

Closed
wants to merge 53 commits into from

Conversation

barw4
Copy link
Contributor

@barw4 barw4 commented Jul 1, 2024

🎫 Issue IBX-8418

Related PRs:

ibexa/admin-ui#1321
https://github.com/ibexa/content-tree/pull/85

Description:

Currently, drafts without a location (so not yet published) that are orphaned due to missing ancestor location are forever stuck in the void. They cause multiple issues in different parts of DXP and they are not easily removable.

This PR makes sure that every time a location is trashed drafts under this given location or its child locations are removed.

Documentation:

Probably required

image

@barw4 barw4 added Bug Something isn't working Ready for review labels Jul 1, 2024
@barw4 barw4 self-assigned this Jul 1, 2024
@barw4 barw4 force-pushed the ibx-8418-deleting-orphaned-drafts branch from a70054e to 964cd70 Compare July 1, 2024 19:00
@barw4 barw4 force-pushed the ibx-8418-deleting-orphaned-drafts branch from 964cd70 to d737438 Compare July 2, 2024 09:54
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Ok, so I was thinking about something completely different during internal sync.

It's possible to create content items without Locations. Maybe in case of removal of a Location, just node assignment should be removed, keeping drafts "dangling"? These drafts should still be visible on Drafts view.

Just an idea. // POV ping @ibexa/php-dev

@alongosz alongosz requested a review from a team July 2, 2024 10:08
@barw4
Copy link
Contributor Author

barw4 commented Jul 2, 2024

@alongosz That's a cool idea, however, I think there are too many problems with drafts without node assignment. We cannot publish them using UI and I assume some other things will fail for them as well.

@barw4 barw4 force-pushed the ibx-8418-deleting-orphaned-drafts branch from f2e6dd0 to f80cb5c Compare July 7, 2024 12:52
@barw4 barw4 marked this pull request as ready for review July 8, 2024 08:57
@barw4 barw4 requested a review from alongosz July 8, 2024 08:57
@barw4 barw4 changed the title IBX-8418: Remove drafts when trashing its parent or ancestor location IBX-8418: Remove draft when trashing its parent or ancestor location Jul 8, 2024
{
$subLocations = $this->locationGateway->getChildren($locationId);
foreach ($subLocations as $subLocation) {
$this->deleteChildrenDrafts($subLocation['node_id']);
Copy link
Contributor

Choose a reason for hiding this comment

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

A recursive call? Won't this be a massive performance penalty for deep trees?

Copy link
Contributor Author

@barw4 barw4 Aug 2, 2024

Choose a reason for hiding this comment

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

Few lines above we have the same situation and it was there for ages 🤷🏻 : https://github.com/ibexa/core/blob/main/src/lib/Persistence/Legacy/Content/TreeHandler.php#L196

Removing it in a single query may not be easy as this table is tree-like with the only information being of a record's parent.

@barw4 barw4 requested a review from Steveb-p August 2, 2024 12:58
@alongosz alongosz requested a review from a team August 5, 2024 12:31
@barw4 barw4 requested a review from alongosz August 6, 2024 12:37
@konradoboza konradoboza requested a review from a team August 7, 2024 05:54
@barw4 barw4 requested a review from konradoboza August 7, 2024 07:38
alongosz and others added 23 commits October 17, 2024 18:23
For more details see #406

Key changes:

* Extracted common base for TextBlock and TextLine field types

* [Tests] Aligned tests with TextBlock and TextLine changes

* [Tests] Reduced complexity of TextBlock and TextLine test classes

* [PHPStan] Aligned baseline with the changes

---------

Co-authored-by: Paweł Niedzielski <[email protected]>
For more details see #407

Key changes:

* Extracted redundant Binary and Media SearchField into a common base

* [Tests] Extracted common base for Binary and Media ft integration tests

* [Tests] Made MediaIntegrationTest::getValidFieldSettings return type more strict

* [PHPStan] Aligned baseline with the changes

---------

Co-authored-by: Adam Wójs <[email protected]>
Co-authored-by: Mikolaj Adamczyk <[email protected]>
For more details see #424

Key changes:

* Extracted abstract `AffixBasedTextMatcher` for redundant Host & URI text matchers

* [PHPStan] Aligned baseline with the changes

---------

Co-authored-by: Adam Wójs <[email protected]>
Co-authored-by: Konrad Oboza <[email protected]>
…425)

For more details see #425

Key changes:

* Refactored Float and Integer field types to use external validators

* [Tests] Refactored Float, Integer, and String validator tests

---------

Co-authored-by: Adam Wójs <[email protected]>
Co-authored-by: Paweł Niedzielski <[email protected]>
For more details see https://issues.ibexa.co/browse/IBX-8138 and #385

Key changes:

* [Rector] Applied Symfony 5.1 CommandConstantReturnCodeRector

* [Rector] Applied Symfony 5.2 RenameMethodRector

* [Rector] Applied Symfony 5.3 Rector sets

* [Rector] Applied Symfony code quality Rector sets

  Applied rules:
    * LiteralGetToRequestClassConstantRector
    * ResponseStatusCodeRector
    * MakeCommandLazyRector

* [Rector] Applied Return type rectors

  Applied rules:
    * ReturnTypeFromStrictNativeCallRector
    * ReturnTypeFromStrictScalarReturnExprRector

* [Rector] Applied Symfony 6.0 AddReturnTypeDeclarationRector

* [Rector] Added Symfony Bundle::getContainerExtension return type

* Added strict types for InstallPlatformCommand consts

* Made XML serialization exception more verbose in Author and DateTime converters

* Implemented `\Ibexa\Core\MVC\Symfony\Security\User::getUserIdentifier`

---------

Co-Authored-By: Paweł Niedzielski <[email protected]>
Co-Authored-By: Konrad Oboza <[email protected]>
)

For more details see #427

Key changes:

* Fixed strict types for URLChecker HTTPHandler::createCurlHandlerForUrl

* Introduced strict getters for URL ValueObject

* Improved LogicException message

* [PHPStan] Aligned baseline with the changes
@barw4 barw4 force-pushed the ibx-8418-deleting-orphaned-drafts branch from 0352c82 to 9c886eb Compare October 17, 2024 16:24
Copy link

sonarcloud bot commented Oct 17, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
8.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@barw4 barw4 closed this Oct 17, 2024
@barw4 barw4 deleted the ibx-8418-deleting-orphaned-drafts branch October 17, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Ready for QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.