completed config table for each city's database. #3281
Conversation
Merge remote-tracking branch 'origin/develop' into develop
…Webpage into 2772-mark-id merging develop
| @@ -867,4 +867,4 @@ class ProjectSidewalkAPIController @Inject()(implicit val env: Environment[User, | |||
| Future.successful(Ok(APIFormats.projectSidewalkStatsToJson(LabelTable.getOverallStatsForAPI(filterLowQuality)))) | |||
| } | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Looks like by default it's also removing new lines at the ends of files, which has happened to a bunch of files in the PR, which is the opposite of what's in the style guide! So definitely make sure to add the newlines back to the files.
There was a problem hiding this comment.
At least for this file compared to the one in the develop branch, they appear the same to me. Is there something I am not seeing for this file?
There was a problem hiding this comment.
Not sure I totally understand. The only info I'm working off of is what I see in the "Files changed" tab. If you want it to be identical to develop, you could legit just download the file from Github and paste it in there 🤷
| val regionLat1: Double = ConfigTable.getRegionLatOne | ||
| val regionLng1: Double = ConfigTable.getRegionLngOne | ||
| val regionLat2: Double = ConfigTable.getRegionLatTwo | ||
| val regionLng2: Double = ConfigTable.getRegionLngTwo | ||
|
|
||
| Future.successful(Ok(Json.obj( |
There was a problem hiding this comment.
Building on the comment above, you can add a toJSON function to the case class that will return a JSON object that looks like the ones below so that you can just call that function instead of building the JSON object for each of them.
| import play.api.Play.current | ||
| import scala.slick.lifted.ForeignKeyQuery | ||
|
|
||
| case class ConfigApi(apiAttributeCenterLat: Double, apiAttributeCenterLng: Double, |
There was a problem hiding this comment.
Since it's useful to add a MapParams class to simplify other code, it might make sense to replace this big ConfigAPI class with that one?
There was a problem hiding this comment.
Kinda agree? If you mean just re-naming ConfigAPI to MapParams then that works! However, adding additional parameters would be interesting since ConfigAPI already has 21 parameters and if I recall correctly there is a 22 parameter limit.
There was a problem hiding this comment.
I'm suggesting removing the ConfigApi class, and instead adding in a MapParams class that has only 7 (I think it was 7?) fields, but the Config class would contain 3-4 of them instead of just the one ConfigAPI class.
There was a problem hiding this comment.
It'd be something like
case class Config(
openStatus: String, mapathonEventLink: Option[String], overallMapParams: MapParams,
tutorialStreetEdgeID: Int, offsetHours: Int, excludedTags: String,
apiAttributeParams: MapParams, apiStreetParams: MapParams, apiRegionParams: MapParams
)
There was a problem hiding this comment.
Sounds good to me, will be pinging you on Slack about this later then :)
…Webpage into 2772-mark-id g the commit.
|
@misaugstad review this when you have a chance! Will make a ticket to make the code cleaner for a future task, but would like to have this ready to go. |
|
Something is broken with Github so I'm going to close this PR and try to make a new one... |
Resolves #2772
Implemented a table that holds all of the relevant columns for a respective city's configuration such as latitude, offset hour, tutorial street edge id to easily update them. Had to include some funny SQL in 194.sql to have config be a one row table.
NOTE: Before approving, there is one comment with "- Dylan" to confirm if this should use ConfigTable or not.
Testing instructions
Things to check before submitting the PR