A Fix for the permalinks in #1136

[savente93]

Code changes

When taking another look at my PR #1136 I noticed that it didn't actually solve the problem because it didn't merge the permalinks properly. apologies for that. I checked that Also, my git fu is not amazing but I've tried to keep the commit history as clean as possible, I hope it's okay like this.

Checklist

• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Are you doing the PR on the next branch?
• Have you created/updated the relevant documentation page(s)?

[Keats]

The test failures seem legit?

[savente93]

they were, I had forgotten to run the entire test suite rather than just the ones for the taxonomy, that's my bad. The point is that the taxonomy names now get saved slightly differently to detect the collisions where necessary. The only change necessary was to add | replace(from="-", to=" ") | title to the relevant templates where tag.name was used, it should be good now

[Keats]

Can you do a separate PR with some failing tests for the permalinks? I can have a look as well.

[savente93]

I think I found a solution that works and doesn't require moddifications to the templates to pass all tests. If you're happy about the changes I'll try and clean up the history before you merge the request

[Keats]

Looks ok to me. Do you think there are enough tests covering that or should we add more?

[Keats]

stable

[savente93]

I assume you mean that you prefer the original bit since that is already stable, which is fine, I'll revert it when I make the change

[savente93]

Edit, sorry I just realised sorting by slug first is actually necessary since the deduping otherwise fails. Tests fail with the original sorting

[savente93]

Looks ok to me. Do you think there are enough tests covering that or should we add more?

Thanks for the feedback. I added tests for all cases that I could think of that would result in the same permalink (added whitespace, inconsistent capitalisation and substituting spaces for hyphens) so unless you can think of something on top of that I think it should be okay?

The only thing that is perhaps left untested is how this all interacts with Unicode, but I thought that valid URLs are limited to ASCII, so I didn't add anything for that. I don't actually know enough to make a definitive comment on that so I'll defer to your judgement on that

[Keats]

The only thing that is perhaps left untested is how this all interacts with Unicode, but I thought that valid URLs are limited to ASCII, so I didn't add anything for that.

They get escaped but you can get URL outside of ASCII: https://fr.wikipedia.org/wiki/%C3%89cole (eg École).
If someone is not turning paths into slugs, the 2 taxonomies ecole and école should NOT be merged. If you are slugifying them on the other hand, they should end up merged.

[savente93]

I've added two tests to cover those two cases

[Keats]

Thanks!