-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fixing the bug that appeared when anonymize customer last name option has been added. #89
Conversation
Oksydan
commented
Dec 9, 2020
Questions | Answers |
---|---|
Description? | Fixing the bug that appeared when anonymize customer last name option has been added. |
Type? | bug fix |
BC breaks? | no |
Deprecations? | no |
Fixed ticket? | Fixes PrestaShop/PrestaShop#22222. |
How to test? | Issue describes it enough. |
controllers/front/ListComments.php
Outdated
@@ -65,7 +65,7 @@ public function display() | |||
$productComment['content'] = htmlentities($productComment['content']); | |||
$productComment['date_add'] = $dateFormatter->format($dateAdd); | |||
|
|||
if ($isLastNameAnynomus) { | |||
if ($isLastNameAnynomus && isset($productComment['firstname']) && isset($productComment['lastname'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello
What's the idea behind checking if firstname is set if we don't use it in the condition ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
isset($productComment['firstname']) && isset($productComment['lastname'])
- for deleting account firstname and lastname is not set. I just want to be sure that nothing weird are going to happen when first name will not be available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you are right. Check exists on line 74.
CSFixer is red, please run it :) |
Cs fixer is red for a bad reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase your pull request
@PierreRambaud I did it against latest dev branch with your CS fixer changes. |
@Oksydan I don't know how you rebased your PR but something's wrong 🤔, you shouldn't see someone else's commits on your PR. |
40500bd
to
9710c7a
Compare
@atomiix |
Hi @Oksydan, Thanks! |
Thank you @Oksydan |