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

Ensure store-scoped values are selected before default #2966

Merged
merged 3 commits into from
May 12, 2023

Conversation

mattdavenport
Copy link
Contributor

We've encountered a bug in which a user created a category under a store scope with some values filled in (description, title, etc.). They then switched to the default scope, modifying other separate fields, and then switched back to the store scope to find the data they had inputted is gone. This occurred because the _getLoadAttributesSelect() query fetched the default values first upon going back to the store-scoped category page.

To reproduce:

  1. The category is created on a store scope with some values added/selected.
  2. Switch to default store scope view, adding value to category shop by attribute, saving the category.
  3. Switch back to the store scope and some values are gone (description, page title, etc etc.).

This PR ensures store_id is sorted ASC to ensure the store-scoped values are also fetched properly.

@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Jan 17, 2023
@elidrissidev
Copy link
Member

Rebased to bring the phpstan fix from last week.

If someone has got the time, please check if this is a regression in OM. Cause I tested briefly in a store running OM 19.4.14 and I couldn't reproduce it. I will check again later.

@elidrissidev
Copy link
Member

Just confirmed that this bug is not happening in OM 19.4.14, there must have been a change in the following versions that introduced it.

@addison74
Copy link
Contributor

@elidrissidev - Did you look in the history of the file starting with 1.9.14 to identify when the problem occurred? I looked quickly and didn't find anything.

@elidrissidev
Copy link
Member

@addison74 Indeed I did. With the help of git bisect, I was able to find that this issue started from #1198. I would prefer that the fix in this PR be updated, cause it seems like a workaround.

@elidrissidev
Copy link
Member

Hey @mattdavenport, are you interested in updating this PR with a proper fix now that we found the origin of this issue?

@mattdavenport
Copy link
Contributor Author

@elidrissidev I unfortunately won't have time for this in the near-term. If you would like to propose a fix or keep this PR open until I'm able to return to it that's fine as well. Thanks for looking into this btw!

@fballiano fballiano marked this pull request as draft February 21, 2023 17:07
@fballiano fballiano changed the base branch from 1.9.4.x to main May 6, 2023 22:01
@fballiano fballiano changed the base branch from main to 1.9.4.x May 6, 2023 22:02
@fballiano
Copy link
Contributor

@elidrissidev what do you think we should do with this PR? Also, I can't rebase it to main :-(

@elidrissidev
Copy link
Member

Could be closed and converted into an issue until there's a proper fix for it, already was on my list for some time just can't figure it out.

@fballiano
Copy link
Contributor

I think the "order by" is maybe not the cleanest solution ever but I don't see it as an ugly workaround, it's effective and quite easy and I'd probably have that instead of the bug :-)

@mattdavenport would you be able to rebase this PR to the "main" branch? sadly I can't do it for you.

@mattdavenport
Copy link
Contributor Author

@fballiano Agreed on the implementation. Better than nothing I suppose :). I should have some time to rebase this tomorrow so I will take a look. Thanks!

@mattdavenport mattdavenport force-pushed the fix/category-default-scope-bug branch from 5f4e86e to aac60fd Compare May 9, 2023 18:46
@mattdavenport
Copy link
Contributor Author

@fballiano Should be good now. Sorry I didn't notice this was easily doable via Github UI 😄.

@fballiano fballiano changed the base branch from 1.9.4.x to main May 9, 2023 18:49
@fballiano fballiano changed the base branch from main to 1.9.4.x May 9, 2023 18:50
@fballiano
Copy link
Contributor

@mattdavenport I still see it on 1.9.4.x, I see your force push but I still can't change the branch in the PR because it generated 5000+ modified files :-(

When a category is created under a store scope before the default scope,
fetching the attributes on a page load will load NULL values rendering
those fields blank.
@mattdavenport mattdavenport force-pushed the fix/category-default-scope-bug branch from aac60fd to 6f9cc29 Compare May 11, 2023 13:53
@mattdavenport mattdavenport changed the base branch from 1.9.4.x to main May 11, 2023 13:55
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api labels May 11, 2023
@github-actions github-actions bot removed Component: Bundle Relates to Mage_Bundle Mage.php Relates to app/Mage.php Component: CatalogSearch Relates to Mage_CatalogSearch Component: Core Relates to Mage_Core Component: Cms Relates to Mage_Cms phpunit Component: Api PageRelates to Mage_Api PHPStorm Component: Dataflow Relates to Mage_Dataflow Component: Directory Relates to Mage_Directory Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cm/RedisSession Relates to Cm_RedisSession Component: Authorizenet Relates to Mage_Authorizenet Component: Captcha Relates to Mage_Captcha phpstan Component: Contacts Relates to Mage_Contacts ddev Component: Adminhtml Relates to Mage_Adminhtml php-cs-fixer htaccess Component: CatalogRule Relates to Mage_CatalogRule phpcs Component: Customer Relates to Mage_Customer labels May 11, 2023
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested

@fballiano fballiano merged commit 4da40b1 into OpenMage:main May 12, 2023
@fballiano fballiano changed the title Ensure store-scoped values selected before default Ensure store-scoped values are selected before default May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants