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

Add validation for 2 positive integer fields #146

Merged
merged 13 commits into from
Sep 19, 2022

Conversation

leemyongpakvn
Copy link
Contributor

Questions Answers
Description? Field requires positive integer value need validation before saving
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#28929
How to test? Check above issue

Field requires positive integer value need validation before saving
@ghost
Copy link

ghost commented Jul 2, 2022

Hello @leemyongpakvn

Thank you for your involvement in the project
Could you try to use CS-fixer because the test will fail.

Code Before :

Capture d’écran de 2022-07-02 12-32-53

Code After :

Capture d’écran de 2022-07-02 12-33-14

Thank you again.

@leemyongpakvn
Copy link
Contributor Author

@okom3pom Thanks for your tool suggestion. I have updated with php-cs-fixer's fixed code.

'%s is invalid. Please enter an integer greater than %s.'
@leemyongpakvn
Copy link
Contributor Author

We need a new trans string '%s is invalid. Please enter an integer greater than %s.' in Admin.Notifications.Error also.
Admin Notifications Error_GreaterThan

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Jul 12, 2022

It looks like there are problems with PHPStan itself, not the PR's code.
PHPStan_Error

Copy link
Contributor

@Amoifr Amoifr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for this PR. I report 2 types of improvement that can be made for my opinion. I let you take a look on these.

productcomments.php Outdated Show resolved Hide resolved
productcomments.php Outdated Show resolved Hide resolved
leemyongpakvn and others added 3 commits July 19, 2022 17:18
In fact, in Validate class: isUnsignedId is just an alias of isUnsignedInt

Co-authored-by: Amoifr <31698966+Amoifr@users.noreply.github.com>
Co-authored-by: Amoifr <31698966+Amoifr@users.noreply.github.com>
Clearer variable name and function name
@florine2623 florine2623 self-assigned this Sep 15, 2022
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @leemyongpakvn ,

Thanks for the PR!

Tested the following on 1.7.8.x, 8.0.x and develop branch:
Special characters, accented characters, mix of integer/char, fractioned number, negative number.
If using any of the above characters, it won't save. I have an error message: ✅
Screenshot 2022-09-15 at 10 40 15

But if I enter 0 in BO, it will save. ❌
Screenshot 2022-09-15 at 10 40 26

Therefore, there's an error in FO, I can paginate infinitely: ❌

Screen.Recording.2022-09-15.at.10.43.32.mov

Another thing is missing in the alert: the cross to close it. ❌
For example:
Screenshot 2022-09-15 at 10 27 30

Could you check ?
Thanks!

@leemyongpakvn
Copy link
Contributor Author

@florine2623 Thanks for testing and informing. I have added one more positive number check and used Module::displayError for close button. @kpodemski has checked again and approved :)

Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @leemyongpakvn ,

Tested on 1.7.8.x, 8.0.x, develop.
Looking great !

Alert message is fixed, 0 can't be saved, FO pagination is now OK.

It is QA ✅ !
Thanks!

@kpodemski kpodemski added this to the 5.0.3 milestone Sep 19, 2022
@kpodemski kpodemski merged commit 3da8061 into PrestaShop:dev Sep 19, 2022
@kpodemski
Copy link
Contributor

Thanks @leemyongpakvn :)

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

Successfully merging this pull request may close these issues.

BO - Validation of positive integer field of the module configuration form - productcomments
5 participants