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

Fixed faulty media gallery label/description management for mutistore projects #2481

Merged
merged 12 commits into from
Apr 19, 2024

Conversation

fballiano
Copy link
Contributor

We're talking about the "images tab" of the product management page:
Schermata 2022-08-23 alle 22 14 42

The complete problem explanation and testing scenario is explained in issue #685

It wasn't easy to fix, I worked on it quite some hours. Hope I didn't miss anything.

Fixed Issues

  1. Fixes Image label per store view does not work as expected #685

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog JavaScript Relates to js/* Template : admin Relates to admin template labels Aug 23, 2022
@ADDISON74 ADDISON74 self-assigned this Sep 7, 2022
@sreichel sreichel self-requested a review September 13, 2022 02:45
@fballiano
Copy link
Contributor Author

Could somebody test this? It's a proper bug we're having since the beginning of time.

@ADDISON74
Copy link
Collaborator

ADDISON74 commented Sep 26, 2022

There are a lot of check failures in the "Files changed' section (at the end). Could you please investigate if they are real warnings?

@fballiano
Copy link
Contributor Author

I'll check it out asap!

@fballiano
Copy link
Contributor Author

actually those notice are in the "Unchanged files with check annotations", those files weren't changed in this PR so I wouldn't do anything about those notices.

@fballiano
Copy link
Contributor Author

@azambon I'd love your comment on this

@azambon
Copy link
Contributor

azambon commented Oct 9, 2022

Sory for the massive delay, I completely forgot about this!

I did try to reproduce the same scenario that I described in the issue, but I don't think that this PR actually solves it.
As far as I could see, this PR adds the possibility of marking the whole "label" and "position" columns as "use default" in non-default store views.
Even ignoring the fact that someone may want to specify "use default" only for the labels and positions of some images, the bug that I reported is about both a situation where the labels must implement the "use default" behavior, but also about a case when the labels must be different (when I said to set the label "CCC" in the non-default store view). And in both cases I still see the wrong values in the frontend (yes, I disabled all caching, just to be sure).

So, unless I'm missing something, this does not seem to fix issue #685 . At least not yet.

As I see it, there are a couple of things left to solve:

  • fix the values that are saved for the image_label, small_image_label and thumbnail_label attributes for non-default store views. That should fix the frontend and all other places that use those attributes;
  • (optionally) move the "use default" checkboxes to be per-image for the "label" and "position" columns, instead of being global as they were implemented in this PR.

@fballiano
Copy link
Contributor Author

Hi @azambon and thanks for the test,

  • I'm pretty sure the values are saved correctly on the DB, if you switch storeview in the backend you should see the values you entered per every store view
  • I think I didn't check the frontend at all, so that could need another fix
  • I've to say I wouldn't put the checkbox "per image", that would be much harder, maybe it could be done in a second PR but to me at least it should work with "all images"

@fballiano
Copy link
Contributor Author

@azambon I tested the frotend and the image labels seem to be correct to me, both in default and the single store view, the labels changes and are the same I see in the backend. This is tested on RWD theme.

@azambon
Copy link
Contributor

azambon commented Oct 10, 2022

I performed the scenario that I described in the issue once more, taking some screenshots.

As you can see, when I uncheck the new "use default" checkbox that you added in order to set the label CCC for the non-default store view, the data is saved correctly in the catalog_product_entity_media_gallery_value table, but the attributes in catalog_product_entity_varchar are not updated correctly.
And, as you can see, in the frontend of the non-default store view I still see the wrong value.

This is with a fresh installation of OpenMage with the default theme, no customizations at all and all caches disabled.
If you use a frontend theme that does not use the image_label, small_image_label and thumbnail_label product attributes and uses the media gallery directly, then yes, the frontend would work fine too. But those attributes would still contain a wrong value, and anything that used them would get the wrong data.

Schermata da 2022-10-10 12-18-43
Schermata da 2022-10-10 12-18-49
Schermata da 2022-10-10 12-21-12

@fballiano
Copy link
Contributor Author

sorry for the stupid question, did you reindex/recache? cause I checked it yesterday and I see the right things in frontend, if I didn't understand something wrong :-\

@azambon
Copy link
Contributor

azambon commented Oct 10, 2022

All caches are disabled and I did reindex (even though reindexing should not affect the values in the catalog_product_entity_* tables, only in the catalog_product_flat_* tables, if flat tables are enabled).

@fballiano
Copy link
Contributor Author

I've flat disabled, just in case

@fballiano
Copy link
Contributor Author

I'll post a few screenshots to check that i'm not drunk :-D

backend for default:
Schermata 2022-10-10 alle 12 05 38

backend for non default
Schermata 2022-10-10 alle 12 05 48

frontend for default
Schermata 2022-10-10 alle 12 06 17

and frontend for the non default storeview
Schermata 2022-10-10 alle 12 06 23

@github-actions github-actions bot removed phpstan PHPStorm Mage.php Relates to app/Mage.php phpcs labels May 15, 2023
@empiricompany
Copy link
Contributor

i've tested this PR with this value from backend:
default
english
frech

eav_attribute
eav_attribute

catalog_product_entity_varchar (not updated)
catalog_product_entity_varchar

catalog_product_entity_media_gallery (value_id reference for catalog_product_entity_media_gallery_value )
catalog_product_entity_media_gallery

catalog_product_entity_media_gallery_value (correctly updated)
catalog_product_entity_media_gallery_value

catalog_product_flat_1 (not updated no index requested -default store)
catalog_product_flat_1

catalog_product_flat_2 (not updated no index requested - store english)
catalog_product_flat_2

catalog_product_flat_3 (not updated no index requested - store french)
catalog_product_flat_3

result in frontend title is always from admin value default, for exampla french view:
frontend_french

@empiricompany
Copy link
Contributor

empiricompany commented Sep 13, 2023

i've fixed the reported issues
please try this PR: fballiano#21

correct eav values table
catalog_product_entity_varchar
catalog_product_entity_varchar

correct flat per store tables
catalog_product_flat_2
catalog_product_flat_2

empiricompany and others added 2 commits September 13, 2023 18:12
* fix tpl media gallery

* fix save eav attribute labels

* fix use label default

* remove unecessary var

* clean

---------

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano
Copy link
Contributor Author

I've integrated @empiricompany latest patch to this PR and retested it, it seems to be working correctly to me.

* fix tpl media gallery

* fix save eav attribute labels

* fix use label default

* remove unecessary var

* clean

* fix use default checked

* fix use default

---------

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Copy link
Contributor

@kyrena kyrena left a comment

Choose a reason for hiding this comment

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

Tested, seems working.

From #685, this does not work: "In the admin add another image to the product with the label DDD in the default store view. Mark it as the default base image, small image and thumbnail and set its label to DDD" I must save again the product at store view level to have the right label.

Another thing, perhaps already the case, not sure, when I upload a new image, if I select it as default images, when I save the product, I must select again it as default images and save again, else the selection is lost.

@fballiano
Copy link
Contributor Author

true, this needs to be addressed:

  • when you upload a new image, the first time you save the product, the fact that you selected the image as base/small/thumb isn't not saved

@fballiano
Copy link
Contributor Author

I've fixed the last remaining point, it was only 1 line that was removed (and now reintroduced).

Since there were already 2 gray check I'll merge this, it's an important bugfix too long overdue (and a huge amount of work to develop it).

@fballiano fballiano merged commit bd7430d into OpenMage:main Apr 19, 2024
14 checks passed
@fballiano fballiano deleted the issue685 branch April 19, 2024 12:05
@ADDISON74 ADDISON74 removed their assignment Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog documentation environment JavaScript Relates to js/* Rebased: RFC-0002 This PR has been rebased onto either 'main' or 'next' according to RFC-0002 Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image label per store view does not work as expected
6 participants