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

Check reserved words on creating new content type instances #4486

Open
3 tasks
ksuess opened this issue Mar 8, 2023 · 12 comments
Open
3 tasks

Check reserved words on creating new content type instances #4486

ksuess opened this issue Mar 8, 2023 · 12 comments

Comments

@ksuess
Copy link
Member

ksuess commented Mar 8, 2023

As editor I should not be able to create a page "register" in site root. This clashes with the registration form.

@AbhishekCS3459
Copy link

@ksuess should I proceed with this issue??

@ksuess
Copy link
Member Author

ksuess commented Mar 9, 2023

@AbhishekCS3459, I updated the issue description. If @plone/volto-team verifies, than you can go with it.

@Azzam1503
Copy link

@ksuess is this issue verified now?

@ksuess
Copy link
Member Author

ksuess commented Mar 17, 2023

Yes, you can go for it. Great that you want to implement a solution.
Normally a resource is created with title and some more data, but not a concrete id.
You can see this in Chrome developer tools when you create a page.
grafik

In the backend plone.restapi cares to generate a valid id. At least in most cases.
To fix this issue it is necessary to test on reserved words of the frontend. The backend does not know about these reserved words of the frontend.
If the test is positive, then an id (title to lower case and an appended "-1") should be passed additionally to "createContent" in https://github.com/plone/volto/blob/master/src/components/manage/Add/Add.jsx#L193-L212

In plone.restapi you can see that an id can be passed on content creation:
https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/add.py#L27

So for the test, please take into account "config.settings.reservedIds" and "config.settings.nonContentRoutes".
Have a look at "IdWidget" on how this is doing such a test.

Please don't hesitate to ask if you have any questions.

@BhuvaneshPatil
Copy link
Contributor

Hello @ksuess , can you please provide me the steps to reproduce the behavior in browser. This will help me understand the code and process.
thank you.

@ksuess
Copy link
Member Author

ksuess commented Mar 23, 2023

@BhuvaneshPatil , please read the issue description.

@BhuvaneshPatil
Copy link
Contributor

BhuvaneshPatil commented Mar 24, 2023

Hello @ksuess , I am debugging this behavior, my observation is the field id is undefined when we are sending the post request.

image

So my solution would be to check the name (title) of page and compare it to all the reserved keywords for URLs in frontend.

there are two possibilities -

  • The title is single word
  • The title contains more than one word

To get all reserved words we can use defaultRoutes and get all the words from them -
example -
/controlpanel/dexterity-types/:id/layout will give - layout
and further on.

I wanted to know one thing - how the name is being converted to id. My general understanding till now from experimenting is -

take the name - convert it into lowercase - and replace spaces with - sign

or is there any function for the same?

I also have one query -
you have mentioned about modifying id after it's created, does that mean we can send the updated id in the request?
if we send the updated id, for example -
for name = register
if we send it as - register-1, will backend take of it?

@ksuess
Copy link
Member Author

ksuess commented Mar 27, 2023

@BhuvaneshPatil A resource is created with title and some more data, but not a concrete id.
This should stay as it is, with the exception for a positive test.
The test should check, like mentioned before, "config.settings.reservedIds" and "config.settings.nonContentRoutes".
So, no id generation is necessary. Only in the case of a positive test, add a "-1" to the matched word of the test.

At the end, also handle the case that for example "register" two times is tried to use. Then the second "register-1" is not valid, because it is already taken.

@ksuess
Copy link
Member Author

ksuess commented Mar 27, 2023

@BhuvaneshPatil Due to the recent changes in Plone repos, I am not sure if you are able to open a new branch. This was until now the recommended way, instead of opening a branch on a fork. Please try to open a branch here in plone/volto. If it's not possible, please let us know.

@BhuvaneshPatil
Copy link
Contributor

Thank you for your reply, I tried sending register-1 in the id field manually, and the url had the correct id ( /register-1 ) when I clicked on the link for that page from nav bar.

@BhuvaneshPatil
Copy link
Contributor

Hello @ksuess , I have implemented the functionality to check the id for first time. For example -
When we create a page with register title, it will create the page with register-1 as id.
I am deciding on approach to handle situation when someone is registering with same name i.e. register again.

One of my solution is to call an API with modified id, in above case it will be - 'register-1`. If it returns 404 then we can safely continue our operation. If it doesn't we notify the user to change the title of page.

The drawback of this approach is that, we are giving an extra call for API.

To simplify this approach we can use the error thrown by the this.props.createContent().

And show reset the id to undefined, then user will have to change the title.

image

The downside of this approach is bad UX.

I will appreciate your inputs and suggestions on the same.

@ksuess
Copy link
Member Author

ksuess commented Mar 31, 2023

@BhuvaneshPatil I thought about it now. A bit late, I had to focus on a project.

In my opinion, the backend should deal with "Create content with id xy in container, even if an object with this id alreay exists in this container. It is not the frontend that should forsee this problem, I think.

Just to be clear: Why the check for reserved words in frontend is necessary, is that some strings like frontend routes, like "register", are not known by the backend. They are configured in frontend only.

Let's see the opinions about plone/plone.restapi#1613

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

No branches or pull requests

4 participants