added funtion get_allow_discussion_value

[Akshat2Jain]

Description

This pr aims to add the function

def get_allow_discussion_value(context, request):
    return getMultiAdapter((context, request), name="conversation_view").enabled()

in dxcontent.py and site.py

Why

Plone Site serializer and the Dexterity serializer return the same value for the "allow_discussion" field. Rather than duplicating the code, creating a separate function that can be called by both serializers to set the value consistently.

This pr related to the issue mentioned in volto project

fixes https://github.com/plone/volto/issues/4758

[mister-roboto]

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

[netlify[bot]]

✅ Deploy Preview for plone-restapi canceled.

Name	Link
🔨 Latest commit	24f25d7632ed23ffd0087ba810657250242ddccb
🔍 Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/64c8dc620f8eb2000889f2a7

[wesleybl]

@Akshat2Jain see if you can create a branch in the repository itself in plone.restapi. Github Actions tests were not run, because the PR was made from a fork

[Akshat2Jain]

@Akshat2Jain see if you can create a branch in the repository itself in plone.restapi. Github Actions tests were not run, because the PR was made from a fork

Hey @wesleybl, I don't have access rights to create a branch

[stevepiercy]

To allow tests to run on PRs from forks, we need to update the GitHub Workflows, similar to what we did in Volto: https://github.com/plone/volto/commit/5808b787230d4049632a4d5b14c3b0a80cc86e77

We restricted access across the Plone GitHub organization for members of Contributors. See https://community.plone.org/t/gsoc-students-please-focus-on-your-application-restrictions-on-developer-team-access-on-github/17041 for more information.

I created a new issue to deal with the above at https://github.com/plone/plone.restapi/issues/1656

@Akshat2Jain meanwhile would you please add a change log entry, per instructions in https://6.docs.plone.org/contributing/index.html#change-log-entry. Thank you!

[wesleybl]

It can be absent as a package or not installed via GenricSetup.

@jensens checking if the object has the allow_discussion attribute would be more performant than using is_product_installed? Or would a try/except be fine?

[wesleybl]

hmm i think checking if the result dictionary has allow_discussion key would be better.

[Akshat2Jain]

hmm i think checking if the result dictionary has allow_discussion key would be better.

I think this will be a more desirable solution approach because the code remains compatible with Plone 6.1 irrespective of whether the plone.app.discussion package is present or not.

Could you please advise on which approach you recommend and also how to do it? @wesleybl Thanks!

[wesleybl]

@Akshat2Jain I think it would be something like:

if "allow_discussion" in result:
    result["allow_discussion"] = get_allow_discussion_value(self.context, self.request)

You need to rebase with master before continuing.

[Akshat2Jain]

@Akshat2Jain I think it would be something like:

if "allow_discussion" in result:
    result["allow_discussion"] = get_allow_discussion_value(self.context, self.request)

Yeah! This is also correct but I have done something this.

def has_allow_discussion(result):
    return 'allow_discussion' in result

 if has_allow_discussion(result):
        result["allow_discussion"] = getMultiAdapter(
            (self.context, self.request), name="conversation_view"
        ).enabled()

But this will increase no of lines of code! @wesleybl

Thanks!

[jensens]

Some remarks:

1. getMultiAdapter((context, request), name="conversation_view").enabled() is an expensive call.
2. in the diff it looks like there is some indent wrong.
3. please DRY in code.

[wesleybl]

getMultiAdapter((context, request), name="conversation_view").enabled() is an expensive call.

Today it is already used. See:

https://github.com/plone/plone.restapi/blob/053a266be8dfc4a436913942c179d0db9ac4bc01/src/plone/restapi/serializer/dxcontent.py#L128-L130

[Akshat2Jain]

@wesleybl . I have made the changes. Can you review it.

[wesleybl]

@Akshat2Jain by having several commits, I suspect that you were making the commits through the github website. I recommend that next time, you make a clone of the repository and put several files in one commit. That way we won't have multiple commits.

[wesleybl]

Please make the code conditional, because soon, for Plone 6.1, plone.app.discussion becomes an optional core addon.

@jensens since plone.app.discussion will no longer be part of the core, I'm wondering if plone.restapi should be worrying about it. Could it be that instead of a condition, shouldn't we simply remove the allow_discussion handler? Shouldn't plone.app.discussion be the one to worry about?

Do we plan to have a plone.restapi 9 for Plone 6.1? If not, shouldn't it have, to handle situations like this, and to remove support for Plone 5.2?

[jensens]

Conditional support is fine. It is the same as currently with p.a.iterate, in near future with multilingual and there may add up other packages. Plan is to reduce the Plone core (the packages between plone.base and CMFPlone) and add them as core-addons to the space between CMFPlone and the Plone integration package. Distributions then can choose to have them or not.

[Akshat2Jain]

@wesleybl can you review it?

[Akshat2Jain]

@wesleybl . I have made the changes. Can you tell me where exactly black test are failing

[wesleybl]

@Akshat2Jain the github action output itself says what needs to be changed. to see:

https://github.com/plone/plone.restapi/actions/runs/5462642539/jobs/9942277773?pr=1655#step:5:20

But ideally, you have black installed locally, and format the code before committing. You can install it by running the make command inside the plone.restapi folder. After running make, you can run make black. This will automatically format the code for you. Some IDEs also have integration with black, to format the code when saving.

