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

Use defaults value for include_in_menu to avoid inconsistencies in add_update mode #66

Closed
amenk opened this issue Aug 18, 2020 · 17 comments

Comments

@amenk
Copy link

amenk commented Aug 18, 2020

Magento version: 2.3.4-p2
techdivision/import 16.5.2

We are importing categories via add_update mode.

Background

First we had the column include_in_menu = 1 and everything went fine. Then we decided that we do not want to overwrite that flag in already existing categories and removed the column completely, believing that it defaults to 1 (like when creating a category via admin panel). When we moved from staging to live, we did a full new import and bang: all the categories where not shown.
Worse than that: The category can also not be activated via admin panel any more.

Analysis

A category created via admin panel differs from the imported one in the way, that include_in_menu is completely missing.
Is is not 0, or 1 .. it is just not there.

For the imported category it shows "included in menu = yes" in the admin panel, but in the store front it is not shown in the menu. Also setting it to no and back to yes in the admin panel does not seem to help.

vergleich-test

Possible solution

Set all the values which are omitted as columns to the correct defaults (those Magento uses in admin panel) when creating new category entities.

@amenk
Copy link
Author

amenk commented Aug 20, 2020

@pathmissing Were you able to reproduce or do you need further info?

@pathmissing
Copy link
Member

Hi @amenk, I was able to reproduce this issue. Thank you for the detailed analysis!

@amenk
Copy link
Author

amenk commented Aug 26, 2020

@pathmissing Do you have an estimate for the fix?

@pathmissing
Copy link
Member

pathmissing commented Aug 26, 2020

Hi @amenk, currently I don't have an estimate for this issue because we classified it as an improvement and scheduled it for the next minor release. But if you are blocked by this we could definitely handle this as bug and up the priority. In that case I think we could fix this issue in the coming days.

@amenk
Copy link
Author

amenk commented Aug 26, 2020

yes, please, it will block us in a few days :)

@wagnert wagnert moved this from ToDo to In Progress in Pacemaker Community Aug 26, 2020
@pathmissing
Copy link
Member

Hi @amenk, @wagnert is working on this issue right now, a fix should be available in the coming days.
In the mean time you could configure a default value for this column by creating a configuration file under <magento-root-dir>/app/etc/configuration/default-values.json with the following content:

{
  "default-values": {
    "catalog_category": {
      "include_in_menu": 1
    }
  }
}

This will set a default value for the column include_in_menu if (and only if) this column is missing in your csv file. You can find more information about this feature in the documentation:
https://docs.m2if.com/38/getting-started/configuration/default-column-values

Hope this helps :)

@amenk
Copy link
Author

amenk commented Aug 26, 2020

Thanks for the update and the workaround.

If we set this default value, existing categories are not updated, right? That would be enough of a fix for us then. I did not know about this method.

So the final fix to this issue would just to put reasonable default-values which are the same like in admin I guess to make the import smoother :)

@pathmissing
Copy link
Member

When importing categories with a default value for include_in_menu, existing values will be updated to the configured value. Currently there is no alternative to this approach, but we were discussing potential solutions for this problem earlier today. Maybe we could talk about your specific use case tomorrow, if that's okay with you :)

@amenk
Copy link
Author

amenk commented Aug 27, 2020

Sure, we can talk :) For the reference I will elaborate our use case a bit:

The original problem is, that we do not know from the PIM where we get the import-file from, which categories need to be visible in the shop. So we developed a task to run after the import to toggle the include_in_menu flag for all categories, based on whether they have any products or not (empty categories hidden).
The task compares the required state with the current state and updates if necessary.

Now if we make an update-import we removed the include_in_menu column from the CSV because either specifying 1 or 0 would cause problems. If we specify one, all categories would be shown until the toggle task runs, if we use 0 all categories would be hidden. And also the toggle task would take much longer.

We believe we cannot detect during the import if we need to show or hide a category, because we want to support also partial imports in the future and would have to look at both, the database and the import file to determine menu visibility.

I believe that feature request makes sense, because the update part is already working well. Only the add part leads to some kind of data corruption because not the same values are applied as when you would create a category from the backend.
Corruption means, that setting include_in_menu programmatically or via admin panel is not working at all for new categories which were imported without include_in_menu specified.

@amenk
Copy link
Author

amenk commented Aug 28, 2020

thank you @wagnert & @pathmissing !

@wagnert
Copy link
Member

wagnert commented Aug 28, 2020

Hi @amenk This has been a significant change and I'm not sure if we can backport it to the Pacemaker Community 3.8 as there are man breaking changes. So would it be o. k. when I'll create a first 4.0.0-alpha1 for you and you proceed with the new 4.0.x branch versions.

@amenk
Copy link
Author

amenk commented Aug 28, 2020

Ok

@wagnert
Copy link
Member

wagnert commented Aug 28, 2020

All right :) This version actually is pretty still the same as the latest 3.8.x version but contains these changes besides some other minor changes. For example, we added an additional logger that logs log messages with log level > than info to the console, which is something the DEVs always required in our projects, e. g. to see if rows has been skipped.

wagnert added a commit to techdivision/import-cli-simple that referenced this issue Aug 28, 2020
* Fixed #PAC-206: Prevent finder mappings of different libraries to be overwritten
* Add second log handler to log to console also
* Adjust log messages to log only message with log level `notice` to console
* Remove stack trace of exception for missing media directories > log a simple debug message instead
@wagnert
Copy link
Member

wagnert commented Aug 28, 2020

@amenk, we've release 4.0.0-alpha1 now, please give it a try :-)

@wagnert wagnert moved this from In Progress to Pending Release in Pacemaker Community Aug 29, 2020
@amenk
Copy link
Author

amenk commented Sep 2, 2020

I have only specified include_in_menu in the default file and upgraded to 4.0.0-alpha1

{
    "default-values": {
	"catalog_category": {
	    "include_in_menu": null
	}
    }
}

And I am getting

 Uncaught TypeError: Argument 1 passed to TechDivision\Import\Category\Listeners\SortCategoryListener::getAttributeSetNameById() must be of the type integer, null given, called in /home/example/example/projects/shop.example.com/vendor/techdivision/import-category/src/Listeners/SortCategoryListener.php on line 377 and defined in /home/example/example/projects/shop.example.com/vendor/techdivision/import-category/src/Listeners/SortCategoryListener.php:396
Stack trace:
#0 /home/example/example/projects/shop.example.com/vendor/techdivision/import-category/src/Listeners/SortCategoryListener.php(377): TechDivision\Import\Category\Listeners\SortCategoryListener->getAttributeSetNameById(NULL)
#1 /home/example/example/projects/shop.example.com/vendor/techdivision/import-category/src/Listeners/SortCategoryListener.php(176): TechDivision\Import\Category\Listeners\SortCategoryListener->update('Root/Produkte/S...', Array)
#2 [internal function]: TechDivision\Import\Category\Listeners\SortCategoryListener->handle(Object(League\Event\Event), Object(TechDivision\Import\ 

might that be related / is something wrong with the defaults?

@amenk
Copy link
Author

amenk commented Sep 2, 2020

lets follow the latest post in a new issue

@wagnert
Copy link
Member

wagnert commented Sep 2, 2020

Hi @amenk, this is a issue we're already aware of, hope we can fix it within the next days :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Pacemaker Community
  
Pending Release
Development

No branches or pull requests

3 participants