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

Prevent unvalidated form without checkboxes #10936

Merged
merged 1 commit into from
Oct 17, 2018

Conversation

Quetzacoalt91
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 commented Oct 9, 2018

Questions Answers
Branch? 1.7.5.x
Description? When you try to generate an invoice pdf from order statuses, we expect an error when you don't check any checkbox.
Type? bug fix
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? Fixes #10474
How to test? See ticket for details. When you generate the invoices from order statuses without checking any status, an error message must be displayed.

This change is Reviewable

@matks
Copy link
Contributor

matks commented Oct 12, 2018

@Quetzacoalt91 I dont understand. According to FormInterface.php:

    /**
     * Inspects the given request and calls {@link submit()} if the form was
     * submitted.
     *
     * Internally, the request is forwarded to the configured
     * {@link RequestHandlerInterface} instance, which determines whether to
     * submit the form or not.
     *
     * @param mixed $request The request to handle
     *
     * @return $this
     */
    public function handleRequest($request = null);

So why do you need to force the submit ?

@matks matks added the Waiting for author Status: action required, waiting for author feedback label Oct 12, 2018
@Quetzacoalt91
Copy link
Member Author

It seems that if not checkbox is checked in the "get invoices by status", the form is considered as "not submitted".
This means the validators are not called, and we never get the error message "you must select at least one order status". With this change, I force the form check to be done. There is probably another way to do so, but that's the only one I found.

@Quetzacoalt91 Quetzacoalt91 removed the Waiting for author Status: action required, waiting for author feedback label Oct 12, 2018
@matks matks added the migration symfony migration project label Oct 15, 2018
@matks
Copy link
Contributor

matks commented Oct 15, 2018

@Quetzacoalt91 Fair enough 😉

matks
matks previously approved these changes Oct 15, 2018
@matks matks added this to the 1.7.5.0 milestone Oct 15, 2018
@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Oct 15, 2018
@marionf marionf self-assigned this Oct 15, 2018
@marionf
Copy link
Contributor

marionf commented Oct 15, 2018

@Quetzacoalt91

If I check a state whith invoice I have the error message while I shouldn't:

capture d ecran_439

If I check a state without invoices, I should have the error message "No invoice found for this state"

@marionf marionf added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Oct 15, 2018
@marionf marionf removed their assignment Oct 15, 2018
@Quetzacoalt91
Copy link
Member Author

Nice catch. PR modified to send only form data. :)

@Quetzacoalt91 Quetzacoalt91 removed the Waiting for author Status: action required, waiting for author feedback label Oct 16, 2018
@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Oct 16, 2018
@marionf marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Oct 17, 2018
@PierreRambaud PierreRambaud merged commit 97183f5 into PrestaShop:1.7.5.x Oct 17, 2018
@PierreRambaud
Copy link
Contributor

Thanks @Quetzacoalt91

@Quetzacoalt91 Quetzacoalt91 deleted the issue-10474 branch October 17, 2018 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.5.x Branch Bug Type: Bug migration symfony migration project QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants