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

WIP: [8472] Migrate project point to GeoDjango and add a new serializer mixin to convert between gis and geojson #1765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Jan 28, 2025

This is a first attempt to move geographical locations from being stored as plain json/geojson to GeoDjango. This will allow us to filter for locations on a database level.

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@m4ra
Copy link
Contributor

m4ra commented Feb 10, 2025

@goapunk what's your concern at this point?

@goapunk
Copy link
Contributor Author

goapunk commented Feb 11, 2025

@goapunk what's your concern at this point?

@m4ra just that it's a rather big change. And we need to keep in mind this only migrates the points for projects. Points for ideas/proposals and other geometries (e.g. polygon) will stay as json fields.

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this surgical operation :)
It needs a good docs, with payload example and an explanation on why we keep both paradigms, and maybe in the future all map based modules should migrate to django gis

adhocracy4/maps/mixins.py Outdated Show resolved Hide resolved
adhocracy4/maps/mixins.py Outdated Show resolved Hide resolved
adhocracy4/maps/validators.py Outdated Show resolved Hide resolved
name="point",
),
migrations.AddField(
model_name="project",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once data has migrated wth migrate_project_point_field and old point filed is removed you can just rename the field from geos_point back to point, but safer in a new migration. And no need for migrate_project_geos_point_field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly the RenameField migration doesn't work. It just gives a cryptic error. I assume it's because the old field is a normal sqlite field and the new one is a spatialite gis field, but that's more of a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look again just in case I missed something

+ ": "
+ str(geojson_point)
)
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this log warning be showing in sentry? otherwise i am afraid we will forget to check when deloying to prod/stage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it won't show up in sentry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd locally test the migration with the prod db though

adhocracy4/projects/models.py Show resolved Hide resolved
tests/maps/test_map_mixins.py Show resolved Hide resolved
@m4ra
Copy link
Contributor

m4ra commented Feb 11, 2025

@partizipation this PR, introducing django.gis for storing geopoints in the database

points as GeoDjango to the database for better filtering capabilites and
serializes them as geojson features.

- add PointInPolygon validator which validates that a given point is in
  the provided polygon.
@goapunk goapunk force-pushed the jd-2025-01-custom-geometry-field branch from f89db5d to 5cbff12 Compare February 12, 2025 12:21
@goapunk goapunk requested a review from m4ra February 12, 2025 12:21
@goapunk
Copy link
Contributor Author

goapunk commented Feb 12, 2025

@m4ra landing this means we have to enable postgis on all our platforms using a4, this also means we need changes in the admin repo

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.

2 participants