Infocaster / UrlTracker

An Umbraco package that tracks 404 not found results and helps you redirect visitors to the right page
https://infocaster.net/wat-we-doen/umbraco-plugins/301-url-tracker
MIT License
17 stars 12 forks source link

HasDomainOnChildNode does not work on multisite Umbraco v10 #126

Closed HarrySpyrou closed 1 year ago

HarrySpyrou commented 1 year ago

Describe the bug _Using version: 10.1.0 A clear and concise description of what the bug is:

HasDomainOnChildNode does not do anything in umbraco v10.1. I have a multisite application that has multiple root nodes. Each one's first child has the domain name, not the root itself. When trying to create a new redirect, the plugin shows both the root node and the non root node that has the domain in there:

Capture The redacted selections in there are for clarity (and it's because it's a very known client).

To Reproduce Steps to reproduce the behavior:

  1. Create root node, mark it as such (enable the 'can be root' checkbox in its settings').
  2. Create a child of root. Add the domain URL to the child, not the root.
  3. Go into URL Tracker dashboard, try to create a redirect. Notice both root and child of root are showing.
  4. Switch HasDomainOnChildNode to true in appsettings.json.

Expected behavior I'd expect the root nodes would vanish from the URL Tracker dashboard once HasDomainOnChildNode is set to true, but they didn't. They can still be selected, in which case, I don't see what this setting is supposed to be doing.

Desktop (please complete the following information):

D-Inventor commented 1 year ago

Hi @HarrySpyrou ! Thank you very much for reaching out. Your issue is very clear, I'm going to dive into it and come back to you when I have something to share. That should be in about 1 or 2 days.

D-Inventor commented 1 year ago

Hi again @HarrySpyrou ! So I've gone through the bit of code that collects all the nodes for that list and I have made a discovery. This logic hasn't changed since before I started maintaining it and it looks like the option HasDomainOnChildNode doesn't do anything with child nodes at all, but rather enables and disables the retrieval of so-called "wildcard domains", which is something completely different obviously.

So, to summarize: I also don't know what this setting was ever supposed to do 😅. What you say makes a lot of sense though. How about we just make it that way?

I'll see if I can do some work on the URL Tracker this weekend and change the behaviour of the HasDomainOnChildNode setting so that it hides domains on child nodes if this setting is enabled.

HarrySpyrou commented 1 year ago

I found it funny also that it's not doing even remotely what it should. I have a few suggestions on it though. It seems like you have a feature in your hands already developed and ready to go. I think it would make sense to rename the setting appropriately to something like EnableWildcardDomains and then build a separate HasDomainOnChildNode, which I find way more useful personally (as a feature, that is).

All the projects we have use this plugin and they're all humongous companies, with all our domains on the first child, so this would be perfect. Of course, none of this is up to me, it's all up to you.

However, only one thing I can see from your comment:

change the behaviour of the HasDomainOnChildNode setting so that it hides domains on child nodes if this setting is enabled.

This should be the opposite right? If the setting is enabled it should show domains only on child nodes, not hide them. So if it 'has domain on a child node', then show only that node and remove the root that has no domain from the selection. Am I thinking this the wrong way? I hope not. 😄

Either way I appreciate your responses and your help, because I'd have to fight to hack it somehow outside of the plugin otherwise.

D-Inventor commented 1 year ago

What you say makes a lot of sense. I looked at it from two perspectives:

I wasn't really sure, but yeah I think it's the most friendly to make the option HasDomainOnChildNode enable child domains, but it should be true by default as to not change the currently expected behaviour.

I feel like this product should be shaped together with its users, so your input is very valuable to me. It's just me in my spare time though, so sometimes it's difficult to keep a tight schedule with these developments. Especially with the big redesign that I promised and have been procrastinating on 😅 .

So I'm taking your input with me while I fix this and I hope to give you an update before the end of this week.

HarrySpyrou commented 1 year ago

Good to hear! I'll be available to give my input as much as possible.

I wasn't really sure, but yeah I think it's the most friendly to make the option HasDomainOnChildNode enable child domains, but it should be true by default as to not change the currently expected behaviour.

What is confusing to me as another dev and a user of the plugin at the same time is that in your docs, you're clearly saying 'Set this value to true if your website has domains configured on pages that are not in the root of the website.' Which means that by default is false. So, as a consumer, there is a default behaviour (that's how it looks like reading the docs too) and that default behaviour is false. I think either of these mindsets works, so you could as well keep it true by default, but you'll have to change the docs, that give it as false in the example and the description of the docs that implies it's false by default.

