Filter locations in API by location_on_map field

[jonbulz]

Short description

Change location_not_on_map attribute of POIs to location_on_map and use it in api endpoints.

Resolved issues

Fixes: #1038

[timobrembeck]

Awesome, congratz to your first PR 🎉 🚀 😍

In general, this looks very good and does exactly what we want. 👍
I have a few ideas for improvement:

1. We could filter the locations in the database query instead of the python loop - see my comment below
2. Changes to the database structure require migration files to ensure that all existing databases can adapt to the new structure. In your local repository clone, there should appear a new file in the integreat_cms/cms/migrations/ directory which you need to add to your commit as well. If no such file was generated automatically, please execute the dev tool ./dev-tools/migrate.sh. See the Django documentation for more information.

[codeclimate]

Code Climate has analyzed commit 7ab3c0e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 66.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 73.8% (-0.1% change).

View more on Code Climate.

[jonbulz]

Actually I think I messed around a little too much with the migration files. I wasn't sure what they were doing at first, so I did a bit of unnecessary back and forth editing. I tried to get things as clean as possible, but I always ended up creating at least 2 migration files. Is that right?

[timobrembeck]

Thanks for your changes! 💪

Apart from my comment below, we also have to fix the CircleCI build for this PR:

At the moment, there are two problems:

1. The black code style is not applied - you can do this with the dev tool ./dev-tools/black.sh - in the future, I recommend using our pre-commit hooks for this (which can be installed via pipenv run pre-commit install), but if you face any issues with them, you might as well execute the dev tool manually before you commit.
2. The pylint code linter finds a few problems - a missing import and two missing docstrings. The docstring should be fixed with my suggestion, and for the import you need to add the line:

   from distutils.util import strtobool
   

   to the top of the locations api file as stated in my previous comment.

[timobrembeck]

Thanks!
I have one small suggestion left, and then this PR is almost ready to be merged 🎉
After that, please resolve the conflicts with the base branch by executing

git checkout develop
git pull
git checkout feature/location-on-map
git rebase develop
./dev-tools/resolve_translation_conflicts.sh
git rebase --continue
git push --force-with-lease

Then, after that it would be cool if you could clean up the git history a bit and combined your commits into one:

git rebase --interactive $(git merge-base --fork-point origin/develop $(git rev-parse --abbrev-ref HEAD))
# in the editor, replace "pick" by "fixup" in every line except the first one
git rebase --continue
git push --force-with-lease

[timobrembeck]

Awesome, thanks a lot! 💪

Oh sorry, and one thing I forgot:
Could you add exactly one line at the bottom of the "UNRELEASED" of our CHANGELOG with a note of this change? Thanks!
(If you don't want to repeat the git-clean-process, you can just use git commit --no-edit --amend together with git push --force-with-lease)

[jonbulz]

okay, I think I'm done :)

[timobrembeck]

Thanks! Just a tiny little detail, but I think that's it 🙂

[timobrembeck]

Damn, I just wanted to approve this PR, when I noticed that I used the old field recentlly when changing the POI coordinates...

Could you change this line

integreat-cms/integreat_cms/cms/forms/pois/poi_form.py

Line 99 in 62d6128

if not cleaned_data.get("location_not_on_map"):

to:

 if cleaned_data.get("location_on_map"): 

?
Sorry about all the small incremental change requests 😒

[jonbulz]

You're good, I didn't notice either. :D

[timobrembeck]

Amazing, I think that's it 🎉 🥂
Now press that merge button before I find something else 😆

[jonbulz]

🍾