Allow wishlist share when all items are out of stock #26185

pmclain · 2019-12-26T20:04:44Z

Description (*)

Clicking Share on the wishlist view page updates and saves the wishlist
before redirecting to the share page. The redirection to the share page only
happens when the post body includes wishlist descriptions. Because qty and
comment are disabled for out of stock wishlist items these properties are not
sent with the post resulting in the share redirect being unreachable when all
items are out of stock.

This commit updates the controller flow separating the logic conditions for
saving and sharing. This allows sharing wishlist where all items are out of
stock.

Manual testing scenarios (*)

Create simple product
Make simple product out of stock
Add product to wishlist
Confirm wishlist only contains the out of stock product
Click share

Expected Result With PR
You should be redirected to the share page for inputting email addresses and messages.

Current Behavior
You are redirected back to the wishlist view without any messaging.

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)

Clicking `Share` on the wishlist view page updates and saves the wishlist before redirecting to the share page. The redirection to the share page only happens when the post body includes wishlist descriptions. Because qty and comment are disabled for out of stock wishlist items these properties are not sent with the post resulting in the share redirect being unreachable when all items are out of stock. This commit updates the controller flow separating the logic conditions for saving and sharing. This allows sharing wishlist where all items are out of stock.

m2-assistant · 2019-12-26T20:04:46Z

Hi @pmclain. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

lbajsarowicz

Please cover the change with automated tests.

pmclain · 2019-12-27T13:32:12Z

@lbajsarowicz This class is currently lacking unit/integration test coverage entirely. I have no desire to create a set of test cases from scratch. This PR is only open because it was faster to fix the issue myself then spending 1-2 weeks arguing with Magento support for a patch.

lbajsarowicz

I feel your pain. I'll develop MFTF test for that change.

magento-engcom-team · 2019-12-28T20:52:38Z

Hi @lbajsarowicz, thank you for the review.
ENGCOM-6527 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2019-12-30T09:15:47Z

✔️ QA passed

pmclain · 2019-12-30T13:16:06Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    /**
+     * Test for update method Wishlist controller.
+     *
+     * Check if there is not post value result redirect returned.


The controller will return an redirect regardless of the post value unless the requested wishlist does not exist, in which case a \Magento\Framework\Exception\NotFoundException is thrown. What if this test is updated for validating the logic within the controller? This may also be a better use case for an integration test over a unit test.

pmclain · 2019-12-30T13:24:56Z

@engcom-Golf Feel free to close. I'll open a support ticket for the impacted clients.

engcom-Golf · 2019-12-30T13:48:17Z

As we discussed on "slack", i will cover more cases with test

pmclain · 2019-12-31T11:58:03Z

@engcom-Golf thanks! looks good to me.

engcom-Golf · 2020-01-02T09:44:13Z

Hi @lbajsarowicz could you please review new changes ?

engcom-Golf · 2020-01-09T10:07:58Z

@magento run all tests

lbajsarowicz

You need to:

extract magic numbers to const with correct names
replace class attributes names to be explicit when it's mock ($someClassMock) and doc blocks to add MockObject|

testUpdate is completely unreadable. Try to make it clear what it actually does. It's recommended to use 3 sections:

given
when
then

For example

// Given
$this->requestMock->method('getPostValue)->willReturn($postData);
...
$itemMock...

// When
$itemMock->method('load')->with(self::STUB_ITEM_ID)->willReturnSelf();

// Then
$this->messageManagerMock->expects($this->once())->method('addSuccessMessage');
$controllerActionResult = $this->updateController->execute();
$this->assertEquals($this->resultRedirectMock, $controllerActionResult);

Then it's clear what is actually mocked (given), what are our assumptions for the test case (when) and what are expectations (then).

lbajsarowicz · 2020-01-10T07:17:28Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    /**
+     * @var Validator $formKeyValidator
+     */
+    private $formKeyValidator;


formKeyValidatorMock

