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

Fixed issue Unable to open URL for downloadable product #19996

Merged
merged 12 commits into from
Feb 15, 2019

Conversation

shikhamis11
Copy link
Member

@shikhamis11 shikhamis11 commented Dec 27, 2018

Fixed issue Unable to open URL for downloadable product #18944
Preconditions (*)

  1. Magento V2.2.6 Clean Install.
  2. PHP 7.0.28
  3. No special caching is present.

Steps to reproduce (*)

  1. Create downloadable product.
  2. Add a link to this product, for example https://google.com
  3. Save product.
  4. Order product in front-end.
  5. After order status complete navigate to your downloadable products in front-end.
  6. Click the button to open the URL set in step 2 to download your product.

Expected result (*)

A new page of https://google.com is opened and shown in browser.

Actual result (*)

Page is not opened correctly and error by Magento is thrown:
Something went wrong while getting the requested content.

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 @shikhamis11. 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

df2k2
df2k2 previously requested changes Dec 31, 2018
Copy link
Contributor

@df2k2 df2k2 left a comment

Choose a reason for hiding this comment

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

Suggestions to simplify complexity?

    public function getContentType()
    {
        $this->_getHandle();
        if ($this->_linkType === self::LINK_TYPE_FILE) {
            if (function_exists('mime_content_type')
                && ($contentType = mime_content_type(
                    $this->_workingDirectory->getAbsolutePath($this->_resourceFile)
                ))
            ) {
                return $contentType;
            }
            return $this->_downloadableFile->getFileType($this->_resourceFile);
        }
        if ($this->_linkType === self::LINK_TYPE_URL) {
            return (is_array($this->_handle->stat($this->_resourceFile)['type'])
                ? end($this->_handle->stat($this->_resourceFile)['type'])
                : $this->_handle->stat($this->_resourceFile)['type']);
        }
        return $this->_contentType;
    }

Note: if() constructions that return boolean values based on conditions, can be simplified to return the condition instead.
@df2k2 df2k2 dismissed their stale review December 31, 2018 09:57

Added suggested changes via commits

@magento-engcom-team
Copy link
Contributor

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

@sivaschenko
Copy link
Member

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, here is your new Magento instance.
Admin access: https://pr-19996.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@sivaschenko
Copy link
Member

Hi @shikhamis11 we found an issue on the PR code base, please take a look:

Manual testing scenario:

  1. Create a downloadable product with a URL link type and value i.e. https://google.com
  2. Create an order with the product and create an invoice for it
  3. On Frontend Go to My Account -> My Downloadable Products
  4. Click on the download link for the ordered product

"301 Moved" page is opened

@shikhamis11
Copy link
Member Author

hello @sivaschenko thanks for the update
It is fixed to show the actual result of the link set in the product earlier it was showing an error message.

@sivaschenko
Copy link
Member

Hi @shikhamis11 , as I can see it is showing an error ("301 Moved") currently, on this PR code base.

@sdzhepa
Copy link
Contributor

sdzhepa commented Jan 31, 2019

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @sdzhepa. Thank you for your request. I'm working on Magento instance for you

@sdzhepa
Copy link
Contributor

sdzhepa commented Jan 31, 2019

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @sdzhepa. Thank you for your request. I'm working on Magento instance for you

@sivaschenko
Copy link
Member

Hi @shikhamis11 during testing the PR we discovered that original issue cannot be reproduced on the latest 2.3-develop. Can you please recheck if the issue exists on the latest code base?

@shikhamis11
Copy link
Member Author

sure

@shikhamis11
Copy link
Member Author

@magento-engcom-team give me 2.3-develop instance

@magento-engcom-team
Copy link
Contributor

Hi @shikhamis11. Thank you for your request. I'm working on Magento 2.3-develop instance for you

@magento-engcom-team
Copy link
Contributor

Hi @shikhamis11, here is your Magento instance.
Admin access: https://i-19996-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@shikhamis11
Copy link
Member Author

shikhamis11 commented Feb 8, 2019

@sivaschenko no this is not working on current 2.3-develop instance . please check url
screenshot_2019-02-08_17-53-14
screenshot_2019-02-08_17-55-10

@sivaschenko
Copy link
Member

Sorry for confusion @shikhamis11 and thank you for the response! I rechecked and was able to reproduce it. Continuing to process the PR.

@ghost
Copy link

ghost commented Feb 15, 2019

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

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.

10 participants