FACT-Finder-Web-Components / magento2-module

FACT-Finder® Web Components for Magento 2
https://web-components.fact-finder.de/
Other
11 stars 17 forks source link

no central "export" function anymore #134

Open fritzmg opened 5 years ago

fritzmg commented 5 years ago

Previously in the beta versions of the extension, the Omikron\Factfinder\Model\Export\Product class provided an exportProducts function which could simply be called from anywhere, if you want to manually trigger an export of the product feed.

In the current version no such method seems to exist. Instead, the cron and the backend controller have to use duplicated code, where the following happens:

  1. generate filename
  2. generate stream
  3. initialize stream with feed generator factory
  4. execute FTP upload using the stream
  5. push import command to FF server (this is missing from the cron, see https://github.com/FACT-Finder-Web-Components/magento2-module/issues/135)

To avoid code duplication, this should be transferred to some form of helper service instead.

fritzmg commented 5 years ago

May be something like this:

class ExportService
{
    protected $storeManager;
    protected $storeEmulation;
    protected $channelProvider;
    protected $csvFactory;
    protected $feedFactory;
    protected $ftpUploader;
    protected $pushImport;
    protected $feedType;

    public function __construct(
        \Magento\Store\Model\StoreManagerInterface $storeManager,
        \Omikron\Factfinder\Model\StoreEmulation $storeEmulation,
        \Omikron\Factfinder\Api\Config\ChannelProviderInterface $channelProvider,
        \Omikron\Factfinder\Model\Stream\CsvFactory $csvFactory,
        \Omikron\Factfinder\Model\Export\FeedFactory $feedFactory,
        \Omikron\Factfinder\Model\FtpUploader $ftpUploader,
        \Omikron\Factfinder\Model\Api\PushImport $pushImport,
        string $feedType
    ) {
        $this->storeManager = $storeManager;
        $this->storeEmulation = $storeEmulation;
        $this->channelProvider = $channelProvider;
        $this->csvFactory = $csvFactory;
        $this->feedFactory = $feedFactory;
        $this->ftpUploader = $ftpUploader;
        $this->pushImport = $pushImport;
        $this->feedType = $feedType;
    }

    public function exportAllEnabledFeeds(bool $pushImport = false): void
    {
        foreach ($this->storeManager->getStores() as $store) {
            if ($this->channelProvider->isChannelEnabled((int) $store->getId())) {
                $this->exportFeedForStore((int) $store->getId(), $pushImport);
            }
        }
    }

    public function exportFeedForStore(int $storeId, bool $pushImport = false): void
    {
        $this->storeEmulation->runInStore($storeId, function () use ($storeId, $pushImport) {
            $filename = "export.{$this->channelProvider->getChannel()}.csv";
            $stream   = $this->csvFactory->create(['filename' => "factfinder/{$filename}"]);
            $this->feedGeneratorFactory->create($this->feedType)->generate($stream);
            $this->ftpUploader->upload($filename, $stream);
            $this->pushImport->execute($storeId);
        });
    }
}
a-laurowski commented 5 years ago

Hi @fritzmg Thanks for Your observations Since v1.0.0 module introduced totally new architecture which is trying to follow best magento programming practices such as avoiding Helper classes (https://devdocs.magento.com/guides/v2.3/ext-best-practices/extension-coding/common-programming-bp.html#avoid-creating-helper-classes). In beta version most of the module logic was stored in helpers what gives in result a huge multiresponsible monolithic classes - very hard to maintain. In current version functionality is divided to set of dedicated, single responsible classes, which can be consider as reusable tools. Because of mentioned "single responsibility", we decided that neither uploading feed file, nor pushing the import are part of export functionality. Instead of this, they could be part of one, specific strategy of Data Integration which could use all of components required to, generate, upload and push import in FACT-Finder -similar to solution, You have proposed. We'll discuss adding such class

fritzmg commented 5 years ago

Hm, I understand, but it still seems odd to have duplicate code because of that, doesn't it? And it already produced an error: https://github.com/FACT-Finder-Web-Components/magento2-module/issues/135

fritzmg commented 5 years ago

May be the class name "…Helper" caused some confusion ;). I consider this class to be another service - that can be used by the cron, a controller or a command etc.