Release 1.0.0 #2

vhrychenko · 2018-06-13T16:51:17Z

Developer(s): @valerio8787 @vhrychenko
Peer Reviewer:
Tickets:
https://spryker.atlassian.net/browse/ECO-777
Academy PR:

Please confirm

No new OS components - or they have been approved by the legal department
All new or heavily changed classes get an "A" rating (Scrutinizer), except Facades, Factories, QueryContainers and Tests

Documentation

Functional documentation provided or in progress?
Integration guide for projects provided or not needed?
Migration guides for all contained majors provided or not needed?

Release Table

Module	Release Type	Constraints
AkeneoPim	major

Release Notes

1.0 version for akeneo-pim module.

Module AkeneoPim

Patch: Backwards-compatible bug fix

Def of done (by the responsible developer):

All changes are backwards-compatible. The outdated code is marked as deprecated.
The change is isolated and not mixed with any cleanups or other changes.
New code fits the architecture rules.
All new or changed business logic is covered by functional tests (in Zed business layer).

Change log

1.0 version for akeneo-pim module.

* added adapters for all Akeneo APIs

* implemented AkeneoPimService

* added AkeneoResourceCursor wrapper * added AkeneoResourcePage wrapper

[WIP] Eco-777 integration with Akeneo PIM

gerner-spryker

I really liked it how the adapted client is wrapped everywhere, but it still feels a bit mixed in some places. The Akenea namespace is introduced in the middle of the module, it feels a bit wrong. I will try to come up with an actual alternative proposal if i can.

gerner-spryker · 2018-06-14T15:40:18Z

composer.json

@@ -2,7 +2,8 @@
  "name": "spryker-eco/akeneo-pim",
  "description": "Akeneo PIM Integration Module",
  "require": {
-    "akeneo/api-php-client": "^1.0@beta"
+    "akeneo/api-php-client": "^1.0@beta",
+    "php-http/guzzle6-adapter": "*"


please clarify version

kernel is also missing

kernel is added

gerner-spryker · 2018-06-14T16:11:31Z

.gitignore

@@ -0,0 +1,10 @@
+# IDEs


The config.dist.php feels much more like an integration/migration guide. Why was it provided in the code base?

Also jobs.dist.php

I agree with this notice. Removed.

gerner-spryker · 2018-06-14T16:13:26Z

composer.json

@@ -2,7 +2,8 @@
  "name": "spryker-eco/akeneo-pim",
  "description": "Akeneo PIM Integration Module",
  "require": {


php >=7.1 is missing

gerner-spryker · 2018-06-14T16:14:32Z

src/SprykerEco/Shared/AkeneoPim/AkeneoPimConstants.php

@@ -13,11 +13,9 @@
 */


Remove default class comment

gerner-spryker · 2018-06-14T16:14:53Z

src/SprykerEco/Shared/AkeneoPim/AkeneoPimConstants.php

@@ -13,11 +13,9 @@
 */
 class AkeneoPimConstants
 {
-
    const HOST = 'akeneo.host';


public access modifier is missing from constants (php7.1>)

gerner-spryker · 2018-06-15T06:26:11Z

src/SprykerEco/Service/AkeneoPim/Api/Adapter/Attributes/AttributeApiAdapter.php

+    }
+
+    /**
+     * @inheritdoc


{@inheritdoc}

https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html

gerner-spryker · 2018-06-15T07:02:34Z

src/SprykerEco/Service/AkeneoPim/Api/Adapter/AbstractApiAdapter.php

+
+use Akeneo\Pim\ApiClient\AkeneoPimClient;
+
+abstract class AbstractApiAdapter implements ApiAdapterInterface


Shouldn't this class be implemented by the other classes in this directory? Currently there is no usage

Looks like this abstract class is a redundant class here.

gerner-spryker · 2018-06-15T07:11:58Z

src/SprykerEco/Service/AkeneoPim/Api/Adapter/AdapterFactory.php

+    /**
+     * @var \SprykerEco\Service\AkeneoPim\Api\Wrapper\WrapperFactoryInterface
+     */
+    private $wrapperFactory;


why private?

Changed to protected

gerner-spryker · 2018-06-15T07:14:03Z

src/SprykerEco/Service/AkeneoPim/AkeneoPimService.php

+     * @param int $pageSize The size of the page returned by the server.
+     * @param array $queryParameters Additional query parameters to pass in the request
+     *
+     * @return \Akeneo\Pim\ApiClient\Pagination\ResourceCursorInterface


Please fix the return types

What is wrong here?

Service returns a 3rd party class, and actually it doesn't :)

gerner-spryker · 2018-06-15T07:21:05Z

.gitignore

+*.sublime-*
+*.AppleDouble
+*.AppleDB
+*.AppleDesktop


please add phpstan level 7 check (phpstan.json)

ECO-784 tests

PhilinTv · 2018-06-27T13:35:08Z

src/SprykerEco/Service/AkeneoPim/AkeneoPimServiceFactory.php

+    /**
+     * @return \SprykerEco\Service\AkeneoPim\Api\Adapter\AdapterFactoryInterface
+     */
+    public function createAkeneoPimAdapterFactory()


PHP7 Typehints please

PhilinTv · 2018-06-27T13:38:28Z

src/SprykerEco/Service/AkeneoPim/Api/Adapter/Attributes/AttributeApiAdapter.php

+    }
+
+    /**
+     * @inheritdoc


Where are curved brackets?

PhilinTv

Injected namespaces should be in dependency folder.
Please move adapters to Modules/Dependency/External.
Apply PHP7 convention

const visibility
typehints

dereuromark · 2018-09-03T12:34:46Z

.scrutinizer.yml

+build:
+    dependencies:
+        before:
+            - composer config repositories.spryker composer https://code.spryker.com/repo/private


Please remove outdated repo info.

dereuromark · 2018-09-03T12:35:04Z

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2018 SPRYKER SYSTEMS GMBH


This is not the right template.
Please see confluence!

valerio8787 and others added 11 commits February 6, 2018 10:59

feature/ECO-1074-adapters-for-api-client

b26b90a

* added adapters for all Akeneo APIs

feature/ECO-1078-service-layer

3a6b9f0

* implemented AkeneoPimService

feature/ECO-1074-adapters-for-api-client

69fe6d4

* added AkeneoResourceCursor wrapper * added AkeneoResourcePage wrapper

ECO-1078 Add travis and scrutinizer files

3f1bce8

ECO-1078 Add .license and fix phpstan errors

73f443d

ECO-1078 Add tests folder and fix phpmd

cf18559

ECO-1078 Add codeception.yml

664faf5

ECO-1078 Update codeception.yml

da4e280

ECO-1078 Fix scrutinizer.yml

822215e

ECO-1078 Fix scrutinizer.yml

07c3545

Merge pull request #1 from spryker-eco/feature/ECO-1078-service-layer

6c07211

[WIP] Eco-777 integration with Akeneo PIM

alex-galych requested a review from gerner-spryker June 14, 2018 15:24

ECO-784 Tests for akeneo pim service

2651c55

gerner-spryker requested changes Jun 15, 2018

View reviewed changes

vhrychenko added 11 commits June 19, 2018 23:16

dev Some fixes from code review

620260a

dev Update minimum stability (fix scrutinizer)

ac82614

dev Update the main part of comments from review

fb03e91

dev Add php-http

629f4e5

dev Update interfaces and constants

ca5475e

dev Update interfaces and constants

20cf6ce

dev Update interfaces and constants

6dba25f

Merge branch 'development' into feature/ECO-784-tests

6011a7b

ECO-784 Update tests

96baf58

Merge pull request #3 from spryker-eco/feature/ECO-784-tests

73b07e8

ECO-784 tests

dev Update fixes according to phpmd

c00a444

PhilinTv reviewed Jun 27, 2018

View reviewed changes

src/SprykerEco/Service/AkeneoPim/Api/Adapter/Attributes/AttributeApiAdapter.php Outdated

}

/**

* @inheritdoc

Copy link

PhilinTv Jun 27, 2018

Choose a reason for hiding this comment

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

Where are curved brackets?

PhilinTv requested changes Jun 27, 2018

View reviewed changes

dereuromark and others added 2 commits June 28, 2018 11:27

Update .license

ea08aed

dev Fix inheritdoc, vm and move adapters to external namespace

a063eaf

vhrychenko added 5 commits July 9, 2018 13:33

dev Update license file

b36a609

dev Fix code sniff errors

ddde502

dev Fix phpstan errors

daff663

dev Typehints

984ac12

dev Fix tests

c28292b

alex-galych approved these changes Jul 17, 2018

View reviewed changes

dev Add LICENSE and fix ecoci rules

1ff6f12

alex-galych merged commit f715a2c into master Aug 3, 2018

dereuromark reviewed Sep 3, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 1.0.0 #2

Release 1.0.0 #2

vhrychenko commented Jun 13, 2018 •

edited

Loading

gerner-spryker left a comment

gerner-spryker Jun 14, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 14, 2018

gerner-spryker Jun 14, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 14, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 14, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 14, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 20, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 19, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 20, 2018

gerner-spryker Jun 20, 2018

gerner-spryker Jun 15, 2018

vhrychenko Jun 20, 2018

PhilinTv Jun 27, 2018

PhilinTv Jun 27, 2018

PhilinTv left a comment •

edited

Loading

dereuromark Sep 3, 2018

dereuromark Sep 3, 2018


		use Akeneo\Pim\ApiClient\AkeneoPimClient;

		abstract class AbstractApiAdapter implements ApiAdapterInterface

		@@ -0,0 +1,21 @@
		MIT License

		Copyright (c) 2018 SPRYKER SYSTEMS GMBH

Release 1.0.0 #2

Release 1.0.0 #2

Conversation

vhrychenko commented Jun 13, 2018 • edited Loading

Please confirm

Documentation

Release Table

Release Notes

Module AkeneoPim

Change log

gerner-spryker left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilinTv left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhrychenko commented Jun 13, 2018 •

edited

Loading

PhilinTv left a comment •

edited

Loading