search

plone / plone.app.discussion

General commenting system for Plone content
https://pypi.org/project/plone.app.discussion
16 stars 47 forks source link

Turn this into a core addon #211

Closed jensens closed 3 months ago

jensens commented 1 year ago

sync with CMFPlone, make installable and remove old files.

(to be) Merged on its own:

Need to be merged together

Jenkins paste to test together:

https://github.com/plone/plone.app.discussion/pull/211
https://github.com/plone/Products.CMFPlone/pull/3782
https://github.com/plone/plone.app.contenttypes/pull/665
https://github.com/plone/plone.app.dexterity/pull/371
https://github.com/plone/plone.app.upgrade/pull/330
mister-roboto commented 1 year ago

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

jensens commented 1 year ago

pre-commit.ci autofix

gforcada commented 1 year ago

@jensens I see that you added already a 6.1 trove classifier, that's what makes pre-commit fail.

@ericof @mauritsvanrees I requested the new classifier: https://github.com/pypa/trove-classifiers/issues/146

I was about to ask for the 7.0 as well, but hopefully it does not take too much time to process them anyway. 🍀

gforcada commented 1 year ago

@jensens great job! 😄 👍🏾

I have a few questions though, unfortunately we did not have enough time this week to talk about things in depth 😅 I was mostly 😵‍💫 all they long with too many conversations going on 😅

Anyway:

erral commented 1 year ago

This package's translations are already included in the plone domain, so we don't need to do anything. The pot file generation script will take the messages wherever they are.

ericof commented 1 year ago

One important point about Trove classifiers and CI: As soon as the classifier is accepted, w e need to bump the trove-classifiers package everywhere (or in meta)

gforcada commented 1 year ago

The classifier has been merged, but it does not appear, yet, on pypi, but hopefully is a matter of a few days/weeks 🍀

jensens commented 6 months ago

Note: This needs an uninstall as well.

jensens commented 6 months ago

pre-commit.ci autofix

jensens commented 6 months ago

Note: open test failures:

jensens commented 5 months ago

In [Products.CMFPlone.tests.test_robot.RobotTestCase.Scenario Allow comments for Link Type](https://jenkins.plone.org/job/pull-request-6.1-3.12/407/testReport/Products.CMFPlone.tests.test_robot/RobotTestCase/Scenario_Allow_comments_for_Link_Type/)

Text 'Discussion settings' did not appear in 7 seconds.

  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 634, in run
    self._callTestMethod(testMethod)
  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 589, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp312/robotsuite-2.3.2-py3.12.egg/robotsuite/__init__.py", line 528, in runTest
    assert last_status == 'PASS', last_message

Test need to be moved from Products.CMFPlone to plone.app.discussion.

jensens commented 5 months ago

Yay, Jenkins is green. This one is ready. Please review. Be aware, this needs 4 PRs merged together.

mauritsvanrees commented 5 months ago

I did minor changes on two of the PR branches, so I started the combined Jenkins jobs for them for good measure.

mauritsvanrees commented 5 months ago

Something to try though:

jensens commented 5 months ago
  • Create Plone 6.0 site with only CMFPlone. Do not activate discussions, do not add any comments.
  • Update to 6.1 with only CMFPlone, so no p.a.d. (use cored 6.1 probably, but change the included eggs.)
  • Do things go wrong? Do you get errors that ZODB cannot load stuff?

Good point, I'll try it.

  • For such cases: does it help to add p.a.d., run the new uninstall profile, and then remove it?

It should.

  • And perhaps some view/code for removing all existing discussion items could help. But if a 6.0 site really uses comments, and upgrades to 6.1, the best advice is to simply include p.a.d. in the eggs.

I do not think this is what we want. If someone used pad in 6.0 it must be installed in 6.1 as well to have a sane state. Removal of content on uninstall is usually not what we do for add-ons on uninstall too. This is harmful.

mauritsvanrees commented 5 months ago

I get this error when I try an upgrade from 5.2 to 6.1 without plone.app.discussion:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 187, in transaction_pubevents
  Module transaction._manager, line 254, in commit
  Module transaction._manager, line 133, in commit
  Module transaction._transaction, line 282, in commit
  Module transaction._transaction, line 273, in commit
  Module transaction._transaction, line 456, in _commitResources
  Module transaction._transaction, line 430, in _commitResources
  Module ZODB.Connection, line 497, in commit
  Module ZODB.Connection, line 544, in _commit
  Module ZODB.Connection, line 575, in _store_objects
  Module ZODB.serialize, line 434, in serialize
  Module ZODB.serialize, line 443, in _dump
_pickle.PicklingError: Can't pickle <class 'plone.app.discussion.interfaces.IDiscussionLayer'>: import of module 'plone.app.discussion.interfaces' failed
davisagli commented 5 months ago

@mauritsvanrees I would guess there is a reference to plone.app.discussion.interfaces.IDiscussionLayer in the ZCA site manager.

For such cases: does it help to add p.a.d., run the new uninstall profile, and then remove it?

Does this help?

mauritsvanrees commented 5 months ago

I guess the way to handle this, is to add a mock IDiscussion interface in plone.app.upgrade so you don't get these errors, and then write an upgrade step to remove the registry and anything else else plone.app.discussion added.

Note that I added https://github.com/plone/Products.CMFPlone/pull/3977 so that on the @@plone-upgrade page you would see that plone.app.discussion is missing, and then you can decide if you want to add it to your requirements, or go on with the upgrade.

mauritsvanrees commented 3 months ago

I have added an upgrade step in https://github.com/plone/plone.app.upgrade/pull/330. See testing hints there. I have merged main/master in the PR branches of the other packages. This seems ready now.

mauritsvanrees commented 3 months ago

Note for reviewers: several GitHub Actions tests fail in this PR. This is because in GitHub Actions we test against what is on https://dist.plone.org/release/6.1-dev/. And these tests will fail because we need several checkouts. That is what we do on Jenkins, so we can ignore the failing tests here. We could probably add some mxdev configuration in here to fix this. But that would only be temporary until there is a new Products.CMFPlone release.

jensens commented 3 months ago

This should work https://github.com/plone/buildout.coredev/blob/6.1/plips/plip-padiscussion-addon.cfg In past we had Jenkins jobs for the PLIP configurations, well, at least nowadays this can be used to be tested local.

gforcada commented 3 months ago

PLIP jobs in jenkins can be added, it's a matter of configuring them, they do not appear automatically though...

mauritsvanrees commented 3 months ago

I merged all PRs, except this one... Oops. :-) Doing so now.