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-6630: Allowed injecting view type into content preview controller #285

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Oct 19, 2023

Question Answer
JIRA issue IBX-6630
Required by ibexa/page-builder#273
Type feature
Target Ibexa version v4.6
BC breaks no

This PR allows passing optional {viewType} route attribute query parameter for core content preview controller (ibexa.controller.content.preview:previewContentAction).

Preview Controller builds parameters for ViewController::viewAction and forwards request to that controller. It seems full view type is hardcoded there. Till now it was enough because type of template for preview was based solely on a SiteAccess and the same was used both for view and preview.

However, there are some cases, like back-office customizable Dashboard when we have 2 types of content views needed: regular AdminUI content/location view and the actual dashboard page preview also displayed in the context of admin SiteAccess. This enhancement adds more flexibility to other use cases apart from closed-source Dashboard.

Update

For a good beginning of the weekend, I refactored PreviewControllerTest to actually test what it's supposed to test, in a more compact and modern way. I recommend either reviewing the whole test class or just the diff for the test of the essence of the current changes (6ed7e4e).

TODO

  • Update \Ibexa\Tests\Core\MVC\Symfony\Controller\Controller\Content\PreviewControllerTest.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review.

@alongosz alongosz added the Feature New feature request label Oct 19, 2023
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

Aside remarks from @Steveb-p

Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Apart from Paweł's comments which I agree with.

@konradoboza konradoboza requested a review from a team October 20, 2023 06:21
@alongosz alongosz force-pushed the ibx-6630-allow-injecting-view-type-in-preview branch from 69af4ff to 64a8857 Compare October 20, 2023 16:15
@@ -161,7 +161,7 @@ protected function getForwardRequest(Location $location, Content $content, SiteA
],
'location' => $location,
'content' => $content,
'viewType' => ViewManagerInterface::VIEW_TYPE_FULL,
'viewType' => $viewType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, do we even need to be aware of the defaults both here and in the caller? Doesn't admin UI fall back into a full view on it's own? If so, you might want to remove the default and just pass null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity, do we even need to be aware of the defaults both here and in the caller? Doesn't admin UI fall back into a full view on it's own? If so, you might want to remove the default and just pass null.

viewType is a core concept. Even if somehow AdminUI does this on its own, this would be a break in core behavior for anyone directly using that controller. While there's no BC promise on that, since the class is neither final nor internal, this is very low cost to at least try keeping that BC.

@alongosz alongosz force-pushed the ibx-6630-allow-injecting-view-type-in-preview branch from a5154fe to 37f1de0 Compare October 23, 2023 20:15
@sonarcloud
Copy link

sonarcloud bot commented Oct 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alongosz alongosz requested a review from a team October 25, 2023 11:12
@alongosz
Copy link
Member Author

alongosz commented Nov 6, 2023

Merging to unblock a lot of other dependent Pull Requests. Will be sent to QA with Dashboard feature when ready. For now performed some manual tests to make sure it doesn't break anything.

@alongosz alongosz merged commit a945d7a into main Nov 6, 2023
26 checks passed
@alongosz alongosz deleted the ibx-6630-allow-injecting-view-type-in-preview branch November 6, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature request Ready for QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants