-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add AutorouteSettings.AllowSetAsHomePage to control whether user can set a content to display on a home page #8424
Add AutorouteSettings.AllowSetAsHomePage to control whether user can set a content to display on a home page #8424
Conversation
…set a content to display on a home page
@MatteoPiovanelli-Laser opinion ? LGTM |
@sebastienros Thanks for prompt review. |
Looks good to me, but also dangerous in some cases. It's pretty easy to set things up so that nobody can set any homepage. This may be desired, but there should at least be a warning for admins somewhere. Possibly something more than just a warning. This scenario is 100% sure to happen in any number of real-world tenants:
|
@MatteoPiovanelli-Laser When a content item A gets deleted, that person should be available to help to set the setting back to true but it can be a case that there are many admins and a person who creates a content type A and a person who maintains a website is not the same person. |
@MatteoPiovanelli-Laser 1st option 2nd option For me, AllowSetAsHomePage is still valid for some content types that do need to show on the home page at all. Additional suggestion to improve the system |
@MatteoPiovanelli-Laser Any suggestion on this for me to continue improve this PR? |
This won't really help for a scenario where I set up the tenant for a client, and they don't have all required permissions to manage ContentDefinitions. They would still end up with a site without an homepage for the time it takes me to reply to their call/ticket.
I kind of like this. "You are about to set a new home page. Are you sure?".
Maybe this would be better covered by a different setting? I am thinking something more like
There are a lot of ways for a ContentItem to be deleted/unpublished, and it's hard to make sure they would always play nice with what you are describing here. If I remember it right, right now if you somehow delete the HomePage you get notified after the fact, and the tenant is temporarily without an HomePage.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and added some extra description for the new setting.
I understand the concerns laid out above, but the default behavior doesn't change with this addition and the users who can change this setting are presented with the note about potential pitfalls.
Right now, when we create or edit a content item with autoroute part. It shows "Set as home page" option (checkbox)
to a user who has "SetHomePage" permission, let's say admin.
Current behavior
For a custom content type that does not to set as a home page, I think we should have a setting to disable this option because of
showing this option is easy for admin to make a mistake.
For example, if admin create a new content item with that custom content and accidentally check "set as home page".
This can cause an unexpected result to a website's home page and if an admin does not know how to restore a home page by finding the previous home page content item and check set as home page option.
This process can take time for non-experience admin and is not good for a business.
This PR is simple. It only adds AllowSetAsHomePage to AutorouteSettings.
"Set as home page option" will show only if a user is admin and AllowSetAsHomePage is set to true.
Editor page after we set AllowSetAsHomePage to false
No Set as home page option
For backward compatibility, AllowSetAsHomePage is set to true so for existing content types will show this option.
This also can be convenience when defining a new type programmatically so we can do something like this:
Thanks