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 short description limit to be set at 0 #11312

Merged
merged 1 commit into from Nov 9, 2018

Conversation

Projects
None yet
6 participants
@jolelievre
Contributor

jolelievre commented Nov 7, 2018

Questions Answers
Branch? 1.7.5.x
Description? Prevent short description limit to be set at 0
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #11164
How to test? See ticket bug description

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.5.0 milestone Nov 7, 2018

@arouiadib

This comment has been minimized.

arouiadib commented Nov 8, 2018

thanks @jolelievre.
I suggest changes to be equally done for the validation in Catalog > Product > New Product and Catalog > Product > Product Edit , by editing src/PrestaShopBundle/Form/Admin/Product/ProductInformation.php, Lines 194 and 198.

Additionnaly, values greater than 800 should be considered as equal to 800. Then, a comparison with 800 should be added.

And '800' should be declared as a constant , e.g, on the top of class attributes.

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 8, 2018

hello @arouiadib
I'm not sure I understand the modification you suggest, the validation for edition and creation is already present in ProductInformation at the lines you suggested, so there is no validation to add
I just tested it and it works fine, even when you change the value (for example 10) you have the appropriate error message when you try to save

Besides I don't think we should crop the text when it's over 800 as you suggested because it hides the modification to the user who will not understand why his modification was not fully applied. It is a better practice to inform the user about the error and let him fix it himself.

Finally about the 800 constant, do you mean we should define a constant for the default value which would be common between ProductInformation and ProductPreferencesFormDataProvider ? Only for the default value then because we need to keep this value configurable. That's a good idea, I'll have to look for other places where this value is used then.

@arouiadib

This comment has been minimized.

arouiadib commented Nov 8, 2018

@jolelievre You are right. I am sorry.
Thanks.

@matks matks added the waiting for QA label Nov 9, 2018

@marionf marionf self-assigned this Nov 9, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 9, 2018

@marionf marionf removed their assignment Nov 9, 2018

@jolelievre jolelievre merged commit 311b4ab into PrestaShop:1.7.5.x Nov 9, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jolelievre jolelievre deleted the jolelievre:short-description-limit branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment