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

no central "export" function anymore #134

Open
fritzmg opened this issue Jul 8, 2019 · 4 comments
Open

no central "export" function anymore #134

fritzmg opened this issue Jul 8, 2019 · 4 comments

Comments

@fritzmg
Copy link
Contributor

fritzmg commented Jul 8, 2019

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 cron missing import push? #135)

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

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 8, 2019

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
Copy link
Contributor

a-laurowski commented Jul 8, 2019

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
Copy link
Contributor Author

fritzmg commented Jul 8, 2019

Hm, I understand, but it still seems odd to have duplicate code because of that, doesn't it? And it already produced an error: #135

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 8, 2019

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.

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

No branches or pull requests

2 participants