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

Fix display for invalid characters in tax name #26546

Merged
merged 1 commit into from Dec 8, 2021
Merged

Fix display for invalid characters in tax name #26546

merged 1 commit into from Dec 8, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2021

Questions Answers
Branch? develop
Description? See below
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #26515
How to test? Check if the invalid characters are good
Possible impacts? No

Description

Currently the tax name validation uses generic_name
See this line.

The sprintf uses another rule see this line and these lines to see the all restrictions


This change is Reviewable

@ghost ghost self-requested a review as a code owner November 8, 2021 09:23
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels Nov 8, 2021
Copy link
Contributor

@tegbessou tegbessou left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! IMO all is good

@matthieu-rolland matthieu-rolland added the Waiting for QA Status: action required, waiting for test feedback label Nov 8, 2021
@HanaRebaiQA HanaRebaiQA self-assigned this Nov 11, 2021
@HanaRebaiQA
Copy link

Hello,
The issue is not fixed.

See screen record :
https://www.awesomescreenshot.com/video/5993998?key=d3eb130321ac7c462d474321c5a6842a

Could you check it please !

Thanks,

@HanaRebaiQA HanaRebaiQA added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Nov 12, 2021
@ghost
Copy link
Author

ghost commented Nov 14, 2021

Hello @HanaRebaiQA

We should be able to add # and ; in the name of the taxes.
My PR corrects the display of allowed characters.
I can edit to disallow # and ; but I don't understand why these characters are not allowed.

@HanaRebaiQA
Copy link

Hello @PrestaShop/product-team
I created the issue #26515 in order to fix the mismatch between the tax Name field and the message displayed bellow. But the PR fixes the message displayed bellow and not the tax Name field .
So with this PR the special characters ; and # will be allowed. What do you think?
image

Thanks in advance,

@ghost
Copy link
Author

ghost commented Dec 1, 2021

Hello @HanaRebaiQA can you change the label to Waiting for PM

Regards

@HanaRebaiQA HanaRebaiQA added Waiting for PM Status: action required, waiting for product feedback and removed Waiting for author Status: action required, waiting for author feedback labels Dec 1, 2021
@MatShir
Copy link
Contributor

MatShir commented Dec 1, 2021

@PierreRambaud another one 🤔, IMO this field needs should accept every character.
LGTM but I have to put waiting for wording because the behavior is changing ;)
cc @Julievrz could you have a look specifically for the help text ;)

@MatShir MatShir added Waiting for wording Status: action required, waiting for wording and removed Waiting for PM Status: action required, waiting for product feedback labels Dec 1, 2021
@Julievrz
Copy link
Contributor

Julievrz commented Dec 1, 2021

@PierreRambaud another one 🤔, IMO this field needs should accept every character. LGTM but I have to put waiting for wording because the behavior is changing ;) cc @Julievrz could you have a look specifically for the help text ;)

Yes, Hana had already told me about this issue. I don't see any reason why we shouldn't accept these characters. 👍

@Julievrz Julievrz added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Dec 1, 2021
@ghost
Copy link
Author

ghost commented Dec 1, 2021

@Progi1984 or @PierreRambaud If you can add the label Waiting for QA

Sincerely

@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Dec 6, 2021
@HanaRebaiQA
Copy link

Hello @okom3pom

Thanks for your PR.
I have tested this PR and the issue is fixed. The actual behavior now is to allow all caracters expect these ones <>={} as mentioned in the displayed message. So, now the message and the restcition are both compatible. See screen-record below:
https://watch.screencastify.com/v/bcCUniaSgIquxqtHdKh9

So, QA ✔️

I verified the result also with mulilang, multistore
image

Thanks,

@HanaRebaiQA HanaRebaiQA added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Dec 8, 2021
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@matks matks added this to the 8.0.0 milestone Dec 8, 2021
@matks
Copy link
Contributor

matks commented Dec 8, 2021

Thank you @okom3pom

@matks matks merged commit c2c8229 into PrestaShop:develop Dec 8, 2021
@ghost ghost deleted the fix-invalid-char-tax branch December 8, 2021 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch QA ✔️ Status: check done, code approved Wording ✔️ Status: check done, wording approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BO - The tax Name field accept some invalid characters
10 participants