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

Cms page fixes #13144

Merged
merged 19 commits into from Apr 10, 2019
Merged

Cms page fixes #13144

merged 19 commits into from Apr 10, 2019

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Apr 2, 2019

Questions Answers
Branch? 1.7.6.x
Description? Adds save and preview button in create/edit cms page form and missing constraints in cmsPageCategoryType (Added TypedRegexConstraints and missing Length constraints) . Refactors CmsPageController to handle exceptions properly. (Avoid code replication from FrameworkBundleAdminController->getErrorMessageForException which wasn't implemented before).
cms page.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Go to Improve > design > pages, use interface buttons to create or edit cms page. Once form is ready to submit, press Save and preview button. You should be redirected to edit page (same cms page that you previously edited or created) new window should popup with front office of that edited/created cms page. Make sure that browser doesn't block popup, else you will see native browser warning of blocked popup window, but new window won't open. In cmsCategory forms added field length constraints as in legacy code (for example name field max 64 chars). Everything else should work as before. ⚠️ Assets may need to be rebuilt.

This change is Reviewable

@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring Waiting for wording Status: action required, waiting for wording labels Apr 2, 2019
@zuk3975 zuk3975 changed the title Cms page create/edit action "save and preview" button migration [WIP] Cms page fixes Apr 2, 2019
@zuk3975 zuk3975 changed the title [WIP] Cms page fixes Cms page fixes Apr 2, 2019
@zuk3975 zuk3975 changed the title Cms page fixes [WIP]Cms page fixes Apr 3, 2019
@zuk3975 zuk3975 changed the title [WIP]Cms page fixes Cms page fixes Apr 3, 2019
@zuk3975
Copy link
Contributor Author

zuk3975 commented Apr 3, 2019

@matks ready for review

@matks matks added the migration symfony migration project label Apr 3, 2019
@zuk3975
Copy link
Contributor Author

zuk3975 commented Apr 3, 2019

@matks addressed your comments

@matks
Copy link
Contributor

matks commented Apr 4, 2019

@zuk3975 Travis is not happy. I think with a rebase you should be fine as I see code on develoop is fine: https://github.com/PrestaShop/PrestaShop/blob/develop/src/PrestaShopBundle/Form/Admin/Improve/Design/Pages/CmsPageCategoryType.php#L82

open() {
const urlParams = new URLSearchParams(location.search);

if (this.previewUrl && urlParams.has('open_preview') && this.disablePreview !== 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you rename this.disablePreview !== 1 so we can avoid !== check?

@@ -1,3 +1,5 @@
import PreviewOpener from '../../../components/form/preview-opener';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file needs license.

/**
* Opens new page of provided url if query contains specific parameters
*/
open() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be private?

* Responsible for opening another page with specified url.
* For example used in 'Save and preview' cms page create/edit actions
*/
export default class PreviewOpener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document usage of this component.

/**
* @var string
*/
private $previewUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document this attribute?

@@ -158,6 +158,9 @@ public function createAction(Request $request)
} catch (CoreException $e) {
$this->addFlash('error', $this->getErrorMessageForException($e, $this->getErrorMessages()));
}
if ($request->isXmlHttpRequest()) {
return $this->json('test');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm. 😄

}

/* When form is submitted, but invalid, disable opening preview page */
if ($result->isSubmitted()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't check if form is invalid, but comment says you should.

[
sprintf(
'"%s"',
$this->trans('Name', 'Admin.Global')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird place for ) at the end. Same for other items in this array. I think you could inline it.

$cms->getAssociatedShops()
);
$cms->getAssociatedShops(),
$this->context->link->getCMSLink($cms, null, null, $this->context->language->id)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can inject Link and $langId instead of Context.

@zuk3975
Copy link
Contributor Author

zuk3975 commented Apr 8, 2019

@matks ready

@matks
Copy link
Contributor

matks commented Apr 9, 2019

@zuk3975 PR approved 😉 however I need you to make it target 1.7.6.x instead of develop

@prestonBot prestonBot added the 1.7.6.x Branch label Apr 9, 2019
@zuk3975 zuk3975 changed the base branch from develop to 1.7.6.x April 9, 2019 09:07
@matks matks added Waiting for QA Status: action required, waiting for test feedback and removed develop Branch labels Apr 9, 2019
@marionf marionf self-assigned this Apr 9, 2019
@marionf marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 9, 2019
@marionf marionf added this to the 1.7.6.0 milestone Apr 9, 2019
@marionf marionf removed their assignment Apr 9, 2019
@matks
Copy link
Contributor

matks commented Apr 10, 2019

@LouiseBonnard Your requested change has been handled so I add the "Wording ✔️" label 😉

@matks matks added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Apr 10, 2019
@matks matks merged commit f170846 into PrestaShop:1.7.6.x Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.6.x Branch migration symfony migration project QA ✔️ Status: check done, code approved Refactoring Type: Refactoring Wording ✔️ Status: check done, wording approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants