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

Fixed #26345 Reorder in Admin panel leads to report page in case of changed product custom option max characters #26348

Merged
merged 7 commits into from
Mar 17, 2020

Conversation

cedmudit
Copy link
Contributor

Fixed #26345 Reorder in Admin panel leads to report page in case of changed product custom option max characters

Preconditions (*)

Magento ver. 2.3.2-p2.

Steps to reproduce (*)

Configure a product with text (area) custom option, that has "Max Characters" value = 15.
Place an order with the configured product.
2.1. The text custom option value should be at least 11 symbols length.
Change the value of the product text custom option "Max Characters" to 10 (or less).
Go to the placed order in Admin panel.
Click on "Reorder" button.

Expected result (*)

The Admin user see a clear error message, that explains, why the order couldn't reordered.
Probably it's enough to implement it in the same way as on frontend for registered customer:
registered customer - reorder after changing max characters - error message
Actual result (*)

The Admin user see a report page

Description (*)

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Update Reorder.php so that it can handle the Localized Exception and added try catch block
@m2-assistant
Copy link

m2-assistant bot commented Jan 10, 2020

Hi @cedmudit. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@rogyar rogyar self-assigned this Jan 11, 2020
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @cedmudit. Thank you for your contribution. Could I kindly ask you to cover your change using an automated test? I would recommend a unit test or MFTF test in this particular case.

Also, consider checking the failing static tests as well.

Thank you!

@engcom-Echo engcom-Echo self-assigned this Feb 6, 2020
@engcom-Echo
Copy link
Contributor

I will take care of test coverage.

@engcom-Echo engcom-Echo added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Feb 6, 2020
@rogyar
Copy link
Contributor

rogyar commented Feb 9, 2020

@magento run functional tests

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

It looks like the reorder functionality for MSI has been affected by this change. Please, take a look at the failing functional tests.

@engcom-Echo
Copy link
Contributor

@magento run all tests

@rogyar
Copy link
Contributor

rogyar commented Feb 22, 2020

Looks like they were just fluky tests

rogyar
rogyar previously requested changes Feb 22, 2020
$this->_getSession()->setUseOldShippingMethod(true);
$this->_getOrderCreateModel()->initFromOrder($order);
$resultRedirect->setPath('sales/*');
} catch (\Magento\Framework\Exception\LocalizedException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @cedmudit. I've noticed that we don't log the exception. It's not that critical in case of LocalizedException since we show the message using the messageManager. However, it's still recommended to log the exception.

In the case of catching \Exception it's very important to have the details in the error log. Otherwise, we just catch the exception and hide important error details.

Please, add error logging for both cases. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @rogyar. I will add an error logging.

@ghost ghost dismissed rogyar’s stale review February 22, 2020 13:49

Pull Request state was updated. Re-review required.

@rogyar
Copy link
Contributor

rogyar commented Mar 13, 2020

The failing tests are not related to the current PR since we have the same tests failing for other PRs

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-7090 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Echo
Copy link
Contributor

Failed functional tests not related to the changes in this PR

@m2-assistant
Copy link

m2-assistant bot commented Mar 17, 2020

Hi @cedmudit, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
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.

Reorder in Admin panel leads to report page in case of changed product custom option max characters
5 participants