Skip to content

Refactor InventoryDistanceBasedSourceSelection plugin to handle exceptions #2957

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
Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Refactor InventoryDistanceBasedSourceSelection plugin to handle exceptions #2957

wants to merge 8 commits into from

Conversation

@TadeuRodrigues
Copy link
Member

@TadeuRodrigues TadeuRodrigues commented Apr 12, 2020

Description (*)

Refactory InventoryDistanceBasedSourceSelection plugin to show the exceptions

Fixed Issues (if relevant)

  1. Refactor InventoryDistanceBasedSourceSelection plugin to handle exceptions instead of silent fail #2930: Refactor InventoryDistanceBasedSourceSelection plugin to handle exceptions instead of silent fail

Manual testing scenarios (*)

  1. save a source or create a new one
  2. change the 'Store>Configuration>Catalog>Inventory>Distance Provider for Distance Based SSA/Provider' to 'offiline'

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)

@TadeuRodrigues TadeuRodrigues force-pushed the refactor_InventoryDistanceBasedSourceSelection_2930 branch from 4558221 to 495af2b Compare April 15, 2020 22:13
@TadeuRodrigues TadeuRodrigues force-pushed the refactor_InventoryDistanceBasedSourceSelection_2930 branch from 495af2b to 4ad6561 Compare April 16, 2020 00:22
@m2-community-project
Copy link

m2-community-project bot commented May 28, 2020

@TadeuRodrigues unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@novikor novikor self-requested a review August 3, 2020 08:08
@novikor
Copy link
Contributor

novikor commented Aug 3, 2020

@magento run all tests

novikor
novikor suggested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@novikor novikor left a comment

Choose a reason for hiding this comment

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

Hi, @TadeuRodrigues! Thank you for your contribution, we appreciate that.
There are a few things to improve, please check my notes below.

@novikor
Copy link
Contributor

novikor commented Aug 3, 2020

@magento run all tests

1 similar comment
@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Aug 4, 2020

@magento run all tests

@novikor
Copy link
Contributor

novikor commented Aug 4, 2020

@magento run WebAPI Tests

@novikor
Copy link
Contributor

novikor commented Aug 4, 2020

@magento run Functional Tests CE

novikor
novikor approved these changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@novikor novikor left a comment

Choose a reason for hiding this comment

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

Good job! Thank you for your contribution.
The 'Semantic Version Checker' build failure is not related to the PR changes.

novikor
novikor suggested changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@novikor novikor left a comment

Choose a reason for hiding this comment

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

Have just noticed that we use message manager out of Presentation Layer scope which is not acceptable.
I will think about possible solutions.

novikor
novikor suggested changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@novikor novikor left a comment

Choose a reason for hiding this comment

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

I suggest the following solution:

  • Move the current plugin to adminhtml area.
  • Create the same plugin for global area BUT without messageManager usage, and log ANY exception type there.

@TadeuRodrigues TadeuRodrigues requested a review from novikor August 8, 2020 03:12
novikor
novikor reviewed Aug 9, 2020
View reviewed changes
Copy link
Contributor

@novikor novikor 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. Let's wait for the test builds.

@novikor
Copy link
Contributor

novikor commented Aug 9, 2020

@magento run all tests

1 similar comment
@novikor
Copy link
Contributor

novikor commented Aug 10, 2020

@magento run all tests

novikor
novikor previously approved these changes Aug 11, 2020
View reviewed changes
@sidolov
Copy link
Contributor

sidolov commented Nov 9, 2020

@magento run all tests

@novikor
Copy link
Contributor

novikor commented Nov 9, 2020

@magento run Functional Tests EE

@novikor
Copy link
Contributor

novikor commented Jan 18, 2021

Hi, @sidolov. Are there any updates?..

The failed Semantic Version Checker test is not related to the PR changes.

@sidolov
Copy link
Contributor

sidolov commented Jan 19, 2021

@magento run all tests

1 similar comment
@novikor
Copy link
Contributor

novikor commented Jan 20, 2021

@magento run all tests

@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 20, 2021

Captura de tela de 2021-01-20 10-54-04

@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 20, 2021

@magento run all tests

@novikor
Copy link
Contributor

novikor commented Jan 21, 2021 •
edited
Loading

Semantic Version Checker failure is not related to the PR changes, as well as functional tests (AdminAddRemoveDefaultVideoGiftCardProductTest
AdminAddRemoveDefaultVideoVirtualProductTest
AdminAddRemoveDefaultVideoBundleProductTest
StorefrontGalleryConfigurableProductWithVisualSwatchAttributePrependMediaTest
StorefrontGalleryConfigurableProductWithSeveralAttributesPrependMediaTest).

There is something wrong with Integration Tests (failed elasticsearch connection), more likely will be green after a restart.

@novikor
Copy link
Contributor

novikor commented Jan 21, 2021

@magento run Integration Tests

@novikor
Copy link
Contributor

novikor commented Jan 21, 2021

@magento run Functional Tests CE

1 similar comment
@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 26, 2021

@magento run Functional Tests CE

@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 27, 2021

@magento run Functional Tests EE

@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 27, 2021

@magento run Functional Tests B2B

1 similar comment
@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 27, 2021

@magento run Functional Tests B2B

@TadeuRodrigues
Copy link
Member Author

TadeuRodrigues commented Jan 28, 2021

@magento run Semantic Version Checker

@sidolov sidolov changed the base branch from 1.2-develop to develop November 11, 2021 17:51
@sidolov sidolov dismissed novikor’s stale review November 11, 2021 17:51

The base branch was changed.

@engcom-Hotel engcom-Hotel moved this to Review in Progress in Inventory - Pull Request Progress Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phoenix128 phoenix128 Awaiting requested review from phoenix128

@sidolov sidolov Awaiting requested review from sidolov

1 more reviewer

@novikor novikor novikor left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@novikor novikor

Labels

Progress: review

Projects

Inventory - Pull Request Progress
Status: Review in Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TadeuRodrigues @novikor @sidolov