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

Jump to bottom

Avoid displaying the addon modal twice, change form names/IDs to avoid collision #10684

Merged
merged 5 commits into from Oct 5, 2018
Merged

Avoid displaying the addon modal twice, change form names/IDs to avoid collision #10684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77445bb
avoid displaying the addon modal twice, force form name to avoid coll…
jolelievre Sep 24, 2018
d683b08
fond a less intrusive way to adapt the form name in FormHandler class
jolelievre Sep 25, 2018
ab2d60f
change scope assignment to fix table counting in form ids
jolelievre Sep 26, 2018
433ec4a
meta form had doublons
jolelievre Sep 26, 2018
ce18054
rebase and cs fixer
jolelievre Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion admin-dev/themes/default/template/helpers/form/form.tpl
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@
{if isset($identifier_bk) && $identifier_bk == $identifier}{capture name='identifier_count'}{counter name='identifier_count'}{/capture}{/if}
{assign var='identifier_bk' value=$identifier scope='parent'}
{if isset($table_bk) && $table_bk == $table}{capture name='table_count'}{counter name='table_count'}{/capture}{/if}
{assign var='table_bk' value=$table scope='parent'}
{assign var='table_bk' value=$table scope='root'}
Copy link
Member

Choose a reason for hiding this comment

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

Reducing the visibility of this variable may break some compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it actually enhance it since it is visible by root and not only parent
without this the couting can't work because the two forms don't have the same immediate parent

<form id="{if isset($fields.form.form.id_form)}{$fields.form.form.id_form|escape:'html':'UTF-8'}{else}{if $table == null}configuration_form{else}{$table}_form{/if}{if isset($smarty.capture.table_count) && $smarty.capture.table_count}_{$smarty.capture.table_count|intval}{/if}{/if}" class="defaultForm form-horizontal{if isset($name_controller) && $name_controller} {$name_controller}{/if}"{if isset($current) && $current} action="{$current|escape:'html':'UTF-8'}{if isset($token) && $token}&amp;token={$token|escape:'html':'UTF-8'}{/if}"{/if} method="post" enctype="multipart/form-data"{if isset($style)} style="{$style}"{/if} novalidate>
{if $form_id}
<input type="hidden" name="{$identifier}" id="{$identifier}{if isset($smarty.capture.identifier_count) && $smarty.capture.identifier_count}_{$smarty.capture.identifier_count|intval}{/if}" value="{$form_id}" />
Expand Down
2 changes: 1 addition & 1 deletion admin-dev/themes/default/template/helpers/options/options.tpl
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@
</script>
{block name="defaultOptions"}
{if isset($table_bk) && $table_bk == $table}{capture name='table_count'}{counter name='table_count'}{/capture}{/if}
{assign var='table_bk' value=$table scope='parent'}
{assign var='table_bk' value=$table scope='root'}
<form action="{$current|escape:'html':'UTF-8'}&amp;token={$token|escape:'html':'UTF-8'}" id="{if $table == null}configuration_form{else}{$table}_form{/if}{if isset($smarty.capture.table_count) && $smarty.capture.table_count}_{$smarty.capture.table_count|intval}{/if}" method="post" enctype="multipart/form-data" class="form-horizontal">
{foreach $option_list AS $category => $categoryData}
{if isset($categoryData['top'])}{$categoryData['top']}{/if}
Expand Down
10 changes: 8 additions & 2 deletions classes/controller/AdminController.php
View file
Open in desktop
Expand Up @@ -1786,7 +1786,7 @@ public function display()
$header_tpl = file_exists($dir . 'header.tpl') ? $dir . 'header.tpl' : 'header.tpl';
$page_header_toolbar = file_exists($dir . 'page_header_toolbar.tpl') ? $dir . 'page_header_toolbar.tpl' : 'page_header_toolbar.tpl';
$footer_tpl = file_exists($dir . 'footer.tpl') ? $dir . 'footer.tpl' : 'footer.tpl';
$modal_module_list = file_exists($module_list_dir . 'modal.tpl') ? $module_list_dir . 'modal.tpl' : 'modal.tpl';
$modal_module_list = file_exists($module_list_dir . 'modal.tpl') ? $module_list_dir . 'modal.tpl' : '';
$tpl_action = $this->tpl_folder . $this->display . '.tpl';

// Check if action template has been overridden
Expand Down Expand Up @@ -1826,9 +1826,15 @@ public function display()
$this->context->smarty->assign(
array(
'page_header_toolbar' => $this->context->smarty->fetch($page_header_toolbar),
'modal_module_list' => $this->context->smarty->fetch($modal_module_list),
)
);
if (!empty($modal_module_list)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With the ternary condition, $modal_module_list is never empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes an empty string returns true, that's the point of the test

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my bad, miss reading the diff and thinking the ternary was:

file_exists($module_list_dir . 'modal.tpl') ? $module_list_dir . 'modal.tpl' : 'modal.tpl';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that was the previous ternary, but with the new template it no longer has the wanted behavior and instead of adding a modal for available modules (which does not exist in the new theme) it resulted in including the addon login modal in two different locations

$this->context->smarty->assign(
array(
'modal_module_list' => $this->context->smarty->fetch($modal_module_list),
)
);
}
}

$this->context->smarty->assign('baseAdminUrl', __PS_BASE_URI__ . basename(_PS_ADMIN_DIR_) . '/');
Expand Down
21 changes: 19 additions & 2 deletions src/Core/Form/FormHandler.php
View file
Open in desktop
Expand Up @@ -61,14 +61,31 @@ class FormHandler implements FormHandlerInterface
*/
protected $formTypes;

/**
* @var string the form name
*/
protected $formName;

/**
* FormHandler constructor.
*
* @param FormBuilderInterface $formBuilder
* @param HookDispatcherInterface $hookDispatcher
* @param FormDataProviderInterface $formDataProvider
* @param array $formTypes
* @param string $hookName
* @param string $formName
*/
public function __construct(
FormBuilderInterface $formBuilder,
HookDispatcherInterface $hookDispatcher,
FormDataProviderInterface $formDataProvider,
array $formTypes,
$hookName
$hookName,
$formName = 'form'
) {
$this->formBuilder = $formBuilder;
$this->formName = $formName;
$this->formBuilder = $formBuilder->getFormFactory()->createNamedBuilder($formName);
$this->hookDispatcher = $hookDispatcher;
$this->formDataProvider = $formDataProvider;
$this->formTypes = $formTypes;
Expand Down
6 changes: 4 additions & 2 deletions src/PrestaShopBundle/Resources/config/services/bundle/form/form_handler.yml
View file
Open in desktop
Expand Up @@ -164,7 +164,7 @@ services:
- '@prestashop.adapter.order.delivery.slip.options.form_provider'
-
'options': 'PrestaShopBundle\Form\Admin\Sell\Order\Delivery\SlipOptionsType'
- 'OrderDeliverySlipPage'
- 'OrderDeliverySlipOptions'

prestashop.adapter.order.delivery.slip.pdf.form_handler:
class: 'PrestaShop\PrestaShop\Core\Form\FormHandler'
Expand All @@ -174,7 +174,8 @@ services:
- '@prestashop.adapter.order.delivery.slip.pdf.form_provider'
-
'pdf': 'PrestaShopBundle\Form\Admin\Sell\Order\Delivery\SlipPdfType'
- 'OrderDeliverySlipPage'
- 'OrderDeliverySlipPdf'
- 'slip_pdf_form'

prestashop.admin.geolocation.form_handler:
class: 'PrestaShop\PrestaShop\Core\Form\FormHandler'
Expand Down Expand Up @@ -274,6 +275,7 @@ services:
'shop_urls': 'PrestaShopBundle\Form\Admin\Configure\ShopParameters\TrafficSeo\Meta\ShopUrlType'
'url_schema': 'PrestaShopBundle\Form\Admin\Configure\ShopParameters\TrafficSeo\Meta\UrlSchemaType'
- 'MetaPage'
- 'meta_settings_form'

# Entity form handler
prestashop.admin.request_sql.form_handler:
Expand Down
8 changes: 4 additions & 4 deletions tests/Integration/PrestaShopBundle/Controller/Admin/Sell/Order/DeliveryControllerTest.php
View file
Open in desktop
Expand Up @@ -108,14 +108,14 @@ public function testSlipActionWithValidData()

public function testPdfActionWithInvalidData()
{
$token = $this->client->getContainer()->get('security.csrf.token_manager')->getToken('form');
$token = $this->client->getContainer()->get('security.csrf.token_manager')->getToken('slip_pdf_form');
$this->client->request(
'POST',
$this->router->generate(
'admin_order_delivery_slip_pdf'
),
[
'form' => [
'slip_pdf_form' => [
'pdf' => [
'date_from' => 'foo'
],
Expand All @@ -137,14 +137,14 @@ public function testPdfActionWithInvalidData()

public function testPdfActionWithEmptyData()
{
$token = $this->client->getContainer()->get('security.csrf.token_manager')->getToken('form');
$token = $this->client->getContainer()->get('security.csrf.token_manager')->getToken('slip_pdf_form');
$this->client->request(
'POST',
$this->router->generate(
'admin_order_delivery_slip_pdf'
),
[
'form' => [
'slip_pdf_form' => [
'pdf' => [],
'_token' => $token
],
Expand Down