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

6.1: Cleanup plone.app.discussion settings when package is missing #330

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

mauritsvanrees
Copy link
Member

If the site contains comments, we throw an error and stop the upgrade. The advice then is to add the plone.app.discussion package.

This is part of plone/Products.CMFPlone#3958

I tried offering to remove existing comments, when the user has explicitly set an environment variable. But this gets tricky. In the catalog you can search for Discussion Items, but when you do brain.getObject() this fails with an AttributeError because the path contains ++conversation++default and this is only defined when plone.app.discussion is available. We could get all brains, check if they have total_comments more than zero, get the object, and then del __annotations__["plone.app.discussion"]. But then we would need to unindex the actual comments as well, so it starts sounding hairy. Let someone implement this who actually needs it. That would be the case for a site where there were comments in 6.0, but in 6.1 they no longer want the comments. But that really is a corner case.

What I did to test this PR:

  • In Plone 5.2 on Python 3 create one default Plone Site and one Plone Site with comments enabled, and a few comments actually added.
  • Copy the database to the coredev 6.1 buildout.
  • Run the 6.1 buildout with bin/buildout -c plips/plip-padiscussion-addon.cfg. Do git pull first, I have updated the plip file.
  • Run bin/instance-cmfplone fg. This starts the 6.1 site with Products.CMFPlone (so without plone.app.discussion), but with plone.app.upgrade and plone.app.caching as extras, so you can test the upgrade.
  • The @@plone-upgrade of the first Plone Site should work fine and cleanup everything.
  • The upgrade of the second site, so the one with comments, will explicitly fail because we see comments and refuse to continue. Best here is to add plone.app.discussion to the packages. You see this advice in the browser:
ValueError: 3 Discussion Items (comments) were found in the site, but plone.app.discussion is missing.
This package is optional since Plone 6.1.
Please add plone.app.discussion to your Plone installation if you want to keep using them.

In fact, when upgrading from Plone 5.2 on a site that has comments, you already get into a traceback before you reach the new upgrade step:

2024-08-13 18:34:42,957 INFO    [plone.app.upgrade:403][waitress-3] Added image_scales column to catalog metadata schema.
2024-08-13 18:34:42,957 INFO    [plone.app.upgrade:368][waitress-3] Updating metadata.
2024-08-13 18:34:42,957 INFO    [ProgressHandler:73][waitress-3] Process started (10 objects to go)
2024-08-13 18:34:42,962 ERROR   [plone.app.upgrade:295][waitress-3] Upgrade aborted. Error:
Traceback (most recent call last):
  File "/Users/maurits/shared-eggs/cp311/Zope-5.10-py3.11.egg/OFS/Traversable.py", line 236, in unrestrictedTraverse
    next = namespaceLookup(
           ^^^^^^^^^^^^^^^^
  File "/Users/maurits/shared-eggs/cp311/zope.traversing-5.0-py3.11.egg/zope/traversing/namespace.py", line 162, in namespaceLookup
    raise LocationError(object, "++{}++{}".format(ns, name))
zope.location.interfaces.LocationError: (<Document at /Plone2/page>, '++conversation++default')

I also tried first migrating the 5.2 site to 6.0. Then the above traceback does not happen.

Also the PicklingError from this comment only happens when you migrate from 5.2 directly to 6.1. On a site that is first migrated to 6.0 it goes better.

…ailable.

If the site contains comments, we throw an error and stop the upgrade.
The advice then is to add the `plone.app.discussion` package.
@mister-roboto

This comment was marked as resolved.

@mauritsvanrees mauritsvanrees merged commit 97c1816 into master Sep 2, 2024
6 checks passed
@mauritsvanrees mauritsvanrees deleted the pa-discussion-core-addon branch September 2, 2024 14:35
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.

3 participants