-
Notifications
You must be signed in to change notification settings - Fork 2
IBX-11780: Integrated ContentPublicationStrategyInterface into ContentFormProcessor publish flow #113
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
Open
bnowak
wants to merge
3
commits into
6.0
Choose a base branch
from
IBX-11780-integrated-content-publication-strategy-into-ContentFormProcessor
base: 6.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
IBX-11780: Integrated ContentPublicationStrategyInterface into ContentFormProcessor publish flow #113
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,321 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\ContentForms\Form\Processor; | ||
|
|
||
| use Ibexa\ContentForms\Data\Content\ContentCreateData; | ||
| use Ibexa\ContentForms\Data\Content\ContentUpdateData; | ||
| use Ibexa\ContentForms\Event\FormActionEvent; | ||
| use Ibexa\ContentForms\Form\Processor\ContentFormProcessor; | ||
| use Ibexa\Contracts\Core\Repository\ContentService; | ||
| use Ibexa\Contracts\Core\Repository\LocationService; | ||
| use Ibexa\Contracts\Core\Repository\Strategy\ContentPublication\ContentPublicationResult; | ||
| use Ibexa\Contracts\Core\Repository\Strategy\ContentPublication\ContentPublicationStrategyInterface; | ||
| use Ibexa\Contracts\Core\Repository\Values\Content\Content; | ||
| use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; | ||
| use Ibexa\Contracts\Core\Repository\Values\Content\Language; | ||
| use Ibexa\Contracts\Core\Repository\Values\Content\VersionInfo; | ||
| use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType; | ||
| use Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface; | ||
| use Ibexa\Core\Repository\Values\Content\Location; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Symfony\Component\Form\FormConfigInterface; | ||
| use Symfony\Component\Form\FormInterface; | ||
| use Symfony\Component\HttpFoundation\RedirectResponse; | ||
| use Symfony\Component\Routing\RouterInterface; | ||
|
|
||
| /** | ||
| * @covers \Ibexa\ContentForms\Form\Processor\ContentFormProcessor | ||
| */ | ||
| final class ContentFormProcessorTest extends TestCase | ||
| { | ||
| private const int CONTENT_ID = 123; | ||
| private const int DRAFT_MAIN_LOCATION_ID = 42; | ||
| private const int PUBLISHED_MAIN_LOCATION_ID = 77; | ||
| private const int REFERRER_LOCATION_ID = 55; | ||
| private const int TREE_ROOT_LOCATION_ID = 2; | ||
| private const string LANGUAGE_CODE = 'eng-GB'; | ||
| private const string GENERATED_URL = 'generated-url'; | ||
|
|
||
| /** | ||
| * @dataProvider provideProcessPublishCases | ||
| * | ||
| * @param array<string, int|null>|null $expectedRouteParameters route parameters the redirect | ||
| * is expected to be generated with, or null when a custom redirect URL is provided and | ||
| * the router must not be invoked at all | ||
| */ | ||
| public function testProcessPublish( | ||
| bool $isNewContent, | ||
| bool $publishedSynchronously, | ||
| bool $withReferrerLocation, | ||
| ?string $customRedirectUrl, | ||
| ?array $expectedRouteParameters | ||
| ): void { | ||
| $draft = $this->createDraft( | ||
| $isNewContent ? null : self::DRAFT_MAIN_LOCATION_ID, | ||
| $isNewContent ? ContentInfo::STATUS_DRAFT : ContentInfo::STATUS_PUBLISHED, | ||
| ); | ||
| $versionInfo = $draft->getVersionInfo(); | ||
|
|
||
| $data = $isNewContent ? $this->createCreateData() : $this->createUpdateData($draft); | ||
| $options = $withReferrerLocation | ||
| ? ['referrerLocation' => new Location(['id' => self::REFERRER_LOCATION_ID])] | ||
| : []; | ||
| $event = $this->createEvent($data, $this->createForm($customRedirectUrl), $options); | ||
|
|
||
| // The draft is persisted via create for new content or update for existing content; | ||
| // publication must never go through the service. | ||
| $contentService = $this->createMock(ContentService::class); | ||
| $contentService | ||
| ->expects($isNewContent ? self::once() : self::never()) | ||
| ->method('createContent') | ||
| ->willReturn($draft); | ||
| $contentService | ||
| ->expects($isNewContent ? self::never() : self::once()) | ||
| ->method('updateContent') | ||
| ->willReturn($draft); | ||
| $contentService | ||
| ->expects(self::never()) | ||
| ->method('publishVersion'); | ||
|
|
||
| // Publication is delegated to the strategy, called once with the draft version and language. | ||
| $publishedContent = $publishedSynchronously ? $this->createPublishedContent() : null; | ||
| $contentPublicationStrategy = $this->createMock(ContentPublicationStrategyInterface::class); | ||
| $contentPublicationStrategy | ||
| ->expects(self::once()) | ||
| ->method('publishVersion') | ||
| ->with(self::identicalTo($versionInfo), [self::LANGUAGE_CODE]) | ||
| ->willReturn(new ContentPublicationResult($publishedContent)); | ||
|
|
||
| // The router is called once with the expected route parameters, unless a custom redirect | ||
| // URL coming from the form data short-circuits URL generation entirely. | ||
| $router = $this->createMock(RouterInterface::class); | ||
| if ($expectedRouteParameters === null) { | ||
| $router | ||
| ->expects(self::never()) | ||
| ->method('generate'); | ||
| } else { | ||
| $router | ||
| ->expects(self::once()) | ||
| ->method('generate') | ||
| ->with('ibexa.content.view', $expectedRouteParameters) | ||
| ->willReturn(self::GENERATED_URL); | ||
| } | ||
|
|
||
| // The deferred (async) redirect resolves its location from the content tree root config | ||
| // and the location lookup. | ||
| $configResolver = $this->createStub(ConfigResolverInterface::class); | ||
| $configResolver->method('getParameter')->willReturn(self::TREE_ROOT_LOCATION_ID); | ||
|
|
||
| $locationService = $this->createStub(LocationService::class); | ||
| $locationService->method('loadLocation')->willReturn( | ||
| new Location([ | ||
| 'id' => self::TREE_ROOT_LOCATION_ID, | ||
| 'contentInfo' => new ContentInfo(['id' => self::CONTENT_ID]), | ||
| ]) | ||
| ); | ||
|
|
||
| $processor = new ContentFormProcessor( | ||
| $contentService, | ||
| $locationService, | ||
| $router, | ||
| $configResolver, | ||
| $contentPublicationStrategy | ||
| ); | ||
|
|
||
| $processor->processPublish($event); | ||
|
|
||
| self::assertSame($draft->getContentType(), $event->getPayload('content_type')); | ||
| self::assertSame($isNewContent, $event->getPayload('is_new')); | ||
|
|
||
| $response = $event->getResponse(); | ||
| self::assertInstanceOf(RedirectResponse::class, $response); | ||
| self::assertSame($customRedirectUrl ?? self::GENERATED_URL, $response->getTargetUrl()); | ||
| } | ||
|
|
||
| /** | ||
| * @return iterable<array{ | ||
| * isNewContent: bool, | ||
| * publishedSynchronously: bool, | ||
| * withReferrerLocation: bool, | ||
| * customRedirectUrl: string|null, | ||
| * expectedRouteParameters: array{ | ||
| * contentId: int, | ||
| * locationId: int, | ||
| * publishedContentId: int | ||
| * }|null | ||
| * }> | ||
| */ | ||
| public static function provideProcessPublishCases(): iterable | ||
| { | ||
| yield 'sync: update with referrer location' => [ | ||
| 'isNewContent' => false, | ||
| 'publishedSynchronously' => true, | ||
| 'withReferrerLocation' => true, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::REFERRER_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'sync: update without referrer location' => [ | ||
| 'isNewContent' => false, | ||
| 'publishedSynchronously' => true, | ||
| 'withReferrerLocation' => false, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::PUBLISHED_MAIN_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'sync: new content' => [ | ||
| 'isNewContent' => true, | ||
| 'publishedSynchronously' => true, | ||
| 'withReferrerLocation' => false, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::PUBLISHED_MAIN_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'async: update with referrer location' => [ | ||
| 'isNewContent' => false, | ||
| 'publishedSynchronously' => false, | ||
| 'withReferrerLocation' => true, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::REFERRER_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'async: update without referrer location redirects to tree root' => [ | ||
| 'isNewContent' => false, | ||
| 'publishedSynchronously' => false, | ||
| 'withReferrerLocation' => false, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::TREE_ROOT_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'async: new content without location yet redirects to tree root' => [ | ||
| 'isNewContent' => true, | ||
| 'publishedSynchronously' => false, | ||
| 'withReferrerLocation' => false, | ||
| 'customRedirectUrl' => null, | ||
| 'expectedRouteParameters' => [ | ||
| 'contentId' => self::CONTENT_ID, | ||
| 'locationId' => self::TREE_ROOT_LOCATION_ID, | ||
| 'publishedContentId' => self::CONTENT_ID, | ||
| ], | ||
| ]; | ||
|
|
||
| yield 'custom redirect URL after publish bypasses router' => [ | ||
| 'isNewContent' => false, | ||
| 'publishedSynchronously' => false, | ||
| 'withReferrerLocation' => false, | ||
| 'customRedirectUrl' => 'custom-redirect-url', | ||
| 'expectedRouteParameters' => null, | ||
| ]; | ||
| } | ||
|
|
||
| private function createDraft(?int $mainLocationId, int $status): Content | ||
| { | ||
| $contentInfo = new ContentInfo([ | ||
| 'id' => self::CONTENT_ID, | ||
| 'mainLocationId' => $mainLocationId, | ||
| 'mainLanguageCode' => self::LANGUAGE_CODE, | ||
| 'status' => $status, | ||
| ]); | ||
|
|
||
| $versionInfo = $this->createStub(VersionInfo::class); | ||
| $versionInfo->method('getInitialLanguage')->willReturn( | ||
| new Language(['languageCode' => self::LANGUAGE_CODE]) | ||
| ); | ||
| $versionInfo->method('getContentInfo')->willReturn($contentInfo); | ||
|
|
||
| $draft = $this->createStub(Content::class); | ||
| $draft->method('getVersionInfo')->willReturn($versionInfo); | ||
| $draft->method('getContentInfo')->willReturn($contentInfo); | ||
| $draft->method('getContentType')->willReturn($this->createStub(ContentType::class)); | ||
|
|
||
| return $draft; | ||
| } | ||
|
|
||
| private function createPublishedContent(): Content | ||
| { | ||
| $contentInfo = new ContentInfo([ | ||
| 'id' => self::CONTENT_ID, | ||
| 'mainLocationId' => self::PUBLISHED_MAIN_LOCATION_ID, | ||
| 'mainLanguageCode' => self::LANGUAGE_CODE, | ||
| 'status' => ContentInfo::STATUS_PUBLISHED, | ||
| ]); | ||
|
|
||
| $publishedContent = $this->createStub(Content::class); | ||
| $publishedContent->method('getContentInfo')->willReturn($contentInfo); | ||
| $publishedContent->method('getId')->willReturn(self::CONTENT_ID); | ||
|
|
||
| return $publishedContent; | ||
| } | ||
|
|
||
| private function createUpdateData(Content $contentDraft): ContentUpdateData | ||
| { | ||
| return new ContentUpdateData([ | ||
| 'contentDraft' => $contentDraft, | ||
| 'fieldsData' => [], | ||
| ]); | ||
| } | ||
|
|
||
| private function createCreateData(): ContentCreateData | ||
| { | ||
| return new ContentCreateData([ | ||
| 'mainLanguageCode' => self::LANGUAGE_CODE, | ||
| 'fieldsData' => [], | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * @return \Symfony\Component\Form\FormInterface<mixed> | ||
| */ | ||
| private function createForm(?string $redirectUrlAfterPublish = null): FormInterface | ||
| { | ||
| $formConfig = $this->createStub(FormConfigInterface::class); | ||
| $formConfig->method('getOption')->willReturn(self::LANGUAGE_CODE); | ||
|
|
||
| $redirectUrlField = $this->createStub(FormInterface::class); | ||
| $redirectUrlField->method('getData')->willReturn($redirectUrlAfterPublish); | ||
|
|
||
| $form = $this->createStub(FormInterface::class); | ||
| $form->method('getConfig')->willReturn($formConfig); | ||
| $form->method('offsetGet')->willReturn($redirectUrlField); | ||
|
|
||
| return $form; | ||
| } | ||
|
|
||
| /** | ||
| * @param \Symfony\Component\Form\FormInterface<mixed> $form | ||
| * @param array<string, mixed> $options | ||
| */ | ||
| private function createEvent( | ||
| ContentCreateData|ContentUpdateData $data, | ||
| FormInterface $form, | ||
| array $options = [] | ||
| ): FormActionEvent { | ||
| return new FormActionEvent($form, $data, 'publish', $options); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it has been agreed with Product Team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only thing changed for user. In async publication mode, when user is creating a new content and clicks publish right away - we don't have any content location to redirect to yet.
It was agreed with @konradoboza and @kmadejski, but unfortunately I don't see any other alternative here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is at least one place where we store from where user came in and we redirect him to that afterwards. Not sure if this would apply here or if its even worth the effort if we agreed on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there's option to pass
$referrerLocationto event from place where it's triggered. However, it's not always available (the fact if it's given/passed to event or not, is also based on some logic/use-case). If I'd rely on that, I'd need to have information there if we're publishing in sync or async mode (which currently is not available outside of this processor).With the general rule that we cannot change how sync mode works, I adjusted only async path here as a consequence. Although I also think it could be done more simply, considering some wider refactor.