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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add bulk upload of shapes #986

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

meagharty
Copy link
Collaborator

@meagharty meagharty commented Jun 12, 2024

Summary of changes

Asana Ticket: 馃徆 Implement Shuttle Route KML Upload

  • Adds /shapes_upload for uploading a file and extracting the LineString Placemarks
  • Uses the "name" attribute in the KML file for each shape
  • Adds coordinates to shape database from KML, the shape KML isn't valid without it anyway
  • Edit page allows for update of the "name" of the shape within Arrow (not the filename, not the file contents, not the coordinates)

Currently redirects to /shapes on success.
Next iteration can include "and Shape Selection" from the ticket: to add the selection of only certain shapes from the KML and allow modification of their names.

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

Base automatically changed from meag/add-shuttle-shape-page to master June 13, 2024 15:59
@meagharty meagharty force-pushed the meag/add-bulk-upload-shapes branch from 7400b34 to 3bdb23b Compare June 14, 2024 21:20
@@ -345,8 +345,9 @@ CREATE TABLE public.schema_migrations (
CREATE TABLE public.shapes (
id bigint NOT NULL,
name character varying(255),
inserted_at timestamp(0) without time zone NOT NULL,
updated_at timestamp(0) without time zone NOT NULL
inserted_at timestamp with time zone NOT NULL,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NOTE: I need to add a new migration for this change since the original scaffolding was already merged.

@meagharty meagharty requested review from a team and bfauble and removed request for a team June 14, 2024 21:25
@type t :: %__MODULE__{
id: id,
name: String.t(),
coordinates: list(String.t()),
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be saved to the db as well as the kml file uploaded to s3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The designs for bulk upload for shapes is why I chose to include this in the db after all. There's versatility in having both the S3 files and the db representation of the coordinates.

It makes sense to have the kml files in S3 in terms of using the format used by other map/transit partners like Remix and Google Map (and gtfs_creator itself).

Since the ticket spec ended up calling for a bulk upload, with shape selection, shape re-naming, and coordinate consolidation, it seemed like making the backend aware (and able to later show/present) that data was worthwhile.

I see the concerns around the duplication though. What do you think?
cc @nlwstein @Whoops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants