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

🐛 Fixed duplicate tags created when slugs contain spaces #14277

Merged
merged 8 commits into from
Mar 10, 2022

Conversation

SimonBackx
Copy link
Contributor

refs https://github.com/TryGhost/Team/issues/1284

  • Matching tags by slug has been updated to work for slugs with spaces
  • Also added a test to check slugs that are longer than 185 characters (since that will cause the same issue)
  • Now, the tag slug gets filtered before doing any matching unless a tag id (or foreign key) is set when creating or editing posts.
  • Added skipDuplicateChecks option to generateSlug model plugin.
  • Added tests

Edge case

Say that we have an existing tag: {slug: 'my-slug', name: 'This is my slug'}

When you create a new post like this:
api.posts.add({title: 'test post', tags: [{slug: 'my slug'}]})

The current solution is to transform it into the following query in the onSaving event of Post:
api.posts.add({title: 'test post', tags: [{slug: 'my-slug'}]})

This works! But the only downside is, that when creating a new tag, it creates it with the name 'my-slug' instead of 'my slug'.

To work around this issue there needs to be a way for bookshelf-relations to have a different matching logic (to search for existing tags), separate from the insert logic (when creating new tags). Currently you cannot match existing tags on the slug my-slug (and not match on name my slug at the same time), while setting the name to my slug when inserting (but not when updating an existing tag). This gets far too complex for this small issue, since the user can just change the name afterwards.

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #14277 (b355b9d) into main (d8295da) will decrease coverage by 0.00%.
The diff coverage is 46.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14277      +/-   ##
==========================================
- Coverage   55.48%   55.48%   -0.01%     
==========================================
  Files         565      565              
  Lines       46840    46862      +22     
  Branches     3899     3898       -1     
==========================================
+ Hits        25991    26003      +12     
- Misses      20810    20820      +10     
  Partials       39       39              
Impacted Files Coverage Δ
core/server/models/post.js 36.31% <6.66%> (-0.21%) ⬇️
core/server/models/base/plugins/generate-slug.js 85.95% <92.30%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8295da...b355b9d. Read the comment docs.

@SimonBackx SimonBackx force-pushed the fix/duplicate-tags-post-creation branch from 2d15657 to 6f35129 Compare March 7, 2022 16:52
@SimonBackx SimonBackx force-pushed the fix/duplicate-tags-post-creation branch from 96c9cdb to 3b9c439 Compare March 8, 2022 08:06
refs https://github.com/TryGhost/Team/issues/1284

When you create a new post with a tag that contains spaces, those spaces will get replaced by dashes. But instead of reusing an existing tag, a new tag is created.
refs https://github.com/TryGhost/Team/issues/1284

Matching tags by slug has been updated to work for slugs with spaces, but the fix causes a different complicated bug (test added and still failing).
refs https://github.com/TryGhost/Team/issues/1284

I was running into this difficult issue to solve:
Say that we have an existing tag: `{slug: 'my-slug', name: 'This is my slug'}`.

When you create a new post like this:
`api.posts.add({title: 'test post', tags: [{slug: 'my slug'}]})`

The current solution is to transform it into the following query in the onSaving event of `Post`:
`api.posts.add({title: 'test post', tags: [{slug: 'my-slug'}]})`

This works, but the only downside is that when creating a new tag, it creates it with the name 'my-slug' instead of 'my slug'.

To work around this issue there needs to be a way for `bookshelf-relations` to have a different matching logic (to search for existing tags), separate from the insert logic (when creating new tags). Currently you cannot match existing tags on the slug `my-slug` (and not match on name `my slug` at the same time), while setting the name to `my slug` when inserting (but not when updating an existing tag) . As you understand, this gets far too complex for this small issue, since the user can just change the name afterwards.
@SimonBackx SimonBackx force-pushed the fix/duplicate-tags-post-creation branch from 3b9c439 to b355b9d Compare March 10, 2022 11:50
@SimonBackx SimonBackx merged commit da9de95 into TryGhost:main Mar 10, 2022
@SimonBackx SimonBackx deleted the fix/duplicate-tags-post-creation branch March 10, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants