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

fix #411 serializing default_page item property into something like … #983

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sauzher
Copy link

@sauzher sauzher commented Aug 24, 2020

"default_page": {"@id": "http://localhost:8080/Plone/front-page"}

…ike `"default_page": {"@id": "http://localhost:8080/Plone/front-page"}`
@mister-roboto
Copy link

@sauzher you need to sign the Plone Contributor Agreement in order to merge this pull request.

Learn about the Plone Contributor Agreement: http://docs.plone.org/develop/coredev/docs/contributors_agreement_explained.html

@mister-roboto
Copy link

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

@mister-roboto
Copy link

@sauzher you need to sign the Plone Contributor Agreement in order to merge this pull request.

Learn about the Plone Contributor Agreement: http://docs.plone.org/develop/coredev/docs/contributors_agreement_explained.html

@sauzher sauzher closed this Aug 24, 2020
@sauzher sauzher reopened this Aug 24, 2020
@mister-roboto
Copy link

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

@coveralls
Copy link

coveralls commented Aug 25, 2020

Coverage Status

Coverage decreased (-0.1%) to 94.49% when pulling 7b40aa9 on sauzher:issue-411-default_page into 4daffd2 on plone:master.

@sauzher
Copy link
Author

sauzher commented Aug 25, 2020

I really do not know how to solve this https://travis-ci.org/github/plone/plone.restapi/jobs/720903264#L346.
(actually I'm not getting what is the problem)
any hint?

@tisto
Copy link
Sponsor Member

tisto commented Aug 25, 2020

@sauzher please run black on your code base:

bin/black src/

Improving the CI error message has been on my list for quite a while. Travis is limited though.

@sauzher
Copy link
Author

sauzher commented Aug 25, 2020

@jenkins-plone-org please run jobs

Copy link
Contributor

@tiberiuichim tiberiuichim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most useful is that when you include the default page you also serialize it in the response. This would avoid needing to fetch it again. Maybe implement it as an expander?

docs/source/conf.py Outdated Show resolved Hide resolved
@sauzher
Copy link
Author

sauzher commented Aug 27, 2020

@tiberiuichim thanks for the pull review.

I think most useful is that when you include the default page you also serialize it in the response. This would avoid needing to fetch it again. Maybe implement it as an expander?

I don't know the desiderata behind expander section.
But, IMHO the default_page must be considered as a simple object property. actually it is only that. My point is that it is a task of the frontend engine choosing how to handle this information i.e. fetching the default_page object if needed just like any other component of the context.

@tiberiuichim
Copy link
Contributor

@sauzher Indeed, it is the job of the frontend to decide if it wants this information. With an expander, it can do that:

curl http://localhost:3000/somefolder?expand=default_page

So it would get the "context", but also whatever content it needs to use to properly show the page. What should be included in the default_page expander? Just fully serialize that page, so that it can be used to show the proper content.

@sauzher
Copy link
Author

sauzher commented Aug 31, 2020

@sauzher Indeed, it is the job of the frontend to decide if it wants this information. With an expander, it can do that [...]

@tiberiuichim Thanks, I was unaware of this possiblity.

Dunno what to say... honestly, at this time, I personally would prefer a two step approach on fetching a container and its default_page. They are still two separate context with theirs own components information (workflows, actions, etc.). Having ID (in the default_page property) and its title, description, type properties available in the items section I think it might be enough.

Perhaps dealing with containers with thousands items (or more) it may be better having default_page's metadatas directly into the new default_page section?

Fetching the container first and then container?expand=default_page having all the default_page information nested somewhere in the json i think is in someway confusing. But I admit it's quite subjective, though.

@tiberiuichim
Copy link
Contributor

tiberiuichim commented Aug 31, 2020

Just throwing #944 into discussion. I'm ok either way

@sauzher
Copy link
Author

sauzher commented Aug 31, 2020

@jenkins-plone-org please run jobs

@sauzher
Copy link
Author

sauzher commented Oct 22, 2020

@jenkins-plone-org please run jobs

@thomasmassmann
Copy link
Member

For the deserializer, we currently have a PATCH service for a customer project:

class SetDefaultPage(Service):
    """Set the default page for the given content object."""

    def reply(self):
        data = json_body(self.request)
        default_page = data.get("default_page", None)
        try:
            self.context.setDefaultPage(default_page)
        except Unauthorized as exc:
            self.request.response.setStatus(403)
            return dict(error=dict(type="Forbidden", message=str(exc)))
        except BadRequest as exc:
            self.request.response.setStatus(400)
            return dict(error=dict(type="Bad Request", message=str(exc)))

        return self.reply_no_content()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
EEA Priorities
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

6 participants