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

Add Support for Uploading Files #175

Merged
merged 22 commits into from
Nov 7, 2022
Merged

Add Support for Uploading Files #175

merged 22 commits into from
Nov 7, 2022

Conversation

rajadain
Copy link
Contributor

@rajadain rajadain commented Nov 2, 2022

Overview

Adds back-end support for uploading and saving files.

All uploaded files are saved to S3. It was easier to do this than to have development save locally. The development bucket will be shared between all developers and test environments.

Uploaded files are saved to local disk in development, and in S3 buckets in staging and production.

Supports multiple reference image uploads, and a single shape file upload. Supports uploading a zipped shapefile, as well as a geojson file.

Reference images are reported as signed S3 URLs, so the front-end can access them despite the buckets themselves being private. These URLs are valid for about an hour.

The front-end is updated to upload the files correctly, and also to load the shapes and reference images from the back-end. An edge case with L.DistortableImage was handled, without which navigating away from the map was not possible.

Also refactors the tests, updates the reference image tests to upload files, and adds tests for shapefile and geojson uploads.

More UI polish will come with #158.

Closes #81
Closes #158

Demo

2022-11-02.15.38.36.mp4

Testing Instructions

  • Check out this PR, run update and resetdb and server
  • Log in as a contributor, go through the Welcome modals
  • Select a number of files for uploading
    • ℹ️ Use the shapefile or geojson added for testing if you don't have any locally
    • ℹ️ Use any images available for testing reference images
    • Ensure that the uploads are handled correctly
    • Ensure that the reference images are rendered correctly
    • Ensure that the shapefile / geojson is ingested correctly
  • Navigate around the app
    • Make sure there's no other crashes

Checklist

  • fixup! commits have been squashed
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
  • README.md updated if necessary to reflect the changes
  • CI passes after rebase

Copy link
Contributor

@mstone121 mstone121 left a comment

Choose a reason for hiding this comment

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

I'm seeing this error when I try to upload:
Screen Shot 2022-11-03 at 9 35 02 AM
Do I need to do some AWS config? If so, maybe we need to add some readme instructions.

Also, the app appears to be crashing after ./scripts/resetdb and visiting /draw/1 with 'Other Utility'.

@rajadain
Copy link
Contributor Author

rajadain commented Nov 3, 2022

Removed S3 requirement for development in 12d6847

Added a few commits d7a8e29, 23c8599, 63da524, 5af0f1c, a6753dc to complete the file upload experience in the front-end. This now also closes #158.

Also, the app appears to be crashing after ./scripts/resetdb and visiting /draw/1 with 'Other Utility'.

I'm not able to reproduce this, could have been related to the AWS_PROFILE issue, which should no longer be the case. Let me know if this is still happening.

This is ready for another look.

@rajadain rajadain assigned mstone121 and unassigned mstone121 Nov 3, 2022
This terraform configuration will add a data bucket for
staging and production environments. A data bucket for
development was added manually. It will be used by all
developers.
Add a file field to the reference image model, serializer,
views. Saves the file to S3, under a folder structure
grouping it under the boundary.

Splits the serializer into two: one for creation that has
the file details, another for updating which does not.

Also makes the filename a required field.
Send data using FormData rather than JSON to include files.
Read the files from the response and add them to the map.
The previous reference image tests did not send a file,
which is now required by the API. This adds the file to
the test payloads so that they work correctly.

Also refactors the tests into separate files under the
tests/ directory, as has been done for models/, views/,
and serializers/.
This library will be used to read and convert shapefiles
in the back-end.

lib-gdaldev is required to build fiona.
If the payload's shape is a zipped shapefile or a geojson,
they will be parsed as shapes and added to the database. In
the case of geojson, the file is parsed and ingested directly.
In the case of zipped shapefiles, we save them to a temporary
directory, then load them via fiona and extract the shape from
within. fiona cannot read files from memory, and the uploaded
files could potentially be really heavy, so doing so helps
manage the load.

These are the only two formats we are supporting at the
moment, more may be added in the future.

This assumes a single Polygon input. Support for MultiPolygons
will be added in #127.
In addition to ingesting the shape, we would also like to
save the originally uploaded file for reference. This adds
the capability to do so.
In some cases, shapes can render before reference images, and
when we try to fit the map bounds to the shape, since the
reference images are not yet loaded there is a crash.

This try block ensures that the crash does not propagate, and
keeps retrying until the map fits once, and then no more.
For some reason, removing reference image layers after they have
been loaded using actual files was causing a crash where some
variables would be undefined in _unbindListeners. This patch
adds optional chaining to handle those cases more gracefully.
@rajadain
Copy link
Contributor Author

rajadain commented Nov 4, 2022

