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

Sort domains (Culture and Hostnames) #13797

Merged
merged 11 commits into from Feb 14, 2023
Merged

Sort domains (Culture and Hostnames) #13797

merged 11 commits into from Feb 14, 2023

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #8299.

This includes most changes done in PR #8316 that targeted v8.

Description

As mentioned in the above linked issue (and v8 PR), ordering of the node domains is important, since the first one will be treated as the primary domain (when generating absolute URLs and none match the current URL) and this order can also be used to automatically generate a language selector on the front-end.

This PR includes the following changes:

  • Adding the SortOrder property to the service level models (Umbraco.Cms.Core.Models.IDomain and Umbraco.Cms.Core.Models.UmbracoDomain), database model (Umbraco.Cms.Infrastructure.Persistence.Dtos.DomainDto) and routing/NuCache model (Umbraco.Cms.Core.Routing.Domain);
  • Adding the new sortOrder column to the umbracoDomain table and setting initial values in a migration (compatible with SQLite);
  • Add a Sort method to Umbraco.Cms.Core.Services.IDomainService with a default interface implementation (returning a failed operation result) and implemented this in the Umbraco.Cms.Core.Services.DomainService (using a similar contract/implementation as other services);
  • Ensured a new, incremental, sort order is set for new domains:
    • Wildcard domains (setting the language of a content node) are not sorted by setting the sortOrder to -1, as only a single one of this type should be available per content node.
  • Ensured the DomainCache and DomainUtilities both honour the sort order when returning the domains and selecting the primary domain (and thereby ensuring DefaultUrlProvider uses the correct primary domain when generating absolute URLs);
  • Updated the UI to allow sorting the domains and at the same time improved the UX of the 'Culture and Hostnames' dialog:
    • The domains are displayed in a more compact and consistent way:
      Culture and Hostnames dialog
    • Validation messages are now also displayed in a similar way as property errors:
      Culture and Hostnames dialog validation

These changes can be tested by following these steps:

  1. Setup an Umbraco instance with the Starter Kit, but without this PR applied and:
    • Add additional languages (besides en-US);
    • Set the language on the root node and add some additional domains/languages:
    • Do the same on another node, so ordering per node can be tested later;
    • Note the current order of domains.
  2. Apply this PR and complete the upgrade, the migration should have executed and added the new column (check the log and/or database).
  3. Check whether the domains on the configured nodes are sorted the same as before (as the auto-incremented id is set as default sort order in the migration).
  4. Check whether deleting and re-adding domains will always put them last with a single incremented sort order for that node.
  5. Check whether the wildcard domains/node languages sort order is always set to -1, even after setting it to Inherit and back to a language.
  6. Check whether the front-end renders the domains in the correct order by adding this to the master template (for simplicity's sake I'm ignoring nullability):
 @foreach (var domain in UmbracoContext!.PublishedSnapshot!.Domains!.GetAssigned(Model.Root().Id))
 {
     <a href="@Model.Url(domain.Culture)">@Model.Name(domain.Culture)</a>
 }

@ronaldbarendse ronaldbarendse marked this pull request as ready for review February 7, 2023 23:18
Copy link
Member

@Zeegaan Zeegaan left a comment

Choose a reason for hiding this comment

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

Found a few small nitpicky things 👍 But overall looks good 😁

src/Umbraco.Core/Routing/DomainUtilities.cs Show resolved Hide resolved
src/Umbraco.Core/Routing/DomainUtilities.cs Outdated Show resolved Hide resolved
Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>
@Zeegaan
Copy link
Member

Zeegaan commented Feb 14, 2023

Looks good, tests good 🎉

@Zeegaan Zeegaan merged commit 45036f5 into v11/dev Feb 14, 2023
@Zeegaan Zeegaan deleted the v11/feature/sort-domains branch February 14, 2023 09:35
@ronaldbarendse
Copy link
Contributor Author

I just noticed the release notes/blog post mentions the 'Add current domain' as being a new feature, but that was already included in the previous minors (in PR #13436) and should be credited to @prjseal 😉 This PR does improve the button UI and it was recently added, so I do understand the slight confusion...

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

Successfully merging this pull request may close these issues.

None yet

3 participants