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

3437 geo json api file download #3454

Merged
merged 7 commits into from
Jan 23, 2024
Merged

Conversation

JeremyFreiburger
Copy link
Collaborator

@JeremyFreiburger JeremyFreiburger commented Jan 4, 2024

…for API page

Resolves #3437

Creates optional parameter for /attributes and /attributesWithLabels API calls for geojson. Set inline parameter default to false and to true for maps on Project Sidewalk API webpage.

Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Your inlineCheck parameter isn't actually working! If I go to the URL http://localhost:9000/v2/access/attributes?inlineCheck=true, it still downloads a file instead of making it inline!

The other issue is that the URLs where you added the inlineCheck=true are actually the exact endpoints that we don't want to show inline! From the initial issue:

In Seattle, our dataset is now so large that the browser crashes when trying to render the full /attributesWithLabels endpoint in GeoJSON

And the URLs you added inlineCheck=true to are the ones for downloading the entire dataset. I'd like to just see you add those for the URLs where users click on the examples so that we can open up those small examples in a new tab inline!

app/controllers/ProjectSidewalkAPIController.scala Outdated Show resolved Hide resolved
@JeremyFreiburger
Copy link
Collaborator Author

Ready for review again! Files look good

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

The code doesn't seem to be compiling... First error I get is

[error] /opt/app/controllers/ProjectSidewalkAPIController.scala:190: reassignment to val
[error]       Future.successful(Ok.sendFile(content = attributesJsonFile, inline = inline = inline.getOrElse(false), onClose = () => attributesJsonFile.delete()))

I changed the inline = inline = inline.getOrElse(false) to just inline = inline.getOrElse(false). Then I got a new error:

[error] /opt/conf/routes:151: not enough arguments for method getAccessAttributesWithLabelsV2: (lat1: Option[Double], lng1: Option[Double], lat2: Option[Double], lng2: Option[Double], severity: Option[String], filetype: Option[String], inline: Option[Boolean])play.api.mvc.Action[play.api.mvc.AnyContent].
[error] Unspecified value parameter inline.

Is it possible that you didn't push your latest commit..?

@JeremyFreiburger
Copy link
Collaborator Author

Yes, it is very likely I forgot to add a file to the commit, I remember this exact error. I will fix this as soon as possible today.

@JeremyFreiburger
Copy link
Collaborator Author

Just committed changes, hopefully this time i've got it!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

A couple small things:

  1. In the examples on the API page, you added the inline parameter to the call that is for a CSV. It just gets ignored anyway, but it was added unnecessarily, so let's just remove it so that it isn't confusing.
  2. Let's also remove the inline parameter from what users see when they click on the examples on the API page. Let's have the actual link use the inline param, but just don't show it here:
    Screenshot from 2024-01-22 16-20-08

Otherwise, it looks good!

@JeremyFreiburger
Copy link
Collaborator Author

Just committed your last suggested changes.

Copy link
Member

@misaugstad misaugstad 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, thanks @JeremyFreiburger!

@misaugstad misaugstad merged commit f7c4bcd into develop Jan 23, 2024
@misaugstad misaugstad deleted the 3437-GeoJSON-API-file-download branch January 23, 2024 17:41
@misaugstad misaugstad mentioned this pull request Jan 31, 2024
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.

GeoJSON APIs should send a file instead of just the JSON
3 participants