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

Update GeoJSON endpoint to filter by instances with geoms #2335

Merged
merged 8 commits into from
Nov 24, 2022

Conversation

KipSigei
Copy link
Contributor

@KipSigei KipSigei commented Nov 16, 2022

Changes / Features implemented

  • Update geojson enpoint to return 404 if all instances/submissions have empty geoms e.g

before

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "geometry": {
        "type": "GeometryCollection",
        "geometries": []
      },
      "properties": {
        "id": 111,
        "xform": 20
      }
    }
  ]
}

now

{
  "detail": "Not found."
}

Steps taken to verify this change does what is intended

  • Tests

Side effects of implementing this change

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Closes #

@KipSigei KipSigei force-pushed the filter-by-instances-with-geoms branch from b764799 to b321f11 Compare November 16, 2022 08:21
DavisRayM
DavisRayM previously approved these changes Nov 16, 2022
@KipSigei KipSigei force-pushed the filter-by-instances-with-geoms branch 3 times, most recently from 4925713 to 6d62e10 Compare November 17, 2022 17:26
@KipSigei KipSigei force-pushed the filter-by-instances-with-geoms branch from 6d62e10 to 9d36be5 Compare November 21, 2022 05:59
).count()
< 1
):
self.object.xform.instances_with_geopoints = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this implemented on the model level instead of the viewset ? Guessing this field is updated/set on the model level ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DavisRayM Moved the logic to the instance model post save signal instead to handle the update after a submission is soft deleted. Note that with soft deletion the post_save signal is triggered instead of post_delete

Comment on lines 802 to 811
# update xform if no instance has geoms
if (
instance.xform.instances.filter(
deleted_at__isnull=True, geom=None
).count()
< 1
):
instance.xform.instances_with_geopoints = False
instance.xform.save()

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have this happen in update_xform_submission_count and let's try to avoid a count on the XForm. If possible let's use the Forms submission_count property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed via 4008f69

Signed-off-by: Kipchirchir Sigei <[email protected]>
@KipSigei KipSigei force-pushed the filter-by-instances-with-geoms branch from 2ae144d to 4008f69 Compare November 22, 2022 08:52
# update xform if no instance has geoms
if (
instance.xform.instances.filter(
deleted_at__isnull=True, geom=None
Copy link
Contributor

@DavisRayM DavisRayM Nov 22, 2022

Choose a reason for hiding this comment

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

This query won't do what is expected; It's counts instances that are not deleted with no geoms... Let's use a query like xform.instances.filter(deleted_at__isnull=True).exclude(geom=None).count() < 1

Copy link
Contributor

Choose a reason for hiding this comment

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

xform.instances.filter(~Q(geom=None), deleted_at__isnull=True).count() < 1

@KipSigei KipSigei merged commit 0a843cb into main Nov 24, 2022
@KipSigei KipSigei deleted the filter-by-instances-with-geoms branch November 24, 2022 08:35
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