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

Migrate "Configure > Advanced Parameters > Webservices" - part 1 (configuration form) #9310

Merged
merged 8 commits into from Aug 22, 2018

Conversation

Projects
None yet
7 participants
@matks
Contributor

matks commented Jul 12, 2018

Linked to PrestaShop/prestafony-project#37

Questions Answers
Branch? develop
Description? Migrate BO page 'webservice' to Symfony using modern controllers and form handlers - part 1
Type? new feature
Category? BO
BC breaks? yes
Deprecations? no
Fixed ticket?
How to test? See below

How to test

Login on backoffice, then browse /admin-dev/index.php/configure/advanced/webservice/. This shows the SF version of the form of the legacy page /admin-dev/index.php?controller=AdminWebservice. Only the form "configuration", not the listing (this will be delivered later).

You can check the 2 forms behave in the same way.


This change is Reviewable

@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Jul 12, 2018

Collaborator

Hi!

Your pull request description seems to be incomplete or malformed:

  • The category should be one of: FO, BO, CO, IN, TE, WS, LO

Would you mind completing the contribution table ? This would help us understand how interesting your contribution is.

Thank you!

(note: this is an automated message, but answering it will reach a real human )

Collaborator

prestonBot commented Jul 12, 2018

Hi!

Your pull request description seems to be incomplete or malformed:

  • The category should be one of: FO, BO, CO, IN, TE, WS, LO

Would you mind completing the contribution table ? This would help us understand how interesting your contribution is.

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Jul 12, 2018

Contributor

@mickaelandrieu This is the part 1 of the page migration PrestaShop/prestafony-project#37. This is a split from PR #9272 with only the Configuration Form. It carries the fixes asked by you, sarjon and louise.

Can be validated by QA and safely merged into develop if review is valid.

Contributor

matks commented Jul 12, 2018

@mickaelandrieu This is the part 1 of the page migration PrestaShop/prestafony-project#37. This is a split from PR #9272 with only the Configuration Form. It carries the fixes asked by you, sarjon and louise.

Can be validated by QA and safely merged into develop if review is valid.

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Jul 19, 2018

Contributor

Requested changes have been implemented. PR rebased, ready to merge

Contributor

matks commented Jul 19, 2018

Requested changes have been implemented. PR rebased, ready to merge

}
/**
* @param Request $request optional

This comment has been minimized.

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

@param Request $request (optional) Request.

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

@param Request $request (optional) Request.

if ($request !== null) {
if (strpos($request->server->get('SERVER_SOFTWARE'), 'Apache') === false) {
$issues[] = self::ISSUE_NOT_APACHE_SERVER;

This comment has been minimized.

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

Don't understand why using Nginx can be an issue?

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

Don't understand why using Nginx can be an issue?

This comment has been minimized.

@matks

matks Jul 19, 2018

Contributor

This is a rule that was applied in the legacy controller. I do not have the history.

@matks

matks Jul 19, 2018

Contributor

This is a rule that was applied in the legacy controller. I do not have the history.

}
if (false === in_array('mod_rewrite', $apache_modules)) {
$issues[] = self::ISSUE_APACHE_MOD_AUTH_REWRITE_NOT_AVAILABLE;

This comment has been minimized.

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

Weird, where we are under a Symfony page ^^ This one can be removed maybe?

@PierreRambaud

PierreRambaud Jul 19, 2018

Contributor

Weird, where we are under a Symfony page ^^ This one can be removed maybe?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jul 30, 2018

Contributor

we try to be iso functional with legacy: are you 100% sure that in Symfony page this condition is not required?

@mickaelandrieu

mickaelandrieu Jul 30, 2018

Contributor

we try to be iso functional with legacy: are you 100% sure that in Symfony page this condition is not required?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 6, 2018

Contributor

Hello @matks, would you mind to rebase your pull request?

Contributor

mickaelandrieu commented Aug 6, 2018

Hello @matks, would you mind to rebase your pull request?

@eternoendless

Reviewed 6 of 14 files at r1, 4 of 4 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @mickaelandrieu, @matks, and @PierreRambaud)


src/Core/Webservice/WebserviceCanBeEnabledConfigurationChecker.php, line 78 at r3 (raw file):

        $issues = $this->lookForIssues($request);

        $allWarningMessages = $this->getWarningMessages();

It looks like you are translating all the warning messages before even knowing if an error has occurred. Maybe you could do that only if issues is not empty?


src/Core/Webservice/WebserviceCanBeEnabledConfigurationChecker.php, line 83 at r3 (raw file):

        foreach ($issues as $issue) {
            if (false === array_key_exists($issue, $allWarningMessages)) {
                throw new RuntimeException(sprintf('Unexpected configuration issue %s', $issue));
- 'Unexpected configuration issue %s'
+'Unexpected configuration issue: "%s"'

src/Core/Webservice/WebserviceCanBeEnabledConfigurationChecker.php, line 103 at r3 (raw file):

Previously, matks (Mathieu Ferment) wrote…

This is a rule that was applied in the legacy controller. I do not have the history.

I guess web services didn't work with other web services for some reason. Probably due to rewrite rules.


src/Core/Webservice/WebserviceCanBeEnabledConfigurationChecker.php, line 115 at r3 (raw file):

Weird, where we are under a Symfony page ^^

Are you implying that it's needed to access symfony routes? Because it isn't.

Either way, it may be needed for web services (which are legacy-based)


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/WebserviceController.php, line 43 at r3 (raw file):

 * Responsible of "Configure > Advanced Parameters > Webservice" page display
 *
 * @todo: add unit tests

Since this is a controller, I think you meant either survival or functional.

Unit, integration and functional (E2E) tests should not be mistaken. Read more here: https://codeutopia.net/blog/2015/04/11/what-are-unit-testing-integration-testing-and-functional-testing/


src/PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/WebservicePage/webservice.html.twig, line 29 at r3 (raw file):

{% block content %}

This template will have to be updated according to the new horizontal style (See "shop parameters > general")

* This class is responsible of managing the data manipulated using forms
* in "Configure > Advanced Parameters > Webservice" page.
*/
final class WebserviceFormDataProvider implements FormDataProviderInterface

This comment has been minimized.

@matks

matks Aug 21, 2018

Contributor

@mickaelandrieu I do not remember whether this was intentional or a mistake from me. Is it good for FormDataProvider to be final ?

@matks

matks Aug 21, 2018

Contributor

@mickaelandrieu I do not remember whether this was intentional or a mistake from me. Is it good for FormDataProvider to be final ?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

yes it does 👍

@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

yes it does 👍

I need to review this again

@matks

This comment has been minimized.

Show comment
Hide comment
@matks

matks Aug 21, 2018

Contributor

All requested changes have been either answered or implemented in the 2 latest commits. Please re-review and, if eligible, merge @mickaelandrieu 😉

Contributor

matks commented Aug 21, 2018

All requested changes have been either answered or implemented in the 2 latest commits. Please re-review and, if eligible, merge @mickaelandrieu 😉

Requested changes have been applied

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

As the page is not available for people this doesn't QA approval.

We will ask QA for help when we will implement the full page.

@eternoendless I may need a doc about this new "horizontal" theme you're talking about: how I can be sure the integration of pages is good? I don't want you to be required on every pull request 😞

Thanks @matks!

Contributor

mickaelandrieu commented Aug 22, 2018

As the page is not available for people this doesn't QA approval.

We will ask QA for help when we will implement the full page.

@eternoendless I may need a doc about this new "horizontal" theme you're talking about: how I can be sure the integration of pages is good? I don't want you to be required on every pull request 😞

Thanks @matks!

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Aug 22, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

Thanks @matks, the grid will be done later don't worry 👍

Contributor

mickaelandrieu commented Aug 22, 2018

Thanks @matks, the grid will be done later don't worry 👍

@mickaelandrieu mickaelandrieu merged commit f418207 into PrestaShop:develop Aug 22, 2018

1 of 2 checks passed

code-review/reviewable 14 files, 20 discussions left (eternoendless, matks, mickaelandrieu, PierreRambaud)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu referenced this pull request Aug 22, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@matks matks deleted the matks:migrate-webservice-page-part1 branch Aug 24, 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