Fix permalink already in use

[serverside-is]

Because of a small bug, it is not possible to edit taxonomy terms, when using Absolute paths.

To replicate the error:

• Setup Orchard with the Blog recipe.
• Edit the “Category” Content Type, and on the "Autoroute Part" check "Allow absolute path”.
• Edit the "Categories“ Taxonomy Content Item, and (inside) the "Travel” term. Check “Absolute” and Publish the Term. (this replaces the "categories/travel" path by "travel" on the AutotoutePartIndex)
• Edit the “Travel” Term, again, and Publish.

There will be an error: “Your permalink is already in use.”

I think the problem is this test, in AutoroutePartDisplay.cs (Autoroute module):
if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId) || possibleConflicts.Any(x => !string.IsNullOrEmpty(x.ContainedContentItemId) && x.ContainedContentItemId != model.ContentItem.ContentItemId))

When model.ContentItem is a Term this will always be true, because x.CottentItemId is the ID of the Taxonomy and model.ContentItem.ContentItemId is the ID of the Term. In that case, it's the ID of the contained item that needs to be compared.

I think the test should be:
if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId && (string.IsNullOrEmpty(x.ContainedContentItemId) || x.ContainedContentItemId != model.ContentItem.ContentItemId)))

Another modification is needed, for this to work:

In EditPost of the Taxonomies AdminController, a new content item is being created and merged with the existing Term. That content item is sent to the drivers with a ContentItemId that is not the same as the existing content item. This is a problem because this ID is tested in the above mentioned driver. So, I added a contentItem.ContentItemId = existing.ContentItemId;.

[deanmarcussen]

Hmm, I don't disagree with the potential issue, but this is not the correct fix (or if it works for this, it is breaking something else)

It definitely needs to evaluate the list of possible conflicts twice.

I thin you maybe confused as to what model.ContentItem.ContentItemId id is here. It will always be the Term, when evaluating the Term routing.

It may well be that the other part of your fix to maintain content item ids was actually enough to fix the issue.

[serverside-is]

I really think the original version is wrong: When model.ContentItem represents a Term, possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId) will be always be true, making the whole expression true. So, we have a “conflict” even if the index entry represents the exact same Term (witch is not a “conflict”!). That can be verified, with the "replicate steps" I shown above.

After giving this some more thought, I still believe the solution works in all cases. But noticed that the string.IsNullOrEmpty(x.ContainedContentItemId) bit is not doing anything useful:

We just need to find if the index entry does not represent our model.ContentItem (in that case another content item is using the same Path, and we have a conflict). Assuming that every ContentItemId is unique (even a Taxonomy and a Term can't have the same ContentItemId), we just need to do:

if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId && x.ContainedContentItemId != model.ContentItem.ContentItemId)))
{
	// An index entry represents a different item. We have a conflict. Add model error.
	[...]
} 

Does this make sense? Or do I need another cup of coffee? :-)

I made the change, so it can be tested.

(There is yet another problem: we are not running this test (looking for conflicts) when settings.ManageContainedItemRoutes is true and model.Absolute is false, witch allows for duplicate paths like "categories/travel". But I'm not touching that now.)

[deanmarcussen]

Note: this fix needs to be applied to Menus, Custom Settings, and Custom UserSettings (possibly)

[deanmarcussen]

Ok, We all need more coffee when it comes to validation.

Can you merge dev on this, AG renamed the files last week. so we lost all the history as well.

I can repo the issue, we just need to get the fix correct.

What you've done fixes it for contained routing, but introduces other bugs when not using contained routing - which is why this code looks so complicated (and is wrong).

The best thing we could do it for it is write a test, covering some options.

But short of that, let me explain what I think it was supposed to be doing.

if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId) || possibleConflicts.Any(x => !string.IsNullOrEmpty(x.ContainedContentItemId) && x.ContainedContentItemId != model.ContentItem.ContentItemId))

• x => x.ContentItemId != model.ContentItem.ContentItemId for when it is not contained (it doesn't know this inherently)
• !string.IsNullOrEmpty(x.ContainedContentItemId) to stop the next evaluation. When not contained this should be null or empty

At one point this driver got refactored to be shared with the handler, and I wonder if when it was refactored back we lost something.

There was originally a comment above it all originally
// This logic is different to the check in the handler as here we checking

How would you feel about adding some tests to this?
Otherwise we'll go back and forth trying to figure it out I suspect.

[agriffard]

Can you please fix the conflict (merge dev)?

[serverside-is]

Done.

Basically I'm replacing:
if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId) || possibleConflicts.Any(x => !String.IsNullOrEmpty(x.ContainedContentItemId) && x.ContainedContentItemId != model.ContentItem.ContentItemId))

By:
if (possibleConflicts.Any(x => x.ContentItemId != model.ContentItem.ContentItemId && x.ContainedContentItemId != model.ContentItem.ContentItemId))

In possibleConflicts we have index entries for all the items that have the same path as the current one (model.ContentItem). We want to know if any of those is not our current one. In that case, we have another one using the same path: a "conflict".

• If an index entry represents the current item, it will have it's Id either at x.ContentItemId, or, at x.ContainedContentItemId (we don't really care if our current item is a contained item or not. It's Id will be where it should be. And every content item, contained or not, has a different Id.)
• If an index entry represents a different item (conflict!), our current item's Id won't be anywhere to be found (this is what the condition I propose is looking for.)

I think it really is this simple, although it might look a complicated problem at first.

[deanmarcussen]

Finally had time to look at this. Looks good.
Updated the part handler to use the same logic