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

imagento/magento2#8515: Downloadable product is available for downloa… #8917

Merged
merged 4 commits into from
Mar 23, 2017

Conversation

nazarpadalka
Copy link
Contributor

@nazarpadalka nazarpadalka commented Mar 16, 2017

  • Set expired status of downloaded product during change order status to canceled.

Description

Resolve issue with possibility to download downloadable product after order was canceled

Fixed Issues (if relevant)

#8515

  1. magento/magetno2#8515: Downloadable product is available for download even if order state is set canceled.

Manual testing scenarios

  1. Create an order for a downloadable product.
  2. Set order state to STATE_CANCELED programatically.
  3. Goto customer account dashboard, it list the order as 'canceled'.
  4. Now goto "My Downloadable Products" and check if possible to download downloadable product

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)

…d even if order state is set canceled.

- Set expired status of downloaded product during change order status to canceled.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 16, 2017

CLA assistant check
All committers have signed the CLA.

@nazarpadalka
Copy link
Contributor Author

We will provide test for covering this case

@ishakhsuvarov
Copy link
Contributor

Hi @nazarpadalka
We kindly ask you to sign our Contributor License Agreement (CLA) before we can proceed with accepting your contribution. To sign successfully please make sure, that email in the commit is present in your github account.

Thank you

@ishakhsuvarov ishakhsuvarov added this to the March 2017 milestone Mar 16, 2017
@ishakhsuvarov ishakhsuvarov self-assigned this Mar 16, 2017
@pangteypiyush
Copy link

Thanks !!

@ishakhsuvarov
Copy link
Contributor

Hi @nazarpadalka

We will provide test for covering this case

Are you still working on it?

@nazarpadalka
Copy link
Contributor Author

@ishakhsuvarov
my coworker still working on this, we will make new PR for tests separately

@ishakhsuvarov
Copy link
Contributor

@nazarpadalka It is the best practice to deliver tests simultaneously.
Just let us know when you are done.

Thank you

…wnloadable product, after order was canceled.
@Winfle
Copy link
Contributor

Winfle commented Mar 21, 2017

@ishakhsuvarov Hello. Sorry for long response, currently I have a trip.
So, integration test is ready (Winfle@18f92cc), but unfortunately I forgot to ask Nazar to fix permissions to allow me to commit in his develop branch. Tomorrow we will sort out this and I guess, that we can process this PR. Cheers

@nazarpadalka
Copy link
Contributor Author

@ishakhsuvarov we already added tests to PR, please let us know if need some else.
Thanx

@Winfle
Copy link
Contributor

Winfle commented Mar 21, 2017

@ishakhsuvarov Want to confirm, that old branch version did not pass that test.
Also I have suggestion to refactor that observer to reach possibility to write unit-tests. But seems like Magento 2 has more prioritised issues.

/** @var \Magento\Sales\Model\Order $order */
$order = $this->prepareOrder();
$order = $this->orderRepository->save($order);
if ($orderItems = $order->getAllItems()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like assertions will be unreachable in case if order would not have items, therefore making the test green false-positively.

* @magentoDbIsolation enabled
* @return \Magento\Sales\Model\Order
*/
protected function prepareOrder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't you just use the order_with_downloadable_product fixture which is pretty similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ishakhsuvarov It has some difference between this and order_with_downloadable_product fixture. For some reason (probably its an isolation) my test was not able to retrieve order links with specific ID I setup.

@ishakhsuvarov
Copy link
Contributor

@KravetsAndriy Thank you very much!
Looks pretty good, however I have added a few comments, please check them.

@ishakhsuvarov
Copy link
Contributor

@KravetsAndriy Please look into the static test failure on Travis CI

@Winfle
Copy link
Contributor

Winfle commented Mar 21, 2017

@ishakhsuvarov will fix this in the evening. Thanks

@Winfle
Copy link
Contributor

Winfle commented Mar 21, 2017

  • Added fixture and rollback for it to represent order downloadable product which has available link and related product option.
  • Removed over-dependency from SetLinkStatusObserverTest class.
  • Fixed change request.

@ishakhsuvarov
Copy link
Contributor

@KravetsAndriy Thank you for the fixes.

@Winfle
Copy link
Contributor

Winfle commented Mar 22, 2017

@ishakhsuvarov , thank you too! This one took some extra time, since it was first contribute. Now we look for new issues to take and I think, that we will decrease time, spent on formal stuff like we did.

@magento-team magento-team merged commit 6e05a95 into magento:develop Mar 23, 2017
magento-team pushed a commit that referenced this pull request Mar 23, 2017
@ishakhsuvarov
Copy link
Contributor

@nazarpadalka @KravetsAndriy PR is merged into the develop branch.
Thank you for collaboration.

@Winfle
Copy link
Contributor

Winfle commented Mar 24, 2017

@ishakhsuvarov Thanks you too!

@nazarpadalka
Copy link
Contributor Author

@ishakhsuvarov Thanx you

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.

6 participants