Redirects: Fixes self referencing redirects (closes #20139)

[NillasKA]

Description

Addresses: #20139

This also happens on 13. This exact code implemented in 13 will also fix it there if cherry picked to V13.

A bug was found, where if you name a document something, that it has previously been named a self referencing redirect is made.

This PR makes a check, that sees if a redirect is being made that references to the current document URL. If there is, we'll remove that redirect since its not needed.

I am at a loss on how to test this, therefore this is a draft for now until i figured it out.

Integration Test obscurity

Trying to create an integration test for this was obscure to me. My problem was, that when X published a document, the URL generated for this newly published document is the one we run through the service, to see if there exists any redirect to this new URL.

My problem with the tests, was to in essence publish a document with an URL to create this redirect, and then publish it back to the old URL to see if this redirect was deleted, i couldn't figure out how. So what i did instead was to mock some data, to ensure a self referencing redirect already exists through mocked data, and to ensure that the Register() method deletes the self referencing redirect.

I added comments to the test to ensure clarity on what is happening.

Testing

Create a document, like "Test1"
Rename it to "Test2" and see that a redirect has been made.
Name it back to "Test1" and see that no self referencing redirect exists.

[AndyButland]

This looks good @NillasKA, but I just have a little doubt as to where you've placed the code. The reasons is that RedirecUrlService now has a dependency on IPublishedUrlProvider, and the implementation of that requires an Umbraco context. That's the reason you've had to modify the tests.

But that also means the Register method can no longer be called by something outside of Umbraco - like a background thread - as it'll throw an exception when it can't get a required Umbraco context. And there could be solutions out there that rely on that.

Perhaps you can solve this by moving the logic that removes an existing redirect to RedirectTracker. That already depends on and makes calls to IPublishedUrlProvider, so we aren't regressing anything there. And it would seem we can avoid calling this twice, as we have already looked up the new URL for the content here.

So I'm thinking, in RedirectTracker.CreateRedirects, you could do this:

• Get the new route (line 105).
• Check it's valid (lines 106-109)
• Apply the look up and delete logic you have added here, but instead of calling methods on IRedirectUrlRepository use the ones on IRedirectUrlService (or introduce ones on this service if you need them).
• Register the new redirect (line 111).

I haven't tried this so it needs investigation, but do you think this would work?

[NillasKA]

This sounds good. I think it will work

I'll be trying it out today

[NillasKA]

My latest commit is not my final.

I do want to write a few more tests for the RedirectTracker before finalizing this PR. Will do tomorrow.

[NillasKA]

@AndyButland pushed some expanding tests

Had to mock quite a bit to get the RedirectTracker.StoreOldRoute test to work, let me know if you see something out of the ordinary

[AndyButland]

Great work @NillasKA, especially on getting this under test. I pushed a few small updates, nothing significant but you can take a look. Have tested manually too and works as expected.