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

Migration of page Shop Parameters > General > Maintenance #8604

Merged

Conversation

@Quetzacoalt91
Copy link
Member

commented Dec 11, 2017

Questions Answers
Branch? develop
Description? Migrate Maintenance page on Sf core
Type? new feature
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? http://forge.prestashop.com/browse/BOGOSS-70
How to test? Reach page http://<shop url>/<admin folder>/index.php/configure/shop/maintenance

This change is Reviewable

class: 'PrestaShopBundle\Form\Admin\ShopParameters\General\MaintenanceType'
parent: 'form.type.translatable.aware'
calls:
- [ setLocales, ["@=service('prestashop.adapter.legacy.context').getLanguages()"]]

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 11, 2017

Author Member

@mickaelandrieu, don't you think we should move that call in the service form.type.translatable.aware?

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 11, 2017

@LittleBigDev LittleBigDev added the WIP label Dec 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 11, 2017

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch from b8a4511 to ca452ce Dec 11, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 11, 2017

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2017

@eternoendless, do you confirm we have no more bordered versions of the nav-pills and nav-tabs?

I think the integration of a multilanguage textarea could be improved without background.
capture du 2017-12-11 17-42-13

@mickaelandrieu
Copy link
Member

left a comment

Good job so far, some comments but for me it's good. Ping me tomorrow, we need to check if all hooks are here and you need to map the router + remove the legacy :)

$this->configuration = $configuration;
}
/**

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

you can use {@inheritdoc} as you rely on interface

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 11, 2017

Author Member

Done

return $redirectResponse;
}
$this->dispatchHook('actionAdminMaintenanceControllerPostProcessBefore', array('controller' => $this));

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

i need to update docs already, but this hook should be dispatcher after the security check, for perfomance reasons mainly :)

$this->flashErrors($saveErrors);
return $redirectResponse;
}
}

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

end of line

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 11, 2017

Author Member

Done

$this->dispatchHook('actionAdminMaintenanceControllerPostProcessBefore', array('controller' => $this));
$form = $this->get('prestashop.adapter.maintenance.form_handler')->getForm();
$form->handleRequest($request);

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

regarding the complete action, we always returns redirectResponse... we may refactor it, what do you think?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 12, 2017

Author Member

Isn't it already done?
The first line of the function the response creation:

$redirectResponse = $this->redirectToRoute('admin_maintenance');

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

Yeah but I'm sure we can probably do better, it's not a blocker for me :-)

* This class is responsible of managing the data manipulated using forms
* in "Configure > Shop Parameters > General > Maintenance" page.
*/
class MaintenanceFormDataProvider

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

I see you have merged the PR which introduces FormDataProviderInterface, rebase your pr and use it, and you can use {@inheritdoc} annotations too 👍

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

Also, form data providers are usually declared as final: if someone want to change the behavior, he'll need to implement the interface instead of re-use the classes we will unit test (in the end)

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
class IpAddressType extends TextType

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

Maybe a PHPDoc to explain the use of IpAddress block?

use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\Form\Extension\Core\Type\TextareaType;
class MaintenanceType extends TranslatorAwareType

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

Maybe a PHPDoc to explain the use of this form type?

<script src="{{ asset('../js/tiny_mce/tiny_mce.js') }}"></script>
<script src="{{ asset('../js/admin/tinymce.inc.js') }}"></script>
<script src="{{ asset('../js/admin/tinymce_loader.js') }}"></script>
<script>

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

maybe extract this script into MaintenancePage and get it using Webpack? you can reuse the strategy used here => 844ee7b

The long terme idea: everything - including tiny MCE - will be loaded using Webpack 👍

<i class="material-icons">add_circle</i> Add my IP
</button>
</div>
{% endblock ip_address_text_widget %}

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

End of line

<div class="input-group">
{{- block('form_widget_simple') -}}
<button type="button" class="btn btn-outline-primary add_ip_button">
<i class="material-icons">add_circle</i> Add my IP

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

"Add my IP" should be translated, ins't it?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 11, 2017

Author Member

Yep, done

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch 2 times, most recently from 9ac7e05 to 857fba6 Dec 11, 2017

use Symfony\Component\Form\Extension\Core\Type\TextareaType;
/**
* Class retuning the content of the form in the maintenance page.

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 11, 2017

Member

returning (minor typo)

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 12, 2017

@Quetzacoalt91 Quetzacoalt91 removed the WIP label Dec 12, 2017

@Quetzacoalt91 Quetzacoalt91 changed the title [WIP] Migration maintenance page Migration maintenance page Dec 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 12, 2017

use PrestaShop\PrestaShop\Core\Configuration\DataConfigurationInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
class MaintenanceConfiguration implements DataConfigurationInterface

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

PHPDoc missing (sorry I've missed it)

* This class is responsible of managing the data manipulated using forms
* in "Configure > Shop Parameters > General > Maintenance" page.
*/
class MaintenanceFormDataProvider implements FormDataProviderInterface

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

use final class please 👍

* This class manages the data manipulated using forms
* in "Configure > Advanced Parameters > Performance" page.
*/
class MaintenanceFormHandler implements FormHandlerInterface

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

use final class 👍

<i class="material-icons">add_circle</i> {{ 'Add my IP'|trans({}, 'Helper') }}
</button>
</div>
{% endblock ip_address_text_widget %}

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

end of line (you can configure your IDE, but I don't know how xD)

@mickaelandrieu
Copy link
Member

left a comment

This is almost done, good job! Please check for hooks dispatched in legacy/modern pages in order to re-introduce or deprecate related hooks => https://gist.github.com/mickaelandrieu/00d32492113f9e87e326034dee60a7f2#how-to-migrate-hooks

header('Location: ../../../../../../../');
exit;
global.IpInput = {};
IpInput.addRemoteAddr = (target) => {

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Member

as a last comment, some failures from codacy can be fixed for free => https://www.codacy.com/app/PrestaShop/PrestaShop/pullRequest?prid=1148080

And I think it's better to pass ip variable to this function, if you need it, wdyt?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Dec 14, 2017

Author Member

Indeed, I totally missed that solution while migrating the code.

@Quetzacoalt91 Quetzacoalt91 changed the title Migration maintenance page Migration of page Shop Parameters > General > Maintenance Dec 13, 2017

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch from 0a837b7 to 9bdc750 Dec 14, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 14, 2017

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch from 9bdc750 to 76a865b Dec 14, 2017

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch from 8a5f67d to e2654b1 Jan 25, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

Fixed (& rebased)

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

If the switch is on "No" and I try to change the IP or the message, I have a message that forces me to re-click on "No"
screenrecord81-2018-01-25_14 56 52

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

Small fix applied!

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

Reviewed 1 of 1 files at r21, 1 of 1 files at r22.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Small fix indeed 👍

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

It's just missing the translation of the "Yes/No" button and it will be fine :)

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

I have currently some trouble to make this Form type working properly, I'll have to move this last-minute feature outside the pull-request.
Tomorrow I rollback this PR, and a dedicated one will be opened for the Switch integration.

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:migration-maintenance-page branch from 5d2b58d to f76178b Jan 26, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

I removed all the commits regarding the switch in the PR history.
This work will now be done is a dedicated PR, please review that one without the feature.

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

:lgtm: (again)


Reviewed 4 of 5 files at r23.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

:lgtm:


Reviewed 1 of 1 files at r24.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Thank you @Quetzacoalt91

@eternoendless eternoendless merged commit 1159def into PrestaShop:develop Jan 29, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
code-review/reviewable 35 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 deleted the Quetzacoalt91:migration-maintenance-page branch Jan 29, 2018

@eternoendless eternoendless added this to the 1.7.4.0 milestone Apr 13, 2018

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.