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) #8316

Closed

Conversation

ronaldbarendse
Copy link
Contributor

@ronaldbarendse ronaldbarendse commented Jun 19, 2020

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.

Description

Domain service and repository (database)

I've started creating all the necessary plumbing to allow sorting domains (set under 'Culture and Hostnames'):

  • Adding the SortOrder property to the service level models (IDomain and UmbracoDomain) and database model (DomainDto);
  • Adding the new sortOrder column to umbracoDomain and setting initial values in a migration;
  • Add Sort method to DomainService (similar to those on 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.

As per #8299 (comment), I've ensured to limit breaking changes to repository/service interfaces and have only done additions to them.

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:
      Culture and Hostnames
    • 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:
    • The log should include the executed SQL commands:
      Database requires upgrade
      "Executing installation step: 'DatabaseUpgrade'." [Timing "c8007c9"]
      Database upgrade started
      Running 'Upgrade' service
      Starting '"Umbraco.Core"'...
      Execute "AddDomainSortOrder"
      At "{a78e3369-8ea3-40ec-ad3f-5f76929d2b20}"
      SQL [0]: "ALTER TABLE [umbracoDomain] ADD [sortOrder] INTEGER NOT NULL CONSTRAINT [DF_umbracoDomain_sortOrder] DEFAULT ('0')"
      SQL [1]: "ALTER TABLE [umbracoDomain] DROP CONSTRAINT [DF_umbracoDomain_sortOrder]"
      SQL [2]: UPDATE [umbracoDomain] SET sortOrder = id
      SQL [3]: UPDATE [umbracoDomain] SET [umbracoDomain].[sortOrder]=@0 WHERE (LEN(domainName) = 0 OR CHARINDEX('*', domainName) = 1) -- @0:-1
      At "{ace18303-f74a-40d2-83c4-4ab24b33cbfb}"
      PostMigration: Umbraco.Web.Migrations.PostMigrations.ClearCsrfCookies.
      Done (pending scope completion).
      
      • The umbracoDomain table should have the additional sortOrder column with initial sort orders.
  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. Change/reverse the sort order by editing the values in the database, restart the application (to clear the cache) and ensure the domains are returned/shown in the correct order 🎉

Sorting domains in back-office

Next up is updating the ContentController and UI to allow re-ordering/sorting the domains. This is done by keeping track of the saved domains and sort them according to the view model that's made sort-able:

Sortable Culture and Hostnames

Ensuring sorting is honoured within Umbraco

The last step is to make sure the sorting is honoured where needed. For now, I've ensured the DomainCache and DomainUtilities keeps the correct sort order. Because DefaultUrlProvider depends on these classes, it should return URLs in the correct order/with the first domain. Custom IUrlProviders might return the URLs in a different order though, but that's out of this PRs scope.

src/Umbraco.Core/Migrations/Upgrade/UmbracoPlan.cs Outdated Show resolved Hide resolved
@@ -19,6 +19,7 @@ protected override void DefineMaps()
DefineMap<UmbracoDomain, DomainDto>(nameof(UmbracoDomain.RootContentId), nameof(DomainDto.RootStructureId));
DefineMap<UmbracoDomain, DomainDto>(nameof(UmbracoDomain.LanguageId), nameof(DomainDto.DefaultLanguage));
DefineMap<UmbracoDomain, DomainDto>(nameof(UmbracoDomain.DomainName), nameof(DomainDto.DomainName));
DefineMap<UmbracoDomain, DomainDto>(nameof(UmbracoDomain.SortOrder), nameof(DomainDto.SortOrder));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure where this mapper gets used, so I've just added the additional map for the SortOrder property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a mapper can be used instead of the factory methods, but I can't find documentation about the differences (most factories in Umbraco.Core.Persistence.Factories also have a mapper in Umbraco.Core.Persistence.Mappers).

src/Umbraco.Web/Editors/ContentController.cs Show resolved Hide resolved
src/Umbraco.Web/Editors/ContentController.cs Show resolved Hide resolved
Comment on lines +82 to +88
/// <summary>
/// Gets the sort order.
/// </summary>
/// <value>
/// The sort order.
/// </value>
public int SortOrder { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Domain model (used in IDomainCache) also requires a sort order, as the cache doesn't keep the order of the collections.

var domainsAndUris = domains
.Where(d => d.IsWildcard == false)
.Select(d => new DomainAndUri(d, uri))
.OrderByDescending(d => d.Uri.ToString())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ordering by URL is only required for SelectByBase(), so more specific URLs get matched first and therefore removed here to keep the correct domain sort order.

src/Umbraco.Web/Routing/DomainUtilities.cs Show resolved Hide resolved
src/Umbraco.Web/Routing/DomainUtilities.cs Outdated Show resolved Hide resolved
@ronaldbarendse ronaldbarendse marked this pull request as ready for review June 29, 2020 20:51
@ronaldbarendse
Copy link
Contributor Author

The changes in DomainUtilities do seem to work, as the Umbraco.Tests.Routing.UrlsProviderWithDomainsTests.Get_Url_Alternate test now returns the domains in a different order 😏 I'll add explicit sort orders in the UmbracoDomain mocks (instead of setting them all to 0) and fix the assertions order!

@nul800sebastiaan nul800sebastiaan added community/pr status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier) labels Jul 3, 2020
@ronaldbarendse
Copy link
Contributor Author

@nul800sebastiaan I've merged the latest changes from v8/contrib again and verified everything still works. Would be great to have this feature added to v8 and merged to v9 before the final version is released. The migration currently targets 8.16, but changing this to a later version would be a small change to the UmbracoPlan and the directory/namespace of the migration 😉

Although this PR adds a new property on the IDomain/UmbracoDomain models, that shouldn't be a major breaking change (it is even documented as non-breaking).


This feature allows changing the primary domain that's returned when generating URLs (if the current request isn't associated to a domain):

