Skip to content

788: Integration test rework. Introduced new extension point for New Shipment Controller to retrieve Shipment Data #14634

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
Merged
merged 3 commits into from
Apr 13, 2018
Merged

788: Integration test rework. Introduced new extension point for New Shipment Controller to retrieve Shipment Data #14634

merged 3 commits into from
Apr 13, 2018

Conversation

nmalevanec
Copy link
Contributor

Description

Integration test rework and Shipment controller refactor in order to support magento/inventory#788

Fixed Issues (if relevant)

  1. none

Manual testing scenarios

  1. none.

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 on Travis CI are green)

@maghamed maghamed changed the title 788: Integration test rework. Shipment controller refactor. 788: Integration test rework. Introduced new extension point for New Shipment Controller. Apr 11, 2018
@magento-engcom-team magento-engcom-team added this to the April 2018 milestone Apr 11, 2018
@magento-engcom-team
Copy link
Contributor

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

@maghamed
Copy link
Contributor

maghamed commented Apr 11, 2018 •
edited
Loading

In the scope of Pull Request magento/inventory#788 on MSI project, extension point (ShipmentProviderInterface) which provides Shipment Data from Request object has been introduced. This extension point supposed to be substituted in MSI.

The initial behavior of a Single Stock system is preserved and delivered as out-of-the-box implementation of ShipmentProviderInterface.

Also, there is a slight refactoring of existing Integration tests which instantiated Shipment model via ObjectManager but not via ShipmentFactory (legal entity for Shipment creation), like this
$objectManager->create(\Magento\Sales\Model\Order\Shipment::
or this
$shipment = Bootstrap::getObjectManager()->create(Shipment::class);

@maghamed maghamed changed the title 788: Integration test rework. Introduced new extension point for New Shipment Controller. 788: Integration test rework. Introduced new extension point for New Shipment Controller to retrieve Shipment Data Apr 11, 2018
@maghamed maghamed mentioned this pull request Apr 11, 2018
Merged
sidolov
sidolov requested changes Apr 12, 2018
View reviewed changes
app/code/Magento/Shipping/Controller/Adminhtml/Order/Shipment/NewAction.php Outdated
*/
public function __construct(
Action\Context $context,
\Magento\Shipping\Controller\Adminhtml\Order\ShipmentLoader $shipmentLoader
\Magento\Shipping\Controller\Adminhtml\Order\ShipmentLoader $shipmentLoader,
\Magento\Shipping\Model\ShipmentProviderInterface $shipmentProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, inject new dependencies according to Backward Compatible Development Guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

app/code/Magento/Shipping/Model/ShipmentProvider.php Outdated
/**
* @inheritdoc
*/
public function getShipment(): array
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename method to getShipmentData because the current name may confuse developers, getShipment in most cases means object instead of the array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

app/code/Magento/Shipping/Model/ShipmentProviderInterface.php Outdated
*
* @return array
*/
public function getShipment(): array;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename method to getShipmentData because the current name may confuse developers, getShipment in most cases means object instead of the array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

app/code/Magento/Shipping/Model/ShipmentProviderInterface.php Outdated
namespace Magento\Shipping\Model;

/**
* Provide shipment items data from request.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better remove the request from comment as a source for shipment data because interface shouldn't contain implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

app/code/Magento/Shipping/Model/ShipmentProviderInterface.php Outdated
interface ShipmentProviderInterface
{
/**
* Retrieve shipment items from request.
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@sidolov sidolov self-assigned this Apr 12, 2018
@nmalevanec nmalevanec mentioned this pull request Apr 12, 2018
Merged
@magento-engcom-team magento-engcom-team mentioned this pull request Apr 13, 2018
Closed
@magento-engcom-team magento-engcom-team merged commit 9cda5f0 into magento:2.3-develop Apr 13, 2018
magento-engcom-team pushed a commit that referenced this pull request Apr 13, 2018
ENGCOM-1257: 788: Integration test rework. Introduced new extension p…
208aed4 
…oint for New Shipment Controller. #14634
@magento-engcom-team
Copy link
Contributor

Hi @nmalevanec. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.3.0 release.

@nmalevanec nmalevanec deleted the 788 branch August 21, 2018 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maghamed maghamed maghamed approved these changes

@sidolov sidolov sidolov approved these changes

Assignees

@maghamed maghamed

@sidolov sidolov

Labels
Progress: accept Release Line: 2.3
Projects
None yet
Milestone
April 2018
Development

Successfully merging this pull request may close these issues.
4 participants
@nmalevanec @magento-engcom-team @maghamed @sidolov