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

Search Conf Incorrect successful alert message #24352

Merged
merged 4 commits into from May 6, 2021
Merged

Search Conf Incorrect successful alert message #24352

merged 4 commits into from May 6, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 5, 2021

Questions Answers
Branch? develop
Description? n the BO > Search page, when we try to update an alias, the successful message is displayed. Creation successful instead of Update successful
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #21867
How to test? Check the wording
Possible impacts? Please indicate what parts of the software we need to check to make sure everything is alright.

This change is Reviewable

@ghost ghost self-requested a review as a code owner May 5, 2021 13:06
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix Waiting for wording Status: action required, waiting for wording labels May 5, 2021
PululuK
PululuK previously approved these changes May 5, 2021
Copy link
Member

@PululuK PululuK left a comment

Choose a reason for hiding this comment

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

@okom3pom LGTM !
Just improve wording.

controllers/admin/AdminSearchConfController.php Outdated Show resolved Hide resolved
Co-authored-by: Julie Varisellaz <70583503+Julievrz@users.noreply.github.com>
Co-authored-by: Julie Varisellaz <70583503+Julievrz@users.noreply.github.com>
@ghost
Copy link
Author

ghost commented May 5, 2021

Thank you @PululuK && @Julievrz

@Julievrz Julievrz added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels May 5, 2021
@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label May 5, 2021
@Robin-Fischer-PS
Copy link
Contributor

Hi there !

I think the problem is a bit more complex for this page, because the "Edit alias" can give 3 different results depending on what's modified in the form, and I'm not sure the correct message should always be "Update successful".

Ping @PrestaShop/product-team , need some help for this one !

  • Case 1 :

I edit only the "Result" field : In this case, it's indeed only an update of the alias, no new alias is created. For this case, the message is OK.
In the DB, the alias is updated too.

Edit page :
pr 24352 result mod 1

Result :
pr 24352 result mod 2

  • Case 2 :

I edit both "Alias" and "Result" field : In this case, a new alias is created in addition to the original one. IMHO, the message is not OK : It should be "Successful creation". Or the behavior should be modified and the original alias should be replaced by the new one.
In DB, a new alias is indeed created and the original one is not modified.

Edit page :
pr 24352 alias result mod 1

Result :
pr 24352 alias result mod 2

  • Case 3 :

I edit only the "Alias" field : In this case, a new alias is created in addition to the original one, but if I edit the same alias again, both aliases are displayed in the "Alias" field. In this case, I'm not sure what should be the message displayed...
In DB, a new alias is created and the original one is not modified.

Edit page :
pr 24352 alias mod 1

Result :
pr 24352 alias mod 2

Both aliases in one edit page :
pr 24352 alias mod 3

I made a screenrecord of these 3 cases :

https://drive.google.com/file/d/1GYGQw6sCNiFH7ZThWw6xRklJxi7yc1Eu/view?usp=sharing

Thanks :)

@Robin-Fischer-PS Robin-Fischer-PS added the Waiting for PM Status: action required, waiting for product feedback label May 6, 2021
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this May 6, 2021
@Robin-Fischer-PS Robin-Fischer-PS removed the Waiting for QA Status: action required, waiting for test feedback label May 6, 2021
@MatShir
Copy link
Contributor

MatShir commented May 6, 2021

There is a problem in the behavior as @Robin-Fischer-PS pointed out. In any case, this is an update workflow, so the message is correct even if the actions lead to the creation of a new alias. The behavior is not new, I could reproduce the problem in 1.7.6. So we are not going to adapt the message to a faulty behavior, but we should correct the behavior to match the action and follow the message.
The PR is good, but fixing the behavior should be the subject of another PR.

@MatShir MatShir added PM ✔️ Status: check done, behavior approved and removed Waiting for PM Status: action required, waiting for product feedback labels May 6, 2021
@Robin-Fischer-PS Robin-Fischer-PS added the Waiting for QA Status: action required, waiting for test feedback label May 6, 2021
@Robin-Fischer-PS
Copy link
Contributor

Thanks @MatShir !

Then this PR is OK :
Display of the confirmation message on edit : ✔️
Display of the confirmation message on creation : ✔️

It's QA ✔️ ! Thanks @okom3pom :)

@Robin-Fischer-PS Robin-Fischer-PS added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels May 6, 2021
@prestonBot
Copy link
Collaborator

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

@PululuK PululuK merged commit 7dda5b0 into PrestaShop:develop May 6, 2021
@PululuK
Copy link
Member

PululuK commented May 6, 2021

Thanks @okom3pom @Robin-Fischer-PS

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 PM ✔️ Status: check done, behavior approved 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 - Search - Incorrect successful alert message when update 'Search'
8 participants