plone / plone.staticresources

Static resources for Plone
https://pypi.org/project/plone.staticresources/
4 stars 12 forks source link

folder_contents icons missing in classic UI after migration Products.CMFCore#3838 #337

Open szakitibi opened 1 month ago

szakitibi commented 1 month ago

Fixes plone/Products.CMFPlone#3838, which is still an issue for Plone 5.2 sites migrated to Classic Plone 6.0.11.

What happens:

The two new icons plone-rearrange and plone-selection are not imported for existing Plone 5 sites migrated to Plone 6. As a result, the folder_contents view produces [Products.CMFPlone.browser.icons:103][waitress-2] Icon resolver lookup of 'plone-selection' failed, fallback to Plone icon. messages and fails to lookup the icons.

What should happen:

The icons are imported and the folder_contents view does not produce the icon lookup errors.

Steps to reproduce:

image

image


!Important The problem only exists if the site is upgraded from Plone 5!

The plone.staticresources lacks an upgrade step to import the changes for the contents of plone/staticresources/profiles/default/registry/icons_plone.xml. It has been imported originally with upgrade step 203, but since Apr 22, 2021 there has been changes in the icons_plone.xml file, e.g. on Mar 23, 2022 - see commit https://github.com/plone/plone.staticresources/commit/05ab4fdc1df2aafc10fbc85ea3d5c38c3d67cf48.

This PR's is created to import the missing icons.

mister-roboto commented 1 month ago

@szakitibi thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

szakitibi commented 1 month ago

Should we include a purge=False in anticipation of newly created Plone 6 sites which already have this record?

yurj commented 1 month ago

Should we include a purge=False in anticipation of newly created Plone 6 sites which already have this record?

Yes.

https://5.docs.plone.org/develop/addons/components/genericsetup.html#the-purge-attribute

davisagli commented 1 month ago

purge=False doesn't have any effect for a single value like this. It is used for sequence values (e.g. a list) where existing items in the should not be purged.

mister-roboto commented 1 month ago

Tibor Szakmany your emails are not known to GitHub and thus it is impossible to know if you have signed the Plone Contributor Agreement, which is required to merge this pull request.

Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement How to add more emails to your GitHub account: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/adding-an-email-address-to-your-github-account

szakitibi commented 1 month ago

purge=False doesn't have any effect for a single value like this. It is used for sequence values (e.g. a list) where existing items in the should not be purged.

There is not a lot of documentation on this topic, I've just guessed, but indeed it does not work. Since GS is not capable of this, I've refactored the upgrade to use a python handler instead. The handler will apply the changes only if the records are missing.