Demo with domain names


But I recon the best thing about this would be to allow changing the order of a language dropdown for culture variant content:

Demo with culture variants

In its most simplistic form, this could be achieved using the following code (inside the master template):

@foreach (var domain in UmbracoContext.PublishedSnapshot.Domains.GetAssigned(Model.Root().Id))
{
    <a href="@Model.Url(domain.Culture.Name)">@domain.Culture.DisplayName</a>
}

A more concise way would be to use the domain from the current request (and only fall-back to the root node ID when not found) and check whether the culture is actually published for that content. I would recommend adding this code in a child action of a surface controller, populate a view model and render this using a partial view. This code would then be something like this:

@{
    var currentPage = Model; // Only this variable and the UmbracoContext are required for the following code
    
    var rootId = UmbracoContext.PublishedRequest.HasDomain ? UmbracoContext.PublishedRequest.Domain.ContentId : currentPage.Root().Id;
    foreach (var domain in UmbracoContext.PublishedSnapshot.Domains.GetAssigned(rootId))
    {
        var culture = domain.Culture.Name;
        string url;
        if (currentPage.IsPublished(culture))
        {
            // Use the URL of the current page in the requested culture
            url = currentPage.Url(culture);
        }
        else if (currentPage.Id != rootId && UmbracoContext.PublishedSnapshot.Content.GetById(rootId) is IPublishedContent root && root.IsPublished(culture))
        {
            // Fallback to the root page, so even if the current page isn't available in the requested culture, we can switch to this culture
            url = root.Url(culture);
        }
        else
        {
            // Both the current and root page aren't published in the requested culture
            continue;
        }
        
        // Do something with the URL
        <a class="nav-link" href="@url">@domain.Culture.DisplayName</a>
    }
}

@bjarnef
Copy link
Contributor

bjarnef commented Sep 7, 2021

@ronaldbarendse @nul800sebastiaan is this something that is planned to include in any near future? 🙈
I can't count the number of times I have wanted to sort the domains added on a site/root node.
Often I prepare domains on the site node in Umbraco close the launch of a site on Umbraco Cloud, but it seems if the project-alias.s1.umbraco.io or project-alias.euwest01.umbraco.io domain isn't the first one, menu item in frontend use the real domain. So currently they need to be added in the correct order.

@umbrabot
Copy link

Hi there @ronaldbarendse!

Thanks so much for your contribution! We wanted to let you know that we love the content! Unfortunately, we have spent some time looking at the open PRs for the last minor release of Umbraco 8 and we have decided that while the work is great, we have decided not to merge this.

We hope to make 8.18 an umbazing release so we took the time to assess each contribution during our CMS workshop. If you would like to know a little more about that process, take a look at this article.

We would, however, be super happy to merge this into Umbraco 9. If you would be happy to take a look at how you could adapt this for Umbraco 9, please raise a PR and we'll take a look! For info on how to get started with Umbraco 9, check out these docs.

In the meantime, a big #H5YR goes out to you for your fine work here.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community/pr status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort domains and/or languages
4 participants