lbajsarowicz · 2020-01-10T07:17:37Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+class UpdateTest extends TestCase
+{
+    /**
+     * @var Validator $formKeyValidator


MockObject|Validator

lbajsarowicz · 2020-01-10T07:17:58Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    private $formKeyValidator;
+
+    /**
+     * @var WishlistProviderInterface $wishlistProvider


MockObject|WishlistProviderInterface

lbajsarowicz · 2020-01-10T07:18:38Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+            ->method('getMessageManager')
+            ->willReturn($this->messageManager);
+
+        $this->updateController = new Update(


Use ObjectManager helper for building the tested object.

lbajsarowicz · 2020-01-10T07:19:19Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+            [
+                [
+                    'wishlist_data' => [
+                        'id' => 1,


Replace magic numbers with const - for example STUB_PRODUCT_ID

lbajsarowicz · 2020-01-10T07:26:57Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+     * @dataProvider getWishlistDataProvider
+     * @return void
+     */
+    public function testUpdate(array $wishlistDataProvider): void


Instead of using wishlistDataProvider['wishlist_data'] and wishlistDataProvider['post_data'] just extract them as function parameters: testUpdateForPostRequest(array $wishlistDataProvider, array $postData) and introduce verification if no POST data provided: testUpdateRedirectWhenNoPostData().

lbajsarowicz · 2020-01-10T07:28:48Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    private $wishlistProvider;
+
+    /**
+     * @var LocaleQuantityProcessor $quantityProcessor


MockObject|LocaleQuantityProcessor

lbajsarowicz · 2020-01-10T07:28:59Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    /**
+     * @var LocaleQuantityProcessor $quantityProcessor
+     */
+    private $quantityProcessor;


quantityProcessorMock

lbajsarowicz · 2020-01-10T07:29:13Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    /**
+     * @var RequestInterface $requestMock
+     */
+    private $requestMock;


lbajsarowicz · 2020-01-10T07:29:22Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    private $resultFactory;
+
+    /**
+     * @var RequestInterface $requestMock


MockObject|RequestInterface

lbajsarowicz

Small changes and we're ready to go!

lbajsarowicz · 2020-01-10T10:41:35Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

-            $this->formKeyValidator,
-            $this->wishlistProvider,
-            $this->quantityProcessor
+        $this->updateController = (new ObjectManagerHelper($this))->getObject(


Extract this to separate variable:
$objectManager = new ObjectManagerHelper($this);

lbajsarowicz · 2020-01-10T10:47:06Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+     * Wishlist item id
+     *
+     * @var int
+     */


We don't use Docblocks for consts

lbajsarowicz · 2020-01-10T10:47:37Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+     *
+     * @var int
+     */
+    private const ITEM_ID = 1;


Prefix with STUB_ as it means that is just "stupid replacement" for actual value, that does not matter for tests.

lbajsarowicz · 2020-01-10T10:47:54Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

+    /**
+     * Product qty for wishlist
+     *
+     * @var int


We don't use docblocks for const

lbajsarowicz · 2020-01-10T10:48:02Z

app/code/Magento/Wishlist/Test/Unit/Controller/Index/UpdateTest.php

     */
-    private $formKeyValidator;
+    private const WISHLIST_PRODUCT_QTY = 21;


Prefix with STUB_

variable

magento-engcom-team · 2020-01-10T11:12:41Z

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

m2-assistant · 2020-01-17T18:14:00Z

Hi @pmclain, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

pmclain requested a review from paliarush as a code owner December 26, 2019 20:04

magento-engcom-team added Partner: Something Digital partners-contribution Pull Request is created by Magento Partner labels Dec 26, 2019

Fix static tests

860af38

lbajsarowicz suggested changes Dec 27, 2019

View reviewed changes

ghost assigned lbajsarowicz Dec 27, 2019

ghost added the Progress: needs update label Dec 27, 2019

lbajsarowicz approved these changes Dec 28, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Dec 28, 2019

magento-engcom-team added Release Line: 2.4 Component: Wishlist labels Dec 28, 2019

engcom-Delta self-assigned this Dec 30, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Dec 30, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Dec 30, 2019

engcom-Golf added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Dec 30, 2019

engcom-Golf self-assigned this Dec 30, 2019

Cover changes with Unit test

21bdc67

engcom-Golf added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Dec 30, 2019

pmclain commented Dec 30, 2019

View reviewed changes

pmclain requested a review from lbajsarowicz December 30, 2019 13:16

pmclain added the Progress: needs update label Dec 30, 2019

ghost removed the Progress: extended testing label Dec 30, 2019

Nazar65 force-pushed the hotfix/oos-share-wishlist branch 3 times, most recently from befc091 to 3c3018d Compare December 30, 2019 16:23

Cover changes with unit test

3c3018d

Nazar65 added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Dec 30, 2019

pmclain removed the Progress: needs update label Dec 31, 2019

krishprakash mentioned this pull request Jan 9, 2020

Can not share product if only out of stock product in wish list #26324

Closed

lbajsarowicz suggested changes Jan 10, 2020

View reviewed changes

Refactor per review comments, add new test case

74a244f

lbajsarowicz suggested changes Jan 10, 2020

View reviewed changes

rename const with prefix "stub" move Object Manager to separate

1748633

variable

lbajsarowicz approved these changes Jan 10, 2020

View reviewed changes

ghost added the Progress: ready for testing label Jan 10, 2020

ghost added Progress: testing in progress Progress: extended testing Progress: accept and removed Progress: ready for testing Progress: testing in progress Progress: extended testing labels Jan 13, 2020

magento-engcom-team pushed a commit that referenced this pull request Jan 17, 2020

ENGCOM-6527: Allow wishlist share when all items are out of stock #26185

4d0bb13

magento-engcom-team merged commit 1748633 into magento:2.4-develop Jan 17, 2020

Allow wishlist share when all items are out of stock #26185

Allow wishlist share when all items are out of stock #26185

Conversation

pmclain commented Dec 26, 2019 • edited by antoinekociuba Loading

Description (*)

Manual testing scenarios (*)

Contribution checklist (*)

m2-assistant bot commented Dec 26, 2019

lbajsarowicz left a comment • edited Loading

Choose a reason for hiding this comment

pmclain commented Dec 27, 2019

lbajsarowicz left a comment

Choose a reason for hiding this comment

magento-engcom-team commented Dec 28, 2019

engcom-Delta commented Dec 30, 2019

Choose a reason for hiding this comment

pmclain commented Dec 30, 2019

engcom-Golf commented Dec 30, 2019

pmclain commented Dec 31, 2019

engcom-Golf commented Jan 2, 2020

engcom-Golf commented Jan 9, 2020

lbajsarowicz 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

lbajsarowicz 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

magento-engcom-team commented Jan 10, 2020

m2-assistant bot commented Jan 17, 2020

pmclain commented Dec 26, 2019 •

edited by antoinekociuba

Loading

lbajsarowicz left a comment •

edited

Loading