added funtion get_allow_discussion_value #1655

Akshat2Jain · 2023-06-13T17:35:44Z

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 plone/volto#4758

mister-roboto · 2023-06-13T17:35:49Z

@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 · 2023-06-13T17:35:50Z

✅ Deploy Preview for plone-restapi canceled.

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

wesleybl · 2023-06-13T19:49:50Z

@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 · 2023-06-14T06:17:43Z

@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 · 2023-06-14T08:02:17Z

To allow tests to run on PRs from forks, we need to update the GitHub Workflows, similar to what we did in Volto: plone/volto@5808b78

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 #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!

jensens

Please make the code conditional, because soon, for Plone 6.1, plone.app.discussion becomes an optional core addon. It can be absent as a package or not installed via GenricSetup.

wesleybl · 2023-06-16T20:40:52Z

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 · 2023-06-16T20:44:54Z

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

Akshat2Jain · 2023-06-17T10:18:32Z

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 · 2023-06-19T15:56:58Z

@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 · 2023-06-19T17:07:24Z

@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 · 2023-06-22T13:24:19Z

Some remarks:

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

wesleybl · 2023-06-22T18:31:54Z

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

Today it is already used. See:

plone.restapi/src/plone/restapi/serializer/dxcontent.py

Lines 128 to 130 in 053a266

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

Akshat2Jain · 2023-06-23T11:34:47Z

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

wesleybl · 2023-06-23T13:01:17Z

@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 · 2023-06-23T13:07:48Z

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?

wesleybl

I left some comments. See also that Black and flake8 are failing:

https://github.com/plone/plone.restapi/actions/runs/5355874837/jobs/9714682689?pr=1655

https://github.com/plone/plone.restapi/actions/runs/5355874859/jobs/9714682668?pr=1655

src/plone/restapi/serializer/dxcontent.py

src/plone/restapi/serializer/site.py

jensens · 2023-06-26T15:26:16Z

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 · 2023-07-04T05:50:17Z

@wesleybl can you review it?

wesleybl

Pease make a clone of the repository and put several files in one commit. That way we won't have multiple commits.

Checks at Some checks were not successful:

#1655 (comment)

must pass.

src/plone/restapi/serializer/dxcontent.py

src/plone/restapi/serializer/site.py

Akshat2Jain · 2023-07-05T09:30:22Z

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

wesleybl · 2023-07-05T12:41:14Z

@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.

wesleybl · 2023-07-05T12:54:19Z

news/4758.bugfix

@@ -0,0 +1,6 @@
+add the function


Use the PR number in the file name.

wesleybl · 2023-07-05T12:55:08Z

news/4758.bugfix

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


It is not necessary to have code in Changes.

src/plone/restapi/serializer/dxcontent.py

wesleybl · 2023-07-05T12:59:24Z

src/plone/restapi/serializer/dxcontent.py

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


The test is being done inside the function. So we shouldn't do the test here. Assignment is also being done in the function.

Suggested change

if "allow_discussion" in result:

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

get_allow_discussion_value(self.context, self.request, result)

wesleybl · 2023-07-05T13:00:29Z

src/plone/restapi/serializer/site.py

@@ -121,6 +122,8 @@ def __call__(self, version=None):
            for brain in batch
        ]

+        result["allow_discussion"] = get_allow_discussion_value(self.context, self.request, result)


The assignment is being done inside the function. So we shouldn't do assignment here.

Suggested change

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

get_allow_discussion_value(self.context, self.request, result)

stevepiercy · 2023-07-05T16:35:58Z

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 · 2023-07-06T09:33:52Z

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 · 2023-07-06T12:07:41Z

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 · 2023-07-06T12:51:32Z

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 · 2023-07-06T13:11:01Z

@Akshat2Jain the Makefile is installing the latest from black:

plone.restapi/Makefile

Line 47 in 815dbb6

bin/pip install black || true

Meanwhile, CI is installing version of versions.cfg:

