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.

Sign up for GitHub

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

Jump to bottom

BO - Order Messages - Avoid message with duplicate name #24579

Merged
merged 1 commit into from May 21, 2021
Merged

BO - Order Messages - Avoid message with duplicate name #24579

merged 1 commit into from May 21, 2021

Conversation

Progi1984
Copy link
Contributor

@Progi1984 Progi1984 commented May 20, 2021 •
edited by PierreRambaud

Questions Answers
Branch? 1.7.7.x
Description? BO - Order Messages - Avoid message with duplicate name
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #24535
How to test? Cf. #24535

This change is Reviewable

@Progi1984 Progi1984 requested a review from a team as a code owner May 20, 2021 09:15
@prestonBot prestonBot added 1.7.7.x Branch Bug fix Type: Bug fix labels May 20, 2021
@Progi1984 Progi1984 linked an issue May 20, 2021 that may be closed by this pull request
BO - Orders - Order message - Order message with the same title is not displayed in the "order_message" list #24535
Closed
@prestonBot
Copy link
Collaborator

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label May 20, 2021
@Progi1984 Progi1984 added this to the 1.7.7.5 milestone May 20, 2021
@Progi1984
Copy link
Contributor Author

@Julievrz Could I have an existing message for avoiding the new string ?

(Ping @MatShir)

PierreRambaud
PierreRambaud previously approved these changes May 20, 2021
View reviewed changes
jolelievre
jolelievre reviewed May 20, 2021
View reviewed changes
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Suggestion for an improvement

src/Adapter/OrderMessage/CommandHandler/AddOrderMessageHandler.php Show resolved Hide resolved
@Julievrz Julievrz added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels May 20, 2021
@Progi1984 Progi1984 requested a review from sowbiba May 20, 2021 15:07
@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label May 20, 2021
@hibatallahAouadni hibatallahAouadni self-assigned this May 20, 2021
hibatallahAouadni
hibatallahAouadni reviewed May 21, 2021
View reviewed changes
Copy link
Contributor

@hibatallahAouadni hibatallahAouadni left a comment

Choose a reason for hiding this comment

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

Hello @Progi1984

I think there's a regression within your PR, check my review and the attached screen record below:

https://drive.google.com/file/d/1SkUWwXxIU-9z804s693RklCLQKNIghQL/view

Thanks!

src/PrestaShopBundle/Controller/Admin/Sell/CustomerService/OrderMessageController.php
@@ -244,6 +245,10 @@ private function getErrorMessages(): array
'The object cannot be loaded (or found)',
'Admin.Notifications.Error'
),
OrderMessageNameAlreadyUsedException::class => $this->trans(
Copy link
Contributor

Choose a reason for hiding this comment

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

After OrderMessageNameAlreadyUsedException there's "::" IMHO this is NOK, it should be ":" 🙂
I notice that with your PR, I have Order Settings instead of Orders and Customer Settings instead of Customers, I don't know if it's related but this is the only typo that I found 😉

@hibatallahAouadni hibatallahAouadni added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels May 21, 2021
@Progi1984
Copy link
Contributor Author

@hibatallahAouadni I didn't reproduce your bug. Could you check it again, please?

image

@Progi1984 Progi1984 added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels May 21, 2021
@hibatallahAouadni
Copy link
Contributor

Hello @Progi1984

Thanks for your feedback!
What I encountred is this issue #9762 and it's already fixed with 1.7.8.x.
So, LGTM and QA ✔️

Thanks!

@hibatallahAouadni hibatallahAouadni added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels May 21, 2021
@Progi1984 Progi1984 merged commit 81ea792 into PrestaShop:1.7.7.x May 21, 2021
@Progi1984 Progi1984 deleted the issue24535 branch May 21, 2021 11:25
@Progi1984
Copy link
Contributor Author

Thanks @hibatallahAouadni

@Progi1984 Progi1984 mentioned this pull request May 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sowbiba sowbiba sowbiba approved these changes

@hibatallahAouadni hibatallahAouadni hibatallahAouadni left review comments

@PierreRambaud PierreRambaud PierreRambaud approved these changes

@jolelievre jolelievre Awaiting requested review from jolelievre

Assignees

@hibatallahAouadni hibatallahAouadni

Labels
1.7.7.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved Wording ✔️ Status: check done, wording approved
Projects
None yet
Milestone
1.7.7.5
Development

Successfully merging this pull request may close these issues.

BO - Orders - Order message - Order message with the same title is not displayed in the "order_message" list
7 participants
@Progi1984 @prestonBot @hibatallahAouadni @PierreRambaud @sowbiba @jolelievre @Julievrz