search

plone / plone.app.discussion

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

Extended Moderation of comments #166

Closed ksuess closed 4 years ago

ksuess commented 4 years ago

Existing review workflow extended by two new workflow states "rejected" and "spam". Moderation view displays comments with filtering by workflow state. image

mister-roboto commented 4 years ago

@ksuess thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

ksuess commented 4 years ago

@jenkins-plone-org please run jobs

ksuess commented 4 years ago

collective/plone.app.locales#285 adds the necessary translations.

restapi and CMFPlone are testing workflows.

So tests below run with https://github.com/plone/plone.app.discussion/pull/166 https://github.com/collective/plone.app.locales/pull/285 https://github.com/plone/plone.restapi/pull/843 Plone 5.2: https://github.com/plone/Products.CMFPlone/pull/3032 Plone 5.1: https://github.com/plone/Products.CMFPlone/pull/3033 (https://github.com/plone/plone.app.upgrade/pull/224)

Plone 5.2:

https://github.com/plone/plone.app.discussion/pull/166
https://github.com/collective/plone.app.locales/pull/285
https://github.com/plone/plone.restapi/pull/843
https://github.com/plone/Products.CMFPlone/pull/3032
(https://github.com/plone/plone.app.upgrade/pull/224)

Plone 5.1:

https://github.com/plone/plone.app.discussion/pull/166
https://github.com/collective/plone.app.locales/pull/285
https://github.com/plone/plone.restapi/pull/843
https://github.com/plone/Products.CMFPlone/pull/3033
(https://github.com/plone/plone.app.upgrade/pull/224)
mauritsvanrees commented 4 years ago

Just very quickly looking at this. Don't we simply want to delete comments instead of marking them as spam or rejected? When you only mark them, they stay in the database forever.

Are these new states options that you can enable or disable in the control panel?

ksuess commented 4 years ago

That's the point: Not published comments stay and are still accessible. And the moderator can push them out of the default view of pending comments by marking them as rejected or spam. See filter on top of screenshot above.

Of course the moderator can still delete single comments like before and he also can batch delete multiple comments.

ksuess commented 4 years ago

There is still one test for Plone 5.1 failing:

    browser.getControl(name='form.widgets.IRichTextBehavior.text').value = "Lorem ipsum"
    LookupError: name 'form.widgets.IRichTextBehavior.text'

This is because plone.app.contenttypes is checked out with branch 1.4.x, but the renaming of the interface from IRichText to IRichTextBehavior happend in version 2.0.0 What about changing to master branch?

mauritsvanrees commented 4 years ago

Current Plone 5.1 is not using the master branch of plone.app.discussion. It is using a branch that sadly is called 3.x, but that should have been called 3.0.x. The last release on that branch was 3.0.8, which is what is in the versions.cfg. That may explain several test failures.

So on Plone 5.1 this would need a different PR. Or actually preferably no PR at all: it seems best to do this big new feature only on Plone 5.2.

ale-rt commented 4 years ago

It also seems to me something 5.2 specific (I did not test it anyway, will do now)

ale-rt commented 4 years ago

@ksuess @mauritsvanrees I think that with this https://github.com/plone/Products.CMFPlone/pull/3036 it will be easier merging this one.

mauritsvanrees commented 4 years ago

There was a bit of a discussion at the Alpine City Sprint on whether we should include new features like this in a bugfix release of 5.2. For stability it would be better if 5.2 only gets bug fixes, not new features. That is certainly true for Plone 5.1 (the failing Jenkins 5.1 job can be ignored: 5.1 uses another branch anyway, and should not have been tested with this).

But at the same time we are busy in 5.2 to update Zope from 4.1 to 4.2/4.3. That is more likely to have unintended side effects than this new feature in plone.app.discussion.

So while I like stability, I think for 5.2 we can allow this PR.

Anyone else want to comment, especially the @plone/framework-team ?

Looks like this needs to be merged with https://github.com/plone/plone.restapi/pull/843. Also with https://github.com/collective/plone.app.locales/pull/285, but I don't think this will cause test failures when it is merged a bit later.

jensens commented 4 years ago

From my perspective I am +1 for merging, unless we change our versioning policy - we had similar complex changes already in x.y.Z releases and this is a useful one. Plone major release versions do not follow Semver but are marketing versions. This was once decided at a PLOG (and I am not happy with this decision). So, with the rule in place we can add little features on Z and not feel bad.

mauritsvanrees commented 4 years ago

Okay, we can conclude it is approved for inclusion in Plone 5.2. @ksuess , when I have it correct, this means that we should do things in this order:

  1. [x] Merge https://github.com/plone/plone.restapi/pull/843 (@plone/restapi-team should do that). That is just a minor test fix that is fine even without the current plone.app.discussion PR. It must be merged first though.
  2. [x] Merge the current PR.
  3. [ ] Merge the translations: https://github.com/collective/plone.app.locales/pull/285

Then it is done, right?

ksuess commented 4 years ago

@ale-rt thank you for your effort on all versions "Improve tests for the workflow tool method listWFStatesByTitle". A hero point from me... @mauritsvanrees thank you for your special effort on caring about this multiple package thing. Your comment https://github.com/plone/plone.app.discussion/pull/166#issuecomment-598662157 does reduce the topic to two package PRs, hoohray. That's it. That would push comments moderation forward. Thank you all, @ale-rt , @mauritsvanrees and you, not mentioned but involved helping hands. Keep healthy!

ale-rt commented 4 years ago

Thanks to you for making Plone better!

mauritsvanrees commented 4 years ago

I have merged the PR. Congrats and again thanks! The plone.app.locales PR is open, but I have asked Vincent Fretin to have a look at it first, otherwise we will lose perfectly fine translation on Plone 5.1.

ksuess commented 4 years ago

https://github.com/plone/plone.app.discussion/pull/166#issuecomment-598662157 Translation seems to be OK for merging.

ksuess commented 4 years ago

And, yes, I'll come back to your comments @ale-rt