Skip to content
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.

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

Adding product from wishlist not adding to cart showing warning message. #19653

Merged
merged 9 commits into from
Apr 29, 2019

Conversation

khodu
Copy link
Contributor

@khodu khodu commented Dec 8, 2018

Description (*)

Adding product from wishlist not adding to cart showing warning message.

Fixed Issues (if relevant)

  1. Adding product from wishlist not adding to cart showing warning message. #9155: Adding product from wishlist not adding to cart showing warning message.

Manual testing scenarios (*)

  1. Add a simple product from admin with Minimum Qty Allowed in Shopping Cart 10
  2. Login in front end
  3. Add product to wishlist
  4. Click on wishlist.
  5. Now showing allow qty as default in qty field.

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)

@magento-engcom-team
Copy link
Contributor

Hi @khodu. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.1 milestone Dec 19, 2018
@magento-engcom-team
Copy link
Contributor

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

@sivaschenko
Copy link
Member

Hi @khodu , there are a couple of issues that we faced while testing the PR, can you please take a look at them:

Issue 1: The 'Qty' value is not saved after updating on 'My Wish List' page
Manual testing scenario:

  • Create simple product
  • Create customer and Login in Storefront
  • Add product to wishlist
  • On 'My Wish List' page change 'Qty' field value for product
  • Click 'Update Wish List' button

Issue 2: The 'Comment' value is not deleted after updating on 'My Wish List' page
Manual testing scenario:

  • Create simple product
  • Create customer and Login in Storefront
  • Add product to wishlist
  • On 'My Wish List' page add Comment for product
  • Click 'Update Wish List' button
  • Remove the created comment
  • Click 'Update Wish List' button

patch-9155 : set default qty for add to cart from wishlist.

patch-9155 : set default qty for add to cart in wishlist.
@khodu
Copy link
Contributor Author

khodu commented Jan 12, 2019

Hi @sivaschenko

I have fixed the first issue now if qty value set is less than minimum qty for cart then it will not update but it will update the qty value if it is more than the minimum qty for cart then it will update.

for second issue i have checked in default magento 2.3 and test their and it is also generating their. so it is not relevant to this commit.

Please test my changes and if you find any issue please tell me in comment.
Thanks.

@okorshenko okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019
@ghost ghost added the Progress: accept label Mar 19, 2019
@sivaschenko sivaschenko changed the base branch from 2.3-develop to 2.2-develop March 20, 2019 13:12
@sivaschenko sivaschenko changed the base branch from 2.2-develop to 2.3-develop March 20, 2019 13:12
@sivaschenko
Copy link
Member

There is a unit test failure on this pull request:

paypal/js/view/payment/method-renderer/paypal-express-abstract check smart button initialization.express-checkout-smart-buttons is initialized (from paypal_js_view_payment_method-renderer_paypal-express-abstract check smart button initialization)

TypeError: undefined is not an object (evaluating 'window.checkoutConfig.quoteData['entity_id']') in http://localhost:8001/pub/static/frontend/Magento/luma/en_US/Magento_Paypal/js/view/payment/method-renderer/in-context/checkout-express.js (line 76)

@khodu
Copy link
Contributor Author

khodu commented Mar 30, 2019

Hello Sergii,

I haven't change in any Js files. can you please check my pull requests.
Thanks.

@m2-assistant
Copy link

m2-assistant bot commented Apr 29, 2019

Hi @khodu, 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.

array $data = [],
?ConfigInterface $config = null,
?UrlBuilder $urlBuilder = null,
?View $productView = null
Copy link
Contributor

Choose a reason for hiding this comment

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

@sivaschenko @torhoehn injected variables must not be nullable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@orlangur why not?
There are only two required parameters in \Magento\Wishlist\Block\AbstractBlock::__construct.
So theoretically it should also be possible to instantiate the \Magento\Wishlist\Block\Customer\Wishlist\Item\Column\Cart with only two params.
Thus we also need to make the corresponding parameters nullable in this case, don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmytro-ch because we never want to pass null explicitly. It is needed only as a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it now. It does make sense.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants