numero2 / contao-storelocator

Contao Plugin for managing stores (or in common address data) and providing a frontend-search based on geo data.
https://www.numero2.de/contao/erweiterungen/storelocator.html
GNU Lesser General Public License v3.0
13 stars 8 forks source link

CSV import: extend functionality by update & remove actions #57

Closed felixschwan closed 4 months ago

felixschwan commented 4 months ago

Would be nice if the user can add another column to stores CSV like "published". Contao standard field "published" already exists: https://github.com/numero2/contao-storelocator/blob/master/src/Resources/contao/dca/tl_storelocator_stores.php#L229

But importer doesn't consider this field when importing: https://github.com/numero2/contao-storelocator/blob/master/src/Resources/contao/modules/ModuleStoreLocatorImporter.php#L128

New CSV-column could be 1 or 0 for example.

Then following scenarios should be considered:

Why? Our customer has thousands of stores world wide... it's a time consuming taks to manually click through storeLocator in Contao backend and update/remove stores. Furthermore it would lower the number of Googple-Maps-API requests (if store already exists and geo-data is the same --> no API call necessary).

bennyborn commented 4 months ago

Would be nice if the user can add another column to stores CSV like "published".

At the time we've built this extension we decided against adding the column published in the import since you always should check if the geo-coordinates are generates correctly.

and store exists --> update (or maybe check if data has changed and than update)

That would require that your CSV-file includes the ID of the StoreLocator stores. Otherwise a matching could be quite difficult. I mean how should we match these entries? By name? Or maybe by address? I see big potential for trouble here 🤔

I'd suggest that we add an event during the import for each individual row. This way it would be easy to add additional columns and maybe implement some kind of matching for updating already existing records.

felixschwan commented 4 months ago

Thanks for your relpy!

The idea with an extra event sounds pretty smart.

Additionally I would add an extra column like "store number" - an unique number for each store that is given by the user who fills the CSV (could be easily 1 to n) or even better: In our scenario our customer has a separate shop where users can register/unregister their store. By exporting all stores into CSV, the "store number" of each store should be saved and used as unique identifier.

storeId name email etc.
000001 store A info@store-a.com ...
000002 store B info@store-b.com ...

Now, no matter what data of store "000001" changes, we can check that against that unique ID.

What do you think about that approach?

bennyborn commented 4 months ago

Once we've added an event you could certainly use such a custom field to identify already imported stores to update them.

michb commented 4 months ago

We added this in 16479dc9581b20fc9ac8550151b7508c212930f5

felixschwan commented 3 months ago

Hi, I wanted to share my experience with the new EventListener - maybe helpful for others facing the same obstacles - maybe some stuff to add in the docs - let's go:

After placing the code from the docs into src/EventListener/StoreImportListener.php I thought I could make this run with a simple Contao cache clear & warmup. But fritzmg told me in the Contao slack community that composer install -o is the way to go. After that, I was able to verify the new Service with: vendor/bin/contao-console debug:container StoreImportListener.

So far so good. When I started debugging an CSV import I ran into several errors:

Internal Server Error App\EventListener\StoreImportListener::__invoke(): Argument #1 ($event) must be of type Contao\StoreLocatorBundle\Event\StoreLocatorEvents, numero2\StoreLocatorBundle\Event\StoreImportEvent given, called in /var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php on line 270

Ok, so I changed "Contao\" to "numero2\" in the use-statements. Next try:

Internal Server Error App\EventListener\StoreImportListener::__invoke(): Argument #1 ($event) must be of type numero2\StoreLocatorBundle\Event\StoreLocatorEvents, numero2\StoreLocatorBundle\Event\StoreImportEvent given, called in /var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php on line 270

I replaced the class in the invoke function and then it works as expected: `public function invoke( StoreImportEvent $event )`

I don't know if this is a particular problem with my setup or not. But I wanted to share all this with you - maybe it helps :)

PS: Full error log attached! prod-2024-06-18.log

michb commented 3 months ago

Just fixed the readme. Sorry for the typos. Could you try again with the template that is now given in the readme.

felixschwan commented 2 months ago

Hey, finally I had some time to work on this project. I've almost reached my goal but there is one problem:

When the importer processes an already existing store in the DB, it always runs the INSERT command in save() function instead of using the UPDATE.

Example Scenario to make it clearer - this is my StoreImportListener.php:

<?php
// src/EventListener/StoreImportListener.php
namespace App\EventListener;

use Contao\Model\Registry;
use Doctrine\DBAL\Connection;
use numero2\StoreLocator\StoresModel;
use numero2\StoreLocatorBundle\Event\StoreImportEvent;
use numero2\StoreLocatorBundle\Event\StoreLocatorEvents;
use Symfony\Component\EventDispatcher\Attribute\AsEventListener;

#[AsEventListener(StoreLocatorEvents::STORE_IMPORT)]
class StoreImportListener
{
    /**
     * @var Doctrine\DBAL\Connection
     */
    private Connection $connection;

    public function __construct(Connection $connection)
    {
        $this->connection = $connection;
    }

    public function __invoke(StoreImportEvent $event): void
    {
        $model = $event->getModel();
        $dbTable = $model->getTable();
        $importEntryId = $model->__get('store_id');
        $data = $model->row();
        $currentDataBaseRow = $this->connection
            ->prepare("SELECT * FROM $dbTable WHERE store_id=$importEntryId")
            ->executeQuery()
            ->fetchAssociative();

        // If store does not exist in DB --> return.
        if (!$currentDataBaseRow) return;

        // Check for diffs between import and DB.

        // Keys to compare = $data[keys] without tstamp, pid, store_id
        $keys_to_remove = ["tstamp", "pid", "store_id"];
        $commonKeys = array_diff_key($data, array_flip($keys_to_remove));

        // Only diff $commonkeys and $currentDataBaseRow
        $addressPartials = ["street", "postal", "city", "country"];
        $changes = false;
        $addressChanges = false;

        foreach ($commonKeys as $commonKey => $value) {
            // Compare values
            if ($currentDataBaseRow[$commonKey] != $data[$commonKey]) {
                $changes = true;

                // Check if value is address value
                if (array_key_exists($commonKey, $addressPartials)) {
                    $addressChanges = true;
                }
            }
        }

        // If changes: modify model - else: skip import
        if ($changes) {
            $model->__set('alias', $currentDataBaseRow['alias']);
            $model->__set('id', $currentDataBaseRow['id']);

            // if street,postal,city,country NOT changed --> use lat,long from DB
            if (!$addressChanges) {
                $model->__set('latitude', $currentDataBaseRow['latitude']);
                $model->__set('longitude', $currentDataBaseRow['longitude']);
            }

            $event->setModel($model);
        } else {
            $event->setSkipImport(true);
        }
    }
}

With the current workflow the Registry recognizes the second import as new Object (that's clear because it's a completely new process and the system doesn't know we made "first import" before). So the check if (Registry::getInstance()->isRegistered($this)) will return false but we want to have true to go into UPDATE.

Final question is: What does the event / the model needs to be ready for an UPDATE in the save() function?

michb commented 2 months ago

Hi, you are almost there. To make the Contao model aware that it has to update the entry and not insert it, you first have to search the entry with the model class.

...
        // If changes: modify model - else: skip import
        if ($changes) {

            // find the model in database for update und use it in this event
            $model = StoresModel::findOneById($currentDataBaseRow['id']);
            $event->setModel($model);

            $model->__set('alias', $currentDataBaseRow['alias']);
            // setting the id is not needed as it is already set in the found model
...
felixschwan commented 2 months ago

Perfect thanks! Gave you a star for that great extension + support :)

felixschwan commented 2 months ago

Ok one more thing ☕

How can I automate the import (e.g. via Contao Cron)?

Whats a nice way to pass a CSV file into the vendor/numero2/contao-storelocator/src/Controller/StoreLocatorImportController.php and "simulate" a real Contao request?

bennyborn commented 2 months ago

I don't see any viable solution here since the import function of our Controller is private. Best way would be to create your own custom Cron or Command and do the same things we do in our import 🤷‍♂️