The make command will also install all of the package's dependencies. Then you can run the tests locally, with the command make test.

[stevepiercy]

But ideally, you have black installed locally, and format the code before committing. You can install it by running the make command inside the plone.restapi folder. After running make, you can run make black. This will automatically format the code for you. Some IDEs also have integration with black, to format the code when saving.

The make command will also install all of the package's dependencies. Then you can run the tests locally, with the command make test.

This should go into the documentation: https://6.docs.plone.org/plone.restapi/docs/source/contributing/index.html. It's not in the README. At the very least, an output of the make help command would be good.

[Akshat2Jain]

But ideally, you have black installed locally, and format the code before committing. You can install it by running the make command inside the plone.restapi folder. After running make, you can run make black. This will automatically format the code for you. Some IDEs also have integration with black, to format the code when saving.

The make command will also install all of the package's dependencies. Then you can run the tests locally, with the command make test.

I have run the make black and it formatted around 93 files. Should I commit all those files? @wesleybl

[wesleybl]

I have run the make black and it formatted around 93 files. Should I commit all those files? @wesleybl

It shouldn't format so many files. What version of black is he using? Please show the command output:

./bin/black --version

Can you please also inform the output of make black?

[Akshat2Jain]

It shouldn't format so many files. What version of black is he using? Please show the command output:

./bin/black --version

Can you please also inform the output of make black?

@wesleybl Version

black, 23.3.0 (compiled: yes)
Python (CPython) 3.10.6

output of make black

All done! ✨ 🍰 ✨
93 files formatted, 302 files left unchanged.

[wesleybl]

@Akshat2Jain the Makefile is installing the latest from black:

https://github.com/plone/plone.restapi/blob/815dbb6ecc0ed87af0dac1c8aa1d7679e2bdfd30/Makefile#L47

Meanwhile, CI is installing version of versions.cfg:

https://github.com/plone/plone.restapi/blob/9097cfcb55a7b2a045a5c5e66c46cd04300798c2/.github/workflows/black.yml#L25

Would you like to do another PR, to install the version of black and click that is in versions.cfg in Makefile?

You could also upgrade to the latest versions of click and black in versions.cfg

You need to update your fork's master branch with the original repository's master branch first.

[Akshat2Jain]

Tests are failing. At the end of the CI tracebak it indicates the error. For example:

https://github.com/plone/plone.restapi/actions/runs/5572932135/jobs/10179541947?pr=1655#step:10:789

You can run the tests locally with make test

while running make test I am getting this error

bin/test
/bin/bash: line 1: bin/test: No such file or directory
make: *** [Makefile:76: test] Error 127

@wesleybl

[stevepiercy]

@Akshat2Jain did you issue the command from the root of the Volto repository?

[Akshat2Jain]

@Akshat2Jain did you issue the command from the root of the Volto repository?

did you mean from the root of plone.restapi

[stevepiercy]

Oops, yes. Sorry, I am multitasking.

[stevepiercy]

Actually, I tried make test, and got the same result as you.

So I issued just make, and it is installing the package and its dependencies. When it is complete, I will run make test again, and report back the results.

[stevepiercy]

make test runs the tests for me. Perhaps try again with make clean, then make, and finally make test?

[Akshat2Jain]

make test runs the tests for me. Perhaps try again with make clean, then make, and finally make test?

Yup, Now it is running but I don't know why, I did these steps 2-3 times and at that time it was not running for me.

[wesleybl]

@jensens @sneridagh a test was made to try to verify that plone.app.discussion is installed:

https://github.com/plone/plone.restapi/blob/d819614d6826cbfbff1cf71f5c1e4e2dc9d06f07/src/plone/restapi/serializer/dxcontent.py#L44

However, when plone.app.discussion is installed and enabled globally, but a content doesn't have the behavior, allow_discussion is not added to the json. This test failed:

https://github.com/plone/plone.restapi/blob/04e69827e9f8f935b0091a50096f780d2363b13c/src/plone/restapi/tests/test_dxcontent_serializer.py#L579

See:

https://github.com/plone/plone.restapi/actions/runs/5586686877/jobs/10211032234?pr=1655#step:10:33

Any other test to see if plone.app.discussion is installed would be more costly, and would have to be done on every request.

So I ask, is it ok if we don't have the allow_discussion key in this situation?

[wesleybl]

@jensens @sneridagh any opinions here?

[wesleybl]

@davisagli any opinions here? See comment on @jensens' review and https://github.com/plone/plone.restapi/pull/1655#issuecomment-1640150326.

[Akshat2Jain]

@davisagli @jensens any suggestions? we are close to completing this

[wesleybl]

@jensens @sneridagh @davisagli as plone.app.discussion will soon be optional, any client should be prepared for whether or not they have the allow_discussion key in the response. So I think the failing test should be removed.

@Akshat2Jain please remove the test.

[Akshat2Jain]

I mistakenly committed the changes without updating the branch, should I create a new branch within the repo?

[Akshat2Jain]

@wesleybl , Can You help me with this?

[wesleybl]

@Akshat2Jain you can reset this commit:

https://github.com/plone/plone.restapi/commit/b7c9acf0ff49c51997935e7fbc31a0e65c96bc25

But if you think it's better, you can create a new branch and a new PR.