add tests for tag slugging

[chris48s]

refs #98
refs #116 (comment)

build will be failing for now.. see comments

[codecov]

Codecov Report

Merging #121 into master will increase coverage by 3.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   76.02%   79.23%   +3.21%     
==========================================
  Files          28       25       -3     
  Lines         392      342      -50     
==========================================
- Hits          298      271      -27     
+ Misses         94       71      -23     

Impacted Files	Coverage Δ
project/tagging/models.py	100.00% <100.00%> (+4.76%)	⬆️
project/resources/factories.py
project/tagging/factories.py
project/users/factories.py
project/resources/views.py	72.72% <0.00%> (+4.54%)	⬆️
project/tagging/serializers.py	91.66% <0.00%> (+8.33%)	⬆️

---

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 1ad65a9...7d05d44. Read the comment docs.

[BethanyG]

Going to link issue #72 here as well, since this PR does address some of those scenarios.

[BethanyG]

@chris48s @lpatmo - I think we can probably go ahead and merge this. We'll need to do another pass, once we get solutions/strategies for #123, #124 with related tests .. but I think this is decent start, at least.

[BethanyG]

@lpatmo - since I contributed to this as well, I'd feel better if you gave it a quick review. 😄 Thanks!

[lpatmo]

Just took a look -- thank you for writing this and it looks good, @chris48s! Feel free to merge in!

[chris48s]

Its been a while since I looked at this, but I don't think this is finished.

I'd like to at least add a validator to throw ValidationError on tags with emoji and test that, rather than formalising that a single emoji tag generates a blank slug if that's the desired behaviour.

That said, there are still some open questions here: #121 (comment)

I'd like to understand more about the use-case for slugs in this application first, but if you take #123 at face value, c320bc7 essentially formalises a bug, doesn't it? i.e: the tests pass because the code doesn't do what you want it to. That's the opposite of what you want from a test.

Either way, lets understand what you're doing with the slugs first..

[lpatmo]

I refreshed the page and just saw your new comment! I took a stab at answering the questions in #121 (comment) just now. :)

[lpatmo]

I'd like to at least add a validator to throw ValidationError on tags with emoji and test that

++

[BethanyG]

I'd like to understand more about the use-case for slugs in this application first, but if you take #123 at face value, c320bc7 essentially formalises a bug, doesn't it? i.e: the tests pass because the code doesn't do what you want it to. That's the opposite of what you want from a test.

Yup - technically speaking, it does. So - we can re-write those tests to fail .. but then we need to decide what the expected behavior is...and write some slugification code (or iri_to_uri code) accordingly...or live with 4 continuously failing tests (which seems silly). We could also omit them altogether until we decide.

[chris48s]

OK then. Based on the further discussion, it seems like one of the things you want from slugging is to enforce uniqueness i.e: you shouldn't be able to create 'javascript', 'Javascript' as 2 seperate tags. With the code as it stands on master, I can happily create these as different tags. For example:

>>> tag = CustomTag(name='javascript')
>>> tag.save()
>>> tag.slug
'javascript'

>>> tag = CustomTag(name='Javascript')
>>> tag.save()
>>> tag.slug
'javascript_1'

>>> tag = CustomTag(name='JavaScript')
>>> tag.save()
>>> tag.slug
'javascript_2'

That seems like it is not what you want, so in cdfe87a I have changed this so that:

• we can't have 2 tags which generate the same slug
• we can't have any tag which generates an empty slug (e.g: no single emjoi tag names, or tags called "%" or whatever)

and I've added tests to formalise that behaviour.

On balance, that seems more like what you want. This does, of course, create at least one annoying edge case to work around:

>>> from django.utils.text import slugify
>>> slugify('C')
'c'

>>> slugify('C++')
'c'

>>> slugify('C#')
'c'

That's what I've don so far, but it looks like there is also some further validation needed beyond this. Lets carry on that discussion in #121 (comment)

---

So - we can re-write those tests to fail .. but then we need to decide what the expected behaviour is...and write some slugification code (or iri_to_uri code) accordingly...or live with 4 continuously failing tests (which seems silly). We could also omit them altogether until we decide.

My proposed solution is 49eca65 - we move those examples to a skipped test. This means:

• There is a record in the test suite of the problematic inputs when someone wants to look at it.
• The build passes.
• We don't have to deal with fixing it right now.
• We haven't written a test that passes because of behaviour you don't want.

btw, I'm slightly confused on the stòran-dàta example. Was that one just a typo in the fixture, or is it a "special" s that should actually slug to ss (like a german ß)? Should just one just be moved back to test_valid_slugs() as {"name": "stòran-dàta", "expected_slug": "stòran-dàta"}?

[BethanyG]

btw, I'm slightly confused on the stòran-dàta example. Was that one just a typo in the fixture, or is it a "special" s that should actually slug to ss (like a german ß)? Should just one just be moved back to test_valid_slugs() as {"name": "stòran-dàta", "expected_slug": "stòran-dàta"}?

Apologies, @chris48s - missed this in the whole comment shuffle. YES - AFAK it was a typo, (I don't think Scots Gaelic has an Eszett-type character..and Google appears to agree with me.) so let's just move {"name": "stòran-dàta", "expected_slug": "stòran-dàta"} back into test data.

[chris48s]

I've marked this PR as ready for review now. There are still quite a few unanswered questions about what should/shouldn't be allowed in a tag name/slug but I don't think we've reached a conclusion that results in clearly defined changes to this PR yet.

My suggestion would be we review/merge this as it stands now and revisit following further discussion in issues. It may be that loads of this stuff ends up needing to change soon, but if nothing else this PR forces us to nail down some decisions about stuff that was previously not very well defined :)

[lpatmo]

Feel free to merge this in, @chris48s! 🙏