Rebased on latest develop to resolve merge conflicts

@rajadain
Copy link
Contributor Author

rajadain commented Nov 4, 2022

Ah I'm seeing the draw/1 bug now, will investigate.

@rajadain
Copy link
Contributor Author

rajadain commented Nov 4, 2022

Fixed in 069e469

Copy link
Contributor

@mstone121 mstone121 left a comment

Choose a reason for hiding this comment

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

Requesting a few changes. In addition to the code comments:

  • I'm finding that uploading a shape from the welcome screen won't override an existing shape.
  • Would you mind updating the PR text to reflect that local file storage is being used in development?

src/app/src/api/boundaries.js Show resolved Hide resolved
src/app/src/components/DrawTools/EditPolygonModal.js Outdated Show resolved Hide resolved
src/app/src/components/ModalSections/FileUpload.js Outdated Show resolved Hide resolved
src/django/api/tests/reference_image.py Show resolved Hide resolved
src/django/iow/settings.py Show resolved Hide resolved
@rajadain
Copy link
Contributor Author

rajadain commented Nov 4, 2022

I'm finding that uploading a shape from the welcome screen won't override an existing shape.

This is a good catch, although the solution may be almost out of scope and may require another card to solve properly. In the interest of budget I've included two three commits: a37fb01, cc79043, and 99493d4 which make it so that the user is not taken to the /welcome page unless there are no existing boundaries for the selected utility.

It doesn't make sense for a contributor to go through the /welcome page if they already have a boundary in the system.

I started writing a card for this, but realized the code change would not be that complex, so decided to include them here. I can also pull those into a separate PR if they feel too unrelated.

@rajadain
Copy link
Contributor Author

rajadain commented Nov 4, 2022

This is ready for another look.

Copy link
Contributor

@mstone121 mstone121 left a comment

Choose a reason for hiding this comment

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

Looks good!

@mstone121
Copy link
Contributor

This is a good catch, although the solution may be almost out of scope and may require another card to solve properly.

Just to clarify, is the solution you're referring to not what you implemented in the last three commits?

@rajadain
Copy link
Contributor Author

rajadain commented Nov 7, 2022

Just to clarify, is the solution you're referring to not what you implemented in the last three commits?

I think these three commits are good for now, but there may be aspects of this workflow that need further polish which we may discover once these are in. Will make a follow up card when those issues are discovered.

This allows us to have different file pickers in different
parts of the app. The one in the Welcome page accepts both
shapes and images. The one in the Sidebar only accepts
images. This parameterization allows us to do that.
In addition to the regular geojson input, the /shape/ endpoint
now also accepts a file. This file will be ingested into a
shape. If successful, the newly ingested shape is sent back
as a response.
For better control of when the map zoom is allowed to happen,
we move this flag to redux.
In the edit polygon modal, we now only allow the replacement
of the existing polygon with a new file. The polygon naming
functionality is disabled for now, pending #91.

Choosing a new file fires the Replace Polygon mutation, which
first removes the existing polygon, then sets the map ready
for re-zooming, then waits for the POST query to succeed, and
finally sets the new polygon. If the POST query fails, the
previous polygon is restored.
Requiring S3 access for local development was not ideal, as we
want people to be able to run and contribute to this as easily
as possible.

This adds MEDIA configuration for uploads, which will be used
locally, and will use S3 in staging.
This test image did not have a corresponding file and was
causing crashes.
The prime case where we don't want to take the user to the
submissions list is when they are a new contributor, adding
a boundary for a utility that doesn't have one already. In
all other cases, we want the user to see the submissions
list.

To make that determination, we need to first fetch the list
of boundaries for the contributor's selected utility, which
happens in the submissions list page. Thus, we redirect all
users there, and make the decision whether to redirect to
/welcome there.
Previously, we would redirect all contributors to the /welcome
page. This led to them being welcomed, even if they had already
started a boundary, or submitted one, both cases in which the
/submissions list is a better starting view for them.

The only case when /welcome makes sense is when there are no
boundaries for their selected utility.

Here, we fetch the boundaries for their selected utility, and
iff they are coming from the /login page, we redirect them to
/welcome. Otherwise, they always see the submissions list.
To prevent users from adding multiple maps for the same
utility, we disable the button, enabling it only when there
are no currently active boundaries in review.
@rajadain
Copy link
Contributor Author

rajadain commented Nov 7, 2022

Thanks for the thorough review! Will merge once green.

@rajadain rajadain merged commit de72855 into develop Nov 7, 2022
@rajadain rajadain deleted the test/tt/file-uploads branch November 7, 2022 15:24
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.

Wire up front-end to upload files Add file fields to Reference_Image and Submission
2 participants