codebuddies / backend

CodeBuddies back-end
https://codebuddies.org
GNU General Public License v3.0
20 stars 25 forks source link

Issue 124 (as well as 123 & 127) - Tagging Tests, Fixes & Touchup #141

Closed BethanyG closed 4 years ago

BethanyG commented 4 years ago

Issues

Closes #123 - Mangled Abugida slugs Closes #124- Emoji and Pitctograms slugging to blank spaces. Closes #127- Tag Validation


Please see the following two github threads for some details on how taggit handles situations where a tag.name and a tag.slug match, and the app is set to TAGGIT_CASE_INSENSITIVE=True:

  1. https://github.com/jazzband/django-taggit/issues/448#issuecomment-414474054
  2. https://github.com/wagtail/wagtail/issues/4786#issuecomment-426436030

Changes

Decided to remove the unique constraint from the slug field of CustomTag. Now, if two tags slug to the same value but their name/slug combo is unique, the tag name is saved to the DB, and the slug value is repeated (as in the slug is no longer unique in its column). What this means is that queries for tag slugs will return more than one name value...so we will have to account for that in search.

Also decided to bypass Djangos built-in slugify for Abugida scripts. Now, if an Abugida is detected, it will go through a different slugification process that does not remove the combination and diacritical marks.

Finally, wrote a regex that rejects any emoji, pictograms, letter-like symbols, etc. Any of these will now throw a serializer.ValidationError - as will items that are not of type str() and items that are not in a list.

Tests are ongoing, and I probably missed some scenarios (feedback and help always welcome!). Some of them were more involved than others, since validation code is only triggered through the serializers, and does not get called by invoking tag.save(). More duplicate and unique_together tag tests are probably needed as well.

codecov[bot] commented 4 years ago

Codecov Report

Merging #141 into master will increase coverage by 0.82%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
+ Coverage   82.88%   83.70%   +0.82%     
==========================================
  Files          29       29              
  Lines         479      491      +12     
==========================================
+ Hits          397      411      +14     
+ Misses         82       80       -2     
Impacted Files Coverage Δ
project/tagging/models.py 100.00% <100.00%> (ø)
project/tagging/serializers.py 97.91% <100.00%> (+6.25%) :arrow_up:

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 30a387c...4c8eb74. Read the comment docs.

lpatmo commented 4 years ago

This looks great! I was having a bit of trouble getting the branch to run without displaying an nginx error, though -- it looked like it was caused by this:

ModuleNotFoundError: No module named 'regex'

So I ran docker-compose run --rm app pip install -r requirements/base.txt and I thought I saw regex was installed, but am still seeing this:

image

Wonder if there's another pip install command I'm missing 🤔

lpatmo commented 4 years ago

Thanks so much for knocking out all those issues, btw :) Once I get my branch to run I'll do a bit more Postman testing!

BethanyG commented 4 years ago

So...I am still not totally clear on all the intermediary build steps, but here are three strategies that worked for me when I needed to get the regex library + associated changes to the DB installed & working on my docker/system combo:

  1. During dev, in order to try out the library before making changes to requirements/base.txt:

    • while the app container was running, docker exec -it app /bin/bash (to log into the app container with an interactive bash shell)
    • At the /opt/codebuddies directory (you should already be there after running the above command), run pip3 install <insert name of library here>
  2. During dev, after the changes to requirements/base.txt, to test that things were being installed and picked up:

    • Stop all containers via CTRL-C. Follow up with docker-compose down
    • Re-build all containers via docker-compose up -d --build
  3. During dev, after changing the tagging model (this supposes that the migration files have been made) and during the writing of validation code and tests:

    • Stop all containers via CTRL-C. Follow up with docker-compose down
    • Clean up containers via docker-compose rm. Use this as an opportunity to follow up with any docker prune commands needed to clean up hanging and orphan containers and images.
    • Further clean up data directory by running docker volume rm backend_db_data (this is so you don't have bogus or confusing data in your DB)
    • Re-build code from scratch using docker-compose up -d --build. Then use docker-compose up.
    • Make new super user by logging into the running app container via docker exec -it app /bin/bash and then running python manage.py createsuperuser. This can also be done via docker-compose. Both work.
    • Proceed to test, knowing that the DB is entirely empty.

...You can probably get away with using strategy 2, but you won't really be able to cleanly test the deduping and unique together code without nuking all your data, which would use most of strategy 3.

lpatmo commented 4 years ago

Gahhh, I can't believe I forgot about docker-compose up -d --build -- that did the trick. Thanks so much for all those debugging options, Bethany! I'll create a quick PR to make sure docker-compose up -d --build is in the contributing.md / README.md so that future me remembers I need to rebuild to install new packages 😅