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

Add AutorouteSettings.AllowSetAsHomePage to control whether user can set a content to display on a home page #8424

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

aaronamm
Copy link
Contributor

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
image

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
image

For backward compatibility, AllowSetAsHomePage is set to true so for existing content types will show this option.
image

This also can be convenience when defining a new type programmatically so we can do something like this:

ContentDefinitionManager.AlterTypeDefinition(
    "Announcement",
    type => type
        .WithPart("TitlePart")
        .WithPart("BodyPart",
            part => part
                .WithSetting("BodyTypePartSettings.Flavor", "html")
        )
        .WithPart(
            "CommonPart",
            part => part
                .WithSetting("DateEditorSettings.ShowDateEditor", "False")
                .WithSetting("OwnerEditorSettings.ShowOwnerEditor", "False")

        )
        .WithPart("PublishLaterPart")
        .WithPart(
            "AutoroutePart", 
            part => part
                .WithSetting("AutorouteSettings.AllowCustomPattern", "False")
                .WithSetting("AutorouteSettings.AutomaticAdjustmentOnEdit", "False")
                .WithSetting("AutorouteSettings.AllowSetAsHomePage", "False")
                .WithSetting("AutorouteSettings.PatternDefinitions",
                    "[{'Name': 'Announcement details by id', 'Pattern': 'Announcements/{Content.Id}', 'Description': 'Announcements/Announcement-Id'}]"
                ))
        .Draftable()
);

Thanks

@sebastienros
Copy link
Member

@MatteoPiovanelli-Laser opinion ? LGTM

@aaronamm
Copy link
Contributor Author

@sebastienros Thanks for prompt review.

@MatteoPiovanelli-Laser
Copy link
Contributor

Looks good to me, but also dangerous in some cases.
I think it's missing a few "notifications" here and there.

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:

  • Have ContentTypeA.
  • Create a ContentTypeA called HomePage. Set it as homepage.
  • Switch the new setting, so that nobody can set any other content as the new homepage.
  • HomePage gets deleted.
  • Now the tenant has no homepage and errors out (I think it 404s) immediately.
    While this can be made to happen also without the setting, that flag allows the tenant to be set up in such a way that "normal" back office users have no way of setting a new homepage. Even users with the permission to set an homepage would then require a permission to edit the ContentDefinition before they can do that.

@aaronamm
Copy link
Contributor Author

@MatteoPiovanelli-Laser
Thanks for your feedback.
Your comment is valid and I agree with it.
However, this setting is enabled to true by default.
Therefore, a person who creates a content type A should know how to disable this setting. If not, "Set as home page" always show on an editor page.

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.

@aaronamm
Copy link
Contributor Author

aaronamm commented Oct 16, 2020

@MatteoPiovanelli-Laser
I have two options that I can think of.

1st option
Show the "Set as home page" check box on an editor page with a disable state and a hint message on how to enable this feature. e.g.
To enable "Set as home page", log in as an admin user or a user who has SetHomePage permission. Then go to content definition > "current type" > Expand Autoroute setting and check "Allow set as home page".
(An instruction can be improved) or provide a direct link to that setting page.

2nd option
Show an alert message when a user checks "Set as home page" on editor page to confirm them about set a content item to the home page.
This option causes "AllowSetAsHomePage" setting is unnecessary because we always show it.

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
I think for a content item set as home page, when a user tries to delete it we should have a warning message to let them know that they are going to delete a home page content that can cause 404 on the home page.
This should help to prevent unintended deleting as well.

@aaronamm
Copy link
Contributor Author

@MatteoPiovanelli-Laser Any suggestion on this for me to continue improve this PR?
Thanks.

@MatteoPiovanelli-Laser
Copy link
Contributor

@MatteoPiovanelli-Laser
I have two options that I can think of.

1st option
Show the "Set as home page" check box on an editor page with a disable state and a hint message on how to enable this feature. e.g.
To enable "Set as home page", log in as an admin user or a user who has SetHomePage permission. Then go to content definition > "current type" > Expand Autoroute setting and check "Allow set as home page".
(An instruction can be improved) or provide a direct link to that setting page.

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.

2nd option
Show an alert message when a user checks "Set as home page" on editor page to confirm them about set a content item to the home page.

I kind of like this. "You are about to set a new home page. Are you sure?".

This option causes "AllowSetAsHomePage" setting is unnecessary because we always show it.
For me, AllowSetAsHomePage is still valid for some content types that do need to show on the home page at all.

Maybe this would be better covered by a different setting? I am thinking something more like PreventSetAsHomePage. When that is true, a ContentItem of that type cannot be set as HomePage. When that is false (default), contents behave the same as they do now. Would this cover the use cases you have in mind? Then there could be a notification shown in case no ContentType has PreventSetAsHomePage == false.
This could be "extended" with a list of "default" ContentTypes or ContentParts, injectable by reading the config files, that would have PreventSetAsHomePage = true: I am thinking, Terms, Taxonomies and so on.

Additional suggestion to improve the system
I think for a content item set as home page, when a user tries to delete it we should have a warning message to let them know that they are going to delete a home page content that can cause 404 on the home page.

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.

This should help to prevent unintended deleting as well.

Copy link
Member

@BenedekFarkas BenedekFarkas left a 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.

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

Successfully merging this pull request may close these issues.

None yet

4 participants