CheckoutAgreements getList refactoring #13221

sidolov · 2018-01-16T14:57:18Z

Description

Introduced new listing interface for Checkout Agreements with ability to set search criteria.

Manual testing scenarios

Covered with api-functional tests
Verify main cases with agreements on checkout flow

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)

ishakhsuvarov

Please consider using aliases instead of long class names where possible.

ishakhsuvarov · 2018-01-16T15:02:37Z

app/code/Magento/CheckoutAgreements/Api/CheckoutAgreementsListingInterface.php

+ *
+ * @api
+ */
+interface CheckoutAgreementsListingInterface


I would propose to rename this to CheckoutAgreementsListInterface

ishakhsuvarov · 2018-01-16T15:03:10Z

app/code/Magento/CheckoutAgreements/Api/CheckoutAgreementsListingInterface.php

+     * @param \Magento\Framework\Api\SearchCriteriaInterface $searchCriteria
+     * @return \Magento\CheckoutAgreements\Api\Data\AgreementInterface[]
+     */
+    public function getListing(\Magento\Framework\Api\SearchCriteriaInterface $searchCriteria) : array;


To remain consistent with the rest of the codebase it would make sense to rename method to getList

ishakhsuvarov · 2018-01-16T15:04:07Z

...ckoutAgreements/Model/Api/SearchCriteria/CollectionProcessor/FilterProcessor/StoreFilter.php

+namespace Magento\CheckoutAgreements\Model\Api\SearchCriteria\CollectionProcessor\FilterProcessor;
+
+class StoreFilter implements
+    \Magento\Framework\Api\SearchCriteria\CollectionProcessor\FilterProcessor\CustomFilterInterface


I suggest using alias for this long name.

ishakhsuvarov · 2018-01-16T15:05:12Z

app/code/Magento/CheckoutAgreements/Model/CheckoutAgreementsListing.php

+        \Magento\Framework\Api\SearchCriteria\CollectionProcessorInterface $collectionProcessor
+    ) {
+        $this->collectionFactory = $collectionFactory;
+        $this->extAttributesJoinProcessor = $extensionAttributesJoinProcessor;


Please avoid shortened names like extAttributes as it may be unclear what it is supposed to do

ishakhsuvarov · 2018-01-16T15:06:36Z

app/code/Magento/CheckoutAgreements/Api/CheckoutAgreementsListingInterface.php

+namespace Magento\CheckoutAgreements\Api;
+
+/**
+ * Interface CheckoutAgreementsListingInterface


It would be great to include background for such kind of interface

ishakhsuvarov · 2018-01-16T15:10:04Z

app/code/Magento/CheckoutAgreements/etc/webapi.xml

@@ -13,4 +13,10 @@
            <resource ref="Magento_Sales::sales" />
        </resources>
    </route>
+    <route url="/V1/carts/licence/listing" method="GET">


I would suggest renaming route to list.
Please refer to Magento\Catalog\Api\CategoryListInterface to remain consistent with it.

-- review fixes

ishakhsuvarov · 2018-01-17T10:25:15Z

app/code/Magento/CheckoutAgreements/Model/Checkout/Plugin/GuestValidation.php

    ) {
        $this->agreementsValidator = $agreementsValidator;
        $this->scopeConfiguration = $scopeConfiguration;
        $this->checkoutAgreementsRepository = $checkoutAgreementsRepository;
+        $this->checkoutAgreementsList = $checkoutAgreementsList ?: ObjectManager::getInstance()->get(


Plugins are not covered by backward compatibility policy. You may add dependencies as required parameters here.

ishakhsuvarov · 2018-01-17T10:26:31Z

app/code/Magento/CheckoutAgreements/Model/Checkout/Plugin/GuestValidation.php

@@ -117,7 +158,21 @@ private function isAgreementEnabled()
            AgreementsProvider::PATH_ENABLED,
            ScopeInterface::SCOPE_STORE
        );
-        $agreementsList = $isAgreementsEnabled ? $this->checkoutAgreementsRepository->getList() : [];
+        $storeFilter = $this->filterBuilder


This piece of code occurs multiple times in this PR along with 3 dependencies to support it.
It would make sense to extract it to a separate service.

It's just workflow with searchCriteria mechanism, I think it will be useless create new service only for couple of filters

Introducing this many boilerplate code creates a challenge in maintenance of it.
It is much easier to fix/update only 1 service instead of searching all over the module in the future.

ishakhsuvarov · 2018-01-17T10:26:56Z

app/code/Magento/CheckoutAgreements/Model/Checkout/Plugin/Validation.php

    ) {
        $this->agreementsValidator = $agreementsValidator;
        $this->scopeConfiguration = $scopeConfiguration;
        $this->checkoutAgreementsRepository = $checkoutAgreementsRepository;
+        $this->checkoutAgreementsList = $checkoutAgreementsList ?: ObjectManager::getInstance()->get(


Plugins are not covered by BC policy. See above.

ishakhsuvarov · 2018-01-17T10:27:26Z

app/code/Magento/CheckoutAgreements/Model/Checkout/Plugin/GuestValidation.php

@@ -74,7 +115,7 @@ public function beforeSavePaymentInformationAndPlaceOrder(
     * @param string $email
     * @param \Magento\Quote\Api\Data\PaymentInterface $paymentMethod
     * @param \Magento\Quote\Api\Data\AddressInterface|null $billingAddress
-     * @return void


Method still seems to return void.

ishakhsuvarov · 2018-01-17T10:28:10Z

app/code/Magento/CheckoutAgreements/Model/CheckoutAgreementsList.php

+
+namespace Magento\CheckoutAgreements\Model;
+
+class CheckoutAgreementsList implements \Magento\CheckoutAgreements\Api\CheckoutAgreementsListInterface


Class description or inheritdoc would be worth adding here.

ishakhsuvarov · 2018-01-17T10:29:59Z

app/code/Magento/CheckoutAgreements/Test/Unit/Model/Checkout/Plugin/GuestValidationTest.php

+        $storeFilterMock = $this->createMock(\Magento\Framework\Api\Filter::class);
+        $activeFilterMock = $this->createMock(\Magento\Framework\Api\Filter::class);
+
+        $this->filterBuilderMock->expects($this->at(0))->method('setField')->with('store_id')->willReturnSelf();


This kind of test looks like it is intended to freeze code state but not ensure quality.
I would suggest using other approaches or even test types.

This logic also covered with integration test \Magento\CheckoutAgreements\Model\Checkout\Plugin\GuestValidationTest

This unit test may be redundant in this case. You may remove it or leave as is.

-- review fixes

CheckoutAgreements getList refactoring

3c446dc

sidolov requested a review from ishakhsuvarov January 16, 2018 14:57

ishakhsuvarov requested changes Jan 16, 2018

View reviewed changes

CheckoutAgreements getList refactoring

44e7090

-- review fixes

ishakhsuvarov requested changes Jan 17, 2018

View reviewed changes

ishakhsuvarov self-assigned this Jan 17, 2018

Stanislav Idolov added 3 commits January 17, 2018 16:59

CheckoutAgreements getList refactoring

8f293fd

-- review fixes

CheckoutAgreements getList refactoring

47a7406

CheckoutAgreements getList refactoring

3dc9dc5

sidolov force-pushed the agreements-refactoring branch from ff20fff to 3dc9dc5 Compare January 18, 2018 07:10

Fixed constructor docBlock to be compatible with constructor

fc1d8f0

ishakhsuvarov approved these changes Jan 19, 2018

View reviewed changes

ishakhsuvarov added this to the January 2018 milestone Jan 19, 2018

ishakhsuvarov added the Release Line: 2.3 label Jan 19, 2018

magento-team added the Progress: accept label Jan 22, 2018

magento-team merged commit fc1d8f0 into magento:2.3-develop Jan 22, 2018

magento-team pushed a commit that referenced this pull request Jan 22, 2018

MAGETWO-86894: CheckoutAgreements getList refactoring #13221

d05dc82

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckoutAgreements getList refactoring #13221

CheckoutAgreements getList refactoring #13221

sidolov commented Jan 16, 2018 •

edited

Loading

ishakhsuvarov left a comment

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 16, 2018

ishakhsuvarov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

sidolov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

ishakhsuvarov Jan 17, 2018

sidolov Jan 17, 2018

ishakhsuvarov Jan 17, 2018


		namespace Magento\CheckoutAgreements\Model;

		class CheckoutAgreementsList implements \Magento\CheckoutAgreements\Api\CheckoutAgreementsListInterface

CheckoutAgreements getList refactoring #13221

CheckoutAgreements getList refactoring #13221

Conversation

sidolov commented Jan 16, 2018 • edited Loading

Description

Manual testing scenarios

Contribution checklist

ishakhsuvarov 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

sidolov commented Jan 16, 2018 •

edited

Loading