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

Add registry setting for int fields #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

espenmn
Copy link
Member

@espenmn espenmn commented Aug 7, 2023

Added int field used in vocabulary to registry settings.
Not sure how it is best to do the upgrade profile.
I usually use another 'folder' in profiles, but looks like you do it differently.

@espenmn espenmn requested a review from petschki August 7, 2023 10:06
<value_type type="plone.registry.field.TextLine" />
</field>
<value>
<element>intfield</element>
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a project individual value. The default in the package is empty, so I would initialize it empty here too.

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<metadata>
<version>13</version>
Copy link
Member

Choose a reason for hiding this comment

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

Update profiles do not need a metadata.xml file.

<value_type type="plone.registry.field.TextLine" />
</field>
<value>
<element>intfield</element>
Copy link
Member

Choose a reason for hiding this comment

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

Same as above -> empty initialization per default

@@ -98,3 +98,7 @@ def upgrade_to_plone6(context):
del registry.records[key]

reapply_profile(context)

def upgrade_registry_13(context):
Copy link
Member

Choose a reason for hiding this comment

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

If you only want to reload an existing import_step I'd not do this in python code but in zcml see my change suggestion below.

Comment on lines +103 to 111
<genericsetup:upgradeStep
title="collective.collectionfilter: Upgrade v12 to v13"
description="upgrade to version 13"
source="12"
destination="13"
profile="collective.collectionfilter:upgrade-13"
handler=".upgrades.upgrade_registry_13"
/>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<genericsetup:upgradeStep
title="collective.collectionfilter: Upgrade v12 to v13"
description="upgrade to version 13"
source="12"
destination="13"
profile="collective.collectionfilter:upgrade-13"
handler=".upgrades.upgrade_registry_13"
/>
<genericsetup:upgradeDepends
title="collective.collectionfilter: Upgrade v12 to v13"
description="upgrade to version 13"
source="12"
destination="13"
profile="collective.collectionfilter:default"
import_profile="collective.collectionfilter:upgrade-13"
/>

And the registration of the upgrade profile collective.collectionfilter:upgrade-13 is missing in configure.zcml

INTEGER_IDXS = plone.api.portal.get_registry_record(
"collective.collectionfilter.int_types", default=[]
)
except ComponentLookupError:
Copy link
Member

Choose a reason for hiding this comment

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

I thought registry lookup errors are handled in plone.api ... and please follow the suggestion of the docs:

Always import the top-level package (from plone import api) and then use the module namespace to access the method you want (portal = api.portal.get()).

See https://6.docs.plone.org/plone.api/about.html#design-decisions

@petschki
Copy link
Member

petschki commented Aug 7, 2023

Additionally I suggest to implement the other globals also as registry configurable values. Best implementation would be to add an ICollectionFilterSettings schema interface with the configurable fields and load this interface in registry.xml ... it then would be very simple to add a controlpanel for siteadmins to change the values TTW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants