Add default Source in Data Install. #107

bartekszymanski · 2017-09-26T20:56:53Z

Description

Fixed Issues (if relevant)

Add default Source in Data Install. #41: Add default Source in Data Install

Manual testing scenarios

Remove data version from table "setup_module", Magento_InventoryCatalog module
Run bin/magento setup:upgrade
Default source should be visible in table "inventory_source" with id: 1

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)

larsroettig · 2017-09-26T21:47:50Z

app/code/Magento/Inventory/Setup/InstallData.php

+        DataObjectHelper $dataObjectHelper
+    ) {
+        $this->sourceRepository = $sourceRepository;
+        $this->objectManager    = $objectManager;


⁉️ 💥 Pls don't use the object manager inject the SourceInterface

larsroettig · 2017-09-26T21:48:57Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+        self::assertEquals('Default Source', $this->source->getName());
+    }
+
+}


⁉️ Coding Style Newline is missing

larsroettig · 2017-09-26T21:49:59Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+    }
+
+    /**
+     * Test is default source wxist in DB


typo in wxist you mean exist?

larsroettig · 2017-09-26T21:51:21Z

app/code/Magento/Inventory/Setup/InstallData.php

+    }
+
+    /**
+     * @return void


Pls add documentation for function

naydav · 2017-09-27T11:57:54Z

app/code/Magento/Inventory/Setup/InstallData.php

+        DataObjectHelper $dataObjectHelper
+    ) {
+        $this->sourceRepository = $sourceRepository;
+        $this->source = $source;


Pls use autogenerated factory for creating new Data objects
Unlike from services Data Objects have state and does not suppose to be shareable objects

naydav · 2017-09-27T11:59:59Z

app/code/Magento/Inventory/Setup/InstallData.php

+    private function addDefaultSource()
+    {
+        $data = [
+            SourceInterface::SOURCE_ID => 1,


Need to move default Stock data to configuration
It will provide possibility to replace this data with another's modules

I propose to leave it as-is now.

Because we need a decision from PO @mbrinton01 here, regarding Default Source data. And from where to take it.
I suppose it could be some XML configuration with an ability to overwrite it from another module.
But what the data would be stored there?

naydav · 2017-09-27T12:02:17Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+class DefaultSource extends TestCase
+{
+    /**
+     * @var Source


Wrong type
We work with repository

naydav · 2017-09-27T12:02:33Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+ * Class DefaultSource
+ * @package Magento\Inventory\Test\Integration\Source
+ */
+class DefaultSource extends TestCase


DefaultSourceTest

naydav · 2017-09-27T12:03:30Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+     */
+    protected function setUp()
+    {
+        $this->source = Bootstrap::getObjectManager()->create(SourceRepositoryInterface::class);


You should use get method instead of create because we do not need to create new instance of Repository
Services are shared objects (without any state) unlike from Data object classes

naydav · 2017-09-27T12:05:04Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+ * Class DefaultSource
+ * @package Magento\Inventory\Test\Integration\Source
+ */
+class DefaultSource extends TestCase


@maghamed
What do think, maybe is better to create WebApi test instead of Integration?
Because Default Stocks looks like part of api provided CatalogInventory bridge module

@naydav yes, web api test in InventoryCatlog module makes sense

naydav · 2017-09-27T12:07:50Z

app/code/Magento/Inventory/Setup/InstallData.php

+ * See COPYING.txt for license details.
+ */
+
+namespace Magento\Inventory\Setup;


@maghamed
If I am not mistaken we decided to put this functionality in bridge module (which @bartekszymanski introduced on Contribution Day)

@naydav good catch!

yeah, that should be placed in InventoryCatalog module

naydav · 2017-09-27T12:12:22Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+    protected function setUp()
+    {
+        $this->source = Bootstrap::getObjectManager()->create(SourceRepositoryInterface::class);
+        $this->source = $this->source->get(1);


$this->source->get(1) looks like as part of testDefaultSourceExist test
Maybe need to move from setUp (where only initialization should be placed) to corresponding test method

maghamed · 2017-09-27T12:45:29Z

app/code/Magento/Inventory/Setup/InstallData.php

+ * See COPYING.txt for license details.
+ */
+
+namespace Magento\Inventory\Setup;


@naydav good catch!

yeah, that should be placed in InventoryCatalog module

maghamed · 2017-09-27T12:47:40Z

app/code/Magento/Inventory/Setup/InstallData.php

+    private function addDefaultSource()
+    {
+        $data = [
+            SourceInterface::SOURCE_ID => 1,


I propose to leave it as-is now.

Because we need a decision from PO @mbrinton01 here, regarding Default Source data. And from where to take it.
I suppose it could be some XML configuration with an ability to overwrite it from another module.
But what the data would be stored there?

maghamed · 2017-09-27T12:49:00Z

app/code/Magento/Inventory/Test/Integration/Source/DefaultSource.php

+ * Class DefaultSource
+ * @package Magento\Inventory\Test\Integration\Source
+ */
+class DefaultSource extends TestCase


@naydav yes, web api test in InventoryCatlog module makes sense

maghamed · 2017-09-27T19:00:18Z

app/code/Magento/InventoryCatalog/Setup/InstallData.php

+            SourceInterface::LATITUDE => 0,
+            SourceInterface::LONGITUDE => 0,
+            SourceInterface::PRIORITY => 0,
+            SourceInterface::COUNTRY_ID => 'PL',


According to MSI Product Owner @mbrinton01

it should default to US, just because we have more merchants there
and postcode should be 00000
(having no - in the middle)
the other fields can be used as they are now

Where do I keep this data? Configuration xml or leave it as it is?

MC-41614: When dimensions-mode is set to "wesbite" and let price inde…

larsroettig reviewed Sep 26, 2017

View reviewed changes

naydav reviewed Sep 27, 2017

View reviewed changes

bartekszymanski force-pushed the default-source-install-data branch from e7e4897 to 1129d30 Compare September 27, 2017 12:44

maghamed suggested changes Sep 27, 2017

View reviewed changes

bartekszymanski added 3 commits September 28, 2017 23:22

default source install

a974ae0

default source install

5110df3

default source install move to InventoryCatalog

0963bfb

bartekszymanski force-pushed the default-source-install-data branch from dc06389 to 0963bfb Compare September 28, 2017 21:33

bartekszymanski and others added 3 commits September 28, 2017 23:35

add country_id = US, postcode = 00000

6253894

add suffix to test class

7f5181b

Merge branch 'develop' into default-source-install-data

feafb15

maghamed merged commit b5bac61 into develop Oct 4, 2017

vkublytskyi added Award: test coverage Award: special achievement labels Dec 14, 2017

maghamed deleted the default-source-install-data branch December 11, 2018 18:15

magento-cicd2 pushed a commit that referenced this pull request Apr 12, 2021

Merge pull request #107 from magento-tsg-csl3/MC-41614

1f59eec

MC-41614: When dimensions-mode is set to "wesbite" and let price inde…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default Source in Data Install. #107

Add default Source in Data Install. #107

bartekszymanski commented Sep 26, 2017 •

edited

larsroettig Sep 26, 2017

larsroettig Sep 26, 2017

larsroettig Sep 26, 2017

larsroettig Sep 26, 2017

naydav Sep 27, 2017 •

edited

naydav Sep 27, 2017

maghamed Sep 27, 2017

naydav Sep 27, 2017

naydav Sep 27, 2017

naydav Sep 27, 2017 •

edited

naydav Sep 27, 2017

maghamed Sep 27, 2017

naydav Sep 27, 2017 •

edited

maghamed Sep 27, 2017

naydav Sep 27, 2017

maghamed Sep 27, 2017

maghamed Sep 27, 2017

maghamed Sep 27, 2017

maghamed Sep 27, 2017

bartekszymanski Sep 27, 2017

Add default Source in Data Install. #107

Add default Source in Data Install. #107

Conversation

bartekszymanski commented Sep 26, 2017 • edited

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekszymanski commented Sep 26, 2017 •

edited

naydav Sep 27, 2017 •

edited

naydav Sep 27, 2017 •

edited

naydav Sep 27, 2017 •

edited