plone.restapi/.github/workflows/black.yml

Line 25 in 9097cfc

    
           run: pip install click==$(awk '/^click =/{print $NF}' versions.cfg) black==$(awk '/^black =/{print $NF}' versions.cfg)

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.

wesleybl

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

Akshat2Jain · 2023-07-18T06:05:53Z

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 · 2023-07-18T09:22:49Z

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

Akshat2Jain · 2023-07-18T09:36:49Z

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

did you mean from the root of plone.restapi

stevepiercy · 2023-07-18T09:37:35Z

Oops, yes. Sorry, I am multitasking.

stevepiercy · 2023-07-18T09:40:09Z

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 · 2023-07-18T09:59:30Z

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

Akshat2Jain · 2023-07-18T10:15:20Z

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.

stevepiercy

I reviewed only the news item. It still needs review by a maintainer.

stevepiercy · 2023-07-18T11:35:26Z

news/1665.bugfix

@@ -0,0 +1 @@
+added the function get_allow_discussion_value(context, request) returns True if discussion is allowed and False if not, based on the enabled status of the "conversation_view" adapter.@Akshat2Jain


Suggested change

added the function get_allow_discussion_value(context, request) returns True if discussion is allowed and False if not, based on the enabled status of the "conversation_view" adapter.@Akshat2Jain

Added the function `get_allow_discussion_value(context, request)`. It returns `True` if discussion is allowed, or `False` if not, based on the enabled status of the `conversation_view` adapter. @Akshat2Jain

wesleybl · 2023-07-18T12:47:31Z

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

plone.restapi/src/plone/restapi/serializer/dxcontent.py

Line 44 in d819614

if "allow_discussion" in result:

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:

plone.restapi/src/plone/restapi/tests/test_dxcontent_serializer.py

