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 Sell > Orders > Delivery Slips #9139

Merged
merged 25 commits into from Jun 5, 2018

Conversation

Projects
None yet
7 participants
@PierreRambaud
Contributor

PierreRambaud commented May 29, 2018

Questions Answers
Branch? develop
Description? Migrate Delivery/Slip to Symfony Controller with new design, it include the bug fix #9133 otherwise we can't play with datepickers :) Related to PrestaShop/prestafony-project#19
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
How to test? /order/delivery/slip page and try to configure + generate a pdf

This change is Reviewable

@PierreRambaud PierreRambaud removed the 1.6.1.x label May 29, 2018

@PierreRambaud PierreRambaud changed the title from Improvement/delivery slip to Migrate Sell > Orders > Delivery Slips May 29, 2018

@PierreRambaud PierreRambaud added the wip label May 29, 2018

@PierreRambaud PierreRambaud changed the title from Migrate Sell > Orders > Delivery Slips to [WIP] Migrate Sell > Orders > Delivery Slips May 29, 2018

@PierreRambaud PierreRambaud removed the wip label May 30, 2018

@PierreRambaud PierreRambaud changed the title from [WIP] Migrate Sell > Orders > Delivery Slips to Migrate Sell > Orders > Delivery Slips May 30, 2018

/**
* This class manages Debug mode configuration for a Shop
*/
class SlipConfiguration implements DataConfigurationInterface

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

can you make it final please?

use OrderInvoice as InvoiceLegacy;
class Invoice

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

We need an interface here and making this implementation final, you can make it empty and put it in src/Core/Invoice/ new namespace.

To sump up the strategy, what we want here is to be able to change the adapter by a core class later, relying on a contract defined later in Core.

And we don't want contributors to extend this class, because this is a temporary implementation.

return $this->redirectToDefaultPage();
}
$formData = $request->request->get('form');

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

You're not allowed to do validation in controllers, rely on form providers/form handlers instead.

More, this way you don't provide any way to alter the data and forms with hooks.

See https://github.com/PrestaShop/prestafony-project/blob/master/docs/migration-guide.md#modern-form-management

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

if for some reason, you can't rely on form handlers and providers, you need to create a service an extract the validation outside of controller and rely on the Symfony validator.

@@ -38,7 +39,7 @@ class DatePickerType extends AbstractType
*/
public function getParent()
{
return'Symfony\Component\Form\Extension\Core\Type\TextType';
return TextType::class;

This comment has been minimized.

@mickaelandrieu
@@ -0,0 +1,134 @@
<?php

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

I like it a lot :)

Is it something you can document in prestafony-project? We could make the creation of tests required.

Also, for the already migrated pages we could add theses functional tests :)

)
);
$this->assertEquals(Response::HTTP_OK, $this->client->getResponse()->getStatusCode());

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

improvement for another PR: add new specific assertions like assertResponseOk(Response $response)

I have the feeling that @tomlev have already done that somewhere...

This comment has been minimized.

@PierreRambaud

PierreRambaud May 31, 2018

Contributor

Yes but it doesn't return the reponse, we can't continue the test and we can't customize the route and (again) it's for API, will maybe in another PR :)
https://github.com/PrestaShop/PrestaShop/blob/develop/tests/Integration/PrestaShopBundle/Controller/Api/ApiTestCase.php#L229

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

improvement for another PR

will maybe in another PR

<3 totally, the more we do quality, the more we want to increase the quality: hard to keep the focus on such exciting times \o/

@mickaelandrieu

This comment has been minimized.

This comment has been minimized.

Owner

PierreRambaud replied May 31, 2018

You saw nothing John Snow!

@mickaelandrieu

This comment has been minimized.

sounds like stupid but I garanty this will helps us later.

This comment has been minimized.

Owner

PierreRambaud replied May 31, 2018

Nothing is stupid if it can help someone :)

@mickaelandrieu

This comment has been minimized.

$pdf = $form->get('pdf')->getData();
$this->get('prestashop.adapter.legacy.context')->getAdminLink('AdminPdf', true, [
    'date_from' => $pdf['date_from'],
    'date_to' => $pdf['date_to'']
]);

should do the trick ^^

This comment has been minimized.

Owner

PierreRambaud replied May 31, 2018

Nice, realy thanks!

PierreRambaud added some commits May 31, 2018

Merge branch 'improvement/delivery-slip' of github.com:PierreRambaud/…
…PrestaShop into improvement/delivery-slip
'optionsForm' => $form->createView(),
'pdfForm' => $this->get('prestashop.adapter.order.delivery.slip.pdf.form_handler')->getForm()->createView(),
'help_link' => $this->generateSidebarLink($request->attributes->get('_legacy_controller')),
'layoutTitle' => $this->trans('Delivery Slips', 'Admin.NavigationMenu'),

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jun 4, 2018

Contributor

Looks like one full stop is missing: Admin.Navigation.Menu

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

Woot, nice catch, Thanks a lot =)

<div class="card-block">
<div class="card-text">
<div class="form-group">
{{ ps.label_with_help('From'|trans({}, 'Admin.Globals'), ('Format: 2011-12-31 (inclusive).'|trans({}, 'Admin.Orderscustomers.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jun 4, 2018

Contributor

Same here: Admin.Global

{{ form_widget(pdfForm.pdf.date_from) }}
</div>
<div class="form-group">
{{ ps.label_with_help('To'|trans({}, 'Admin.Globals'), ('Format: 2011-12-31 (inclusive).'|trans({}, 'Admin.Orderscustomers.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jun 4, 2018

Contributor

Same here: Admin.Global

</div>
<div class="card-footer">
<div class="d-flex justify-content-end">
<button class="btn btn-primary">{{ 'Generate PDF'|trans({}, 'Admin.Actions') }}</button>

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jun 4, 2018

Contributor

Shouldn't it be Admin.Orderscustomers.Feature instead?

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 4, 2018

Thanks again @LouiseBonnard for the feedback 👍

@marionf marionf added QA ✔️ and removed waiting for QA labels Jun 4, 2018

@mickaelandrieu mickaelandrieu merged commit f1b7c64 into PrestaShop:develop Jun 5, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 5, 2018

Thank you @PierreRambaud !

@PierreRambaud PierreRambaud deleted the PierreRambaud:improvement/delivery-slip branch Jun 5, 2018

@eternoendless eternoendless added this to the 1.7.5.0 milestone Jun 6, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@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