Skip to content

Conversation

@jnptk
Copy link
Member

@jnptk jnptk commented May 27, 2025

@jnptk jnptk requested a review from davisagli May 27, 2025 13:39
@mister-roboto
Copy link

@jnptk 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!

<column value="head_title" />
<column value="hasPreviewImage" />
<column value="image_field" />
<column value="block_types" />
Copy link
Member

Choose a reason for hiding this comment

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

We need an upgrade step for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we modified an indexer here I guess we have to re-index the catalog, right? Is this what we would want:

<genericsetup:upgradeDepends
    title="Reindex block_types_indexer"
    profile="plone.volto:default"
    source="1019"
    destination="1020"
    import_steps="catalog"
  />

Copy link
Member

Choose a reason for hiding this comment

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

@jnptk yes, but...

We try to avoid upgrade steps which need to reindex all content, because it can take a long time in a large site. Actually for this reason, running the "catalog" import step updates the indexes and metadata but doesn't automatically reindex anything.

In this case we only have to reindex the items that provide IBlocks. So I think we should add a Python upgrade step which finds all blocks content (portal_catalog.unrestrictedSearchResults(object_provides=IBlocks.__identifier__)) and reindexes just the new index.

In case someone really wants to avoid it, they can run the upgrade steps in portal_setup where there is a checkbox to disable each step if needed.

We should mention this upgrade step in the release notes.

method="GET"
factory=".get.BlockTypesGet"
for="zope.interface.Interface"
permission="zope2.View"
Copy link
Member

Choose a reason for hiding this comment

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

We should use a permission that is not available to anonymous users.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davisagli do you have a particular permission in mind? I saw that there is a cmf.AddPortalMember permission here which is assigned to Site Administrator and Manager roles, I guess this is what we want, right?

Copy link
Member

Choose a reason for hiding this comment

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

@jnptk "Add portal member" doesn't make sense, that's for protecting the ability to add a new user. We can define a new permission, I would call it "Plone Site Setup: Blocks" for consistency with the other permissions for the control panels (https://github.com/plone/Products.CMFPlone/blob/master/src/Products/CMFPlone/controlpanel/permissions.zcml)

"count": brain.block_types[self.block_type],
}
)

Copy link
Member

Choose a reason for hiding this comment

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

To do: Add the code to get total counts for all blocks if type is None

Copy link
Member Author

Choose a reason for hiding this comment

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

@davisagli I've updated the code to return the block types + a total count when no specific block type is provided. e.g.:

{
  "slate": 22,
  "teaser": 1,
  "title": 3
}

also I changed that we don't return inside an "items" dict since we're very likely loading the items inside an "items" dict in the redux store, this way we can avoid having to access the items in the frontend like this: const items = useSelector((state) => state.blocktypes.items.items)

Copy link
Member Author

Choose a reason for hiding this comment

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

nvm. that was not a good idea, I'll handle that in the frontend via the action... 😅

Copy link
Member

Choose a reason for hiding this comment

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

Like we discussed this morning, I think it was a good idea after all :)

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.

4 participants