Line 579 in 04e6982

    
           def test_allow_discussion_global_enabled_but_instance_has_no_discussion_behavior(

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 · 2023-07-21T15:05:38Z

@jensens @sneridagh any opinions here?

wesleybl · 2023-07-25T12:37:08Z

@davisagli any opinions here? See comment on @jensens' review and #1655 (comment).

Akshat2Jain · 2023-07-30T11:38:57Z

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

wesleybl · 2023-07-31T18:23:15Z

@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 · 2023-08-01T10:29:59Z

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

Akshat2Jain · 2023-08-01T12:35:26Z

@wesleybl , Can You help me with this?

wesleybl · 2023-08-01T14:04:42Z

@Akshat2Jain you can reset this commit:

b7c9acf

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

stevepiercy mentioned this pull request Jun 14, 2023

Allow PRs from forks to run tests? #1656

Closed

jensens requested changes Jun 16, 2023

View reviewed changes

wesleybl requested changes Jun 23, 2023

View reviewed changes

wesleybl requested changes Jul 4, 2023

View reviewed changes

wesleybl requested changes Jul 5, 2023

View reviewed changes

stevepiercy mentioned this pull request Jul 5, 2023

Document steps to contribute that are specific to plone.restapi #1664

Closed

This was referenced Jul 6, 2023

Update makefile #1665

Closed

updated makefile #1666

Closed

updates the Makefile to ensure consistent installations of black and click. #1668

Closed

Akshat2Jain requested a review from wesleybl July 15, 2023 11:19

wesleybl requested changes Jul 17, 2023

View reviewed changes

Akshat2Jain force-pushed the Akshat2Jain-patch-1 branch from b4c1888 to 18f7340 Compare July 18, 2023 11:02

Akshat2Jain requested a review from wesleybl July 18, 2023 11:27

stevepiercy requested changes Jul 18, 2023

View reviewed changes

Akshat2Jain force-pushed the Akshat2Jain-patch-1 branch from d819614 to b7c9acf Compare July 18, 2023 15:06

wesleybl requested review from jensens and sneridagh July 21, 2023 15:06

wesleybl requested a review from davisagli July 25, 2023 12:37

Akshat2Jain closed this Aug 1, 2023

Akshat2Jain force-pushed the Akshat2Jain-patch-1 branch from 78f7e3a to 24f25d7 Compare August 1, 2023 10:20

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

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

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

		@@ -0,0 +1 @@
		added the function get_allow_discussion_value(context, request) returns True if discussion is allowed and False if not, based on the enabled status of the "conversation_view" adapter.@Akshat2Jain

	added the function get_allow_discussion_value(context, request) returns True if discussion is allowed and False if not, based on the enabled status of the "conversation_view" adapter.@Akshat2Jain
	Added the function `get_allow_discussion_value(context, request)`. It returns `True` if discussion is allowed, or `False` if not, based on the enabled status of the `conversation_view` adapter. @Akshat2Jain

added funtion get_allow_discussion_value #1655

added funtion get_allow_discussion_value #1655

Conversation

Akshat2Jain commented Jun 13, 2023 • edited Loading

Description

Why

This pr related to the issue mentioned in volto project

mister-roboto commented Jun 13, 2023

netlify bot commented Jun 13, 2023 • edited Loading

✅ Deploy Preview for plone-restapi canceled.

wesleybl commented Jun 13, 2023

Akshat2Jain commented Jun 14, 2023

stevepiercy commented Jun 14, 2023

jensens left a comment

Choose a reason for hiding this comment

wesleybl commented Jun 16, 2023 • edited Loading

wesleybl commented Jun 16, 2023

Akshat2Jain commented Jun 17, 2023

wesleybl commented Jun 19, 2023

Akshat2Jain commented Jun 19, 2023

jensens commented Jun 22, 2023

wesleybl commented Jun 22, 2023

Akshat2Jain commented Jun 23, 2023

wesleybl commented Jun 23, 2023

wesleybl commented Jun 23, 2023

wesleybl left a comment

Choose a reason for hiding this comment

jensens commented Jun 26, 2023

Akshat2Jain commented Jul 4, 2023

wesleybl left a comment

Choose a reason for hiding this comment

Akshat2Jain commented Jul 5, 2023

wesleybl commented Jul 5, 2023

wesleybl Jul 5, 2023

Choose a reason for hiding this comment

wesleybl Jul 5, 2023

Choose a reason for hiding this comment

wesleybl Jul 5, 2023

Choose a reason for hiding this comment

wesleybl Jul 5, 2023

Choose a reason for hiding this comment

stevepiercy commented Jul 5, 2023

Akshat2Jain commented Jul 6, 2023

wesleybl commented Jul 6, 2023

Akshat2Jain commented Jul 6, 2023

wesleybl commented Jul 6, 2023 • edited Loading

wesleybl left a comment • edited Loading

Choose a reason for hiding this comment

Akshat2Jain commented Jul 18, 2023

stevepiercy commented Jul 18, 2023

Akshat2Jain commented Jul 18, 2023

stevepiercy commented Jul 18, 2023

stevepiercy commented Jul 18, 2023

stevepiercy commented Jul 18, 2023

Akshat2Jain commented Jul 18, 2023 • edited Loading

stevepiercy left a comment

Choose a reason for hiding this comment

stevepiercy Jul 18, 2023

Choose a reason for hiding this comment

wesleybl commented Jul 18, 2023

wesleybl commented Jul 21, 2023

wesleybl commented Jul 25, 2023

Akshat2Jain commented Jul 30, 2023

wesleybl commented Jul 31, 2023

Akshat2Jain commented Aug 1, 2023

Akshat2Jain commented Aug 1, 2023

wesleybl commented Aug 1, 2023

Akshat2Jain commented Jun 13, 2023 •

edited

Loading

netlify bot commented Jun 13, 2023 •

edited

Loading

wesleybl commented Jun 16, 2023 •

edited

Loading

wesleybl commented Jul 6, 2023 •

edited

Loading

wesleybl left a comment •

edited

Loading

Akshat2Jain commented Jul 18, 2023 •

edited

Loading