Also, I'm not sure if you meant it but it needs to also disable roots at the same time. So if Child A of Root A has a domain, then that structure will be cloned throughout the application. E.g. Child B will also have the domain, instead of Root B. So, in my mind at least, you don't need to show either Root A or Root B, unless they themselves have domains in their 'culture and hostnames' fields.

I guess as a TL;DR it would make sense like this: If Root node has domain URL assigned AND its child has domain URL assigned then keep (and show in the URL tracker) both. (but you'll have to set the bool to true for the child to show)

If Root only has domain URL, the current default works fine (child doesn't come up since it doesn't have anything in there) and setting it to true without assigning a domain URL on the child will basically don't make any difference. I have probably missed some part of the logic writing this but I've already coded 9 hours today.

Will be looking forward for the update! Have a good weekend!

D-Inventor commented 1 year ago

Hi there! In a few minutes, version 10.3.0-beta.1 should release. This is a pre-release version that includes changes to fix this issue. Would you be willing to give this version a try and see if it works as expected? The readme should also be up-to-date now to reflect the new configuration values.

HarrySpyrou commented 1 year ago

Hello! Thanks for this. I'm testing it now in the same project I saw the problem and I'll have our QA test it too, before I come back to you. A few days I'd guess.

HarrySpyrou commented 1 year ago

Current release I'm running is: 10.3.0-beta0001. So, quick first glance, it shows that when I have it set to 'true', the child nodes that don't have domains assigned are actually hidden from 'create a redirect' feature. But still, when a child has a domain assigned and the parent doesn't, the parent is still visible. So, if the child is named the same as the parent, they both show as a duplicate name:

Here's screenshots showing that the second child (child of root 2) is hiding when there is no domain assigned to it. Child of root 1 has a domain, both roots don't: Capture1

Capture2 Capture3

Here's how it looks in UrlTracker when you have root node and child named the same:

Capture4

D-Inventor commented 1 year ago

Thanks for your detailed analysis. I'm surprised, because we literally use the domain service to get the domains. If a node doesn't have a domain, I don't expect it to show up in the domain service at all. I'll dive back in to it.

Also, just to be sure: the domains are saved in a memory cache, do the root nodes still show up after you reboot your application?

HarrySpyrou commented 1 year ago

I didn't know they're being saved in the cache too, so I went in there just now, still running the beta version and, I emptied Umbraco's cache (from the Settings), I also tried changing a few domains (e.g. adding and removing one in the 'root' node) but nothing seems to make a difference. I'm not 100% sure I'm doing everything right, if you have anything specific you want me to test, let me know.

D-Inventor commented 1 year ago

I have been able to reproduce this behaviour and I've also found the issue: all the root content is explicitly added to the list, once again for reasons beyond me 😅😭

I'll be right back with an update

D-Inventor commented 1 year ago

@HarrySpyrou Version 10.3.0-beta.2 should release in a few minutes. I would greatly appreciate it if you could give that version a go and let me know what you think!

HarrySpyrou commented 1 year ago

Hey, just tested this. It seems to be working perfectly. I'm going to forward this to our QA and I'll come back to you with a final outcome, but I believe this is it. Thanks a lot!

HarrySpyrou commented 1 year ago

Ok, so coming back from our QA, it is verified this works as expected, I think you're safe to make this a release. I've also tested it and can't find something wrong with it.

Edit: if you manage to tag this is as a release before Thursday, I'll be able to include it. Thank you for your effort!

D-Inventor commented 1 year ago

Awesome that it works! Unfortunately, I wasn't able to include this before thursday :/ Release 10.3.0 should become available somewhere within the next hour

HarrySpyrou commented 1 year ago

It's quite alright. Thanks a lot again! I'll leave you to close this once you have your release.

HarrySpyrou commented 1 year ago

@D-Inventor It's going on production in our next release for the client. Am I good to close this?

D-Inventor commented 1 year ago

Thanks for the reminder, I meant to close this, but had forgotten

D-Inventor commented 1 year ago

This issue is fixed in URL Tracker version 10.3.0