Fix issue #12866: Can't Disable Advanced Reporting

[nfourteen]

Description

Disable Magento_Analytics (aka Advanced Reporting) by default and provide clearer message if an industry is not provided.

Fixed Issues (if relevant)

1. magento/magento2#12866: Can't Disable Advanced Reporting

Manual testing scenarios

1. Install new Magento 2.2 instance and Stores > Configuration > General > Advanced Reporting > Advanced Reporting Service should be set to Disable after install
2. Go to Stores > Configuration > General > Advanced Reporting and change Advanced Reporting Service to Enable/Disable and press Save Config. If an Industry is not provided, then the Please select an industry. error message is shown rather than Please select a vertical.

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-cicd2]

All committers have signed the CLA.

[ishakhsuvarov]

I would suggest changing logic the validation, to better support the case with disabling validation, instead of disabling the feature on every new install. It was intended to be that way.

[nfourteen]

@ishakhsuvarov I can do that, but then what industry should be set as a default? Otherwise, on fresh installs the reporting service will be enabled, but the Industry will be empty and "will cause distortion of the analytics reports", thus breaking the validation logic of Magento\Analytics\Model\Config\Backend\Vertical::beforeSave(). If the module is enabled by default and someone signs up for the service, will the Magento BI alert them that they need to select an Industry?

If that's handled, I can update the pull request so the module is enabled by default and update the disabling validation logic with:

// Magento\Analytics\Model\Config\Backend\Vertical
public function beforeSave()
{
    if (empty($this->getValue()) && ($this->getFieldsetDataValue('enabled') === '1')) {
        throw new LocalizedException(__('Please select an industry.'));
    }

    return $this;
}

which will allow disabling of the module, but make sure an Industry is supplied when trying to save the config with the module enabled.

[ishakhsuvarov]

@nfourteen Currently waiting for someone from product organization to describe the desired state of the feature.
Thank you for collaboration

[nfourteen]

@ishakhsuvarov Thanks for the update. Let me know when you hear something as I'd be happy to edit this PR to reflect the desired state.

[slavvka]

@nfourteen @ishakhsuvarov As far as I know Advanced Reporting should be on for 2.2 and 2.3 by default. But I am waiting for the exact response from PM

[slavvka]

@nfourteen According to the info from our PM, it should be on by default. So I am closing this PR

[nfourteen]

@slavvka Ok, then there is another issue in that Advanced Reporting can't be disabled when it is on by default. See the related issue here: #12866. I think this PR should be reopened and the code changed to address the issue using one of the proposed solutions: see Reason and Proposed Solutions within the issue report.

[slavvka]

@nfourteen I agree. Please provide the changed code

[slavvka]

@nfourteen since the code you provided fixes the issue in a wrong way I am closing this PR. Please feel free to create a new one with a correct solution. Thank you!

[nfourteen]

@slavvka This is a Magento core problem and an issue that needs to be fixed. However, since it's a Magento problem, I don't know how the PM wants to proceed. So, I'd be happy to fix Magento core code, but I need some direction as to which way Magento would like to proceed. Please stop closing this PR because it is a issue that needs to be resolved.

Please have the PM review issue #12866 and advise which proposed solution would be acceptable or if another solution is desired.

[okorshenko]

Hi @nfourteen
Thank you for your contribution. We are closing this PR since the proposed solution cannot be accepted in the current state. We are working with PO right now to guide you for a proper solution.

Thank you for your patience

[slavvka]

@nfourteen Thank you for your attitude but this issue has been already fixed in this PR #15366