Geocoding on activist update #197

jefftienchow · 2023-05-02T06:54:54Z

Attempt at geocoding addresses with google API on activist update

Tested locally on a variety of addresses. Google Geocoding API has a bit undefined behavior when entering bogus addresses but seems to work well with real addresses.

Not sure if I should include package-lock.json changes in my commits?

jakehobbs · 2023-05-02T16:49:23Z

Thanks for this! I will review later today.

alexsapps · 2023-05-02T17:59:10Z

Me too, ty and will check it out! Le mar. 2 mai 2023, 09:49, jakehobbs ***@***.***> a écrit :

…

Thanks for this! I will review later today. — Reply to this email directly, view it on GitHub <#197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS6DL7KDYYOF5CEM33GVILXEE3J3ANCNFSM6AAAAAAXSUA7TI> . You are receiving this because your review was requested.Message ID: ***@***.***>

alexsapps · 2023-05-02T20:21:34Z

Not sure if I should include package-lock.json changes in my commits?

Yes, maintaining package-lock.json changes in version control ensures reproducible builds. package.json only specifies a range of versions allowed to build the project, not exact versions.

alexsapps · 2023-05-02T21:11:03Z

model/activist.go

@@ -1131,12 +1146,14 @@ func UpdateActivistData(db *sqlx.DB, activist ActivistExtra, userEmail string) (
 	// TODO: make this work for other chapters too, maybe based on Chapters table mailing list info?
 	// or maybe just passing chapter Name or ID to the signup service?
 	if origActivist.ChapterID == 47 {
-		activistInfoChanged := activist.Name != origActivist.Name ||
+		activistAddressChanged := activist.StreetAddress != origActivist.StreetAddress ||


looks like StreetAddress is not used by the mailing list, so this should not trigger a call to update the mailing list.

alexsapps · 2023-05-02T21:15:02Z

model/activist.go

+			activist.City != origActivist.City ||
+			activist.State != origActivist.State
+		activistInfoChanged := activistAddressChanged ||
+			activist.Location != origActivist.Location ||


should location be considered in activistAddressChanged instead? it look like it contains the Zip which is used by the mailing list.

but then Zip is not used by the geo API, so maybe this logic should not be shared between geo api logic and mailing list logic, in which case using names like "isMailingListInfoChanged" and "isGeoInfoChanged" would better convey the intent.

Yeah I don't think zip is used for geocoding so I'll do the latter

alexsapps · 2023-05-02T21:19:19Z

model/activist.go

@@ -1155,7 +1172,23 @@ func UpdateActivistData(db *sqlx.DB, activist ActivistExtra, userEmail string) (
 				fmt.Println("ERROR updating activist on mailing list:", err.Error())
 			}
 		}
-		fmt.Println("Updated: ", activist.Location, activist.City, activist.State)
+		if activistAddressChanged && activist.StreetAddress != "" && activist.City != "" && activist.State != "" {


please split the body of this 'if' into a separate function (e.g. called GetCoordinatesFromAddress) in a separate file (single responsibility principle). i suggest the function take a single string parameter for the address and return the location and any error.

additionally, but optionally, the 'if' itself could be its own function called updateActivistCoordinates which would allow early returns in case of error rather than nesting the happy path inside of an if block.

alexsapps · 2023-05-02T21:38:12Z

model/activist.go

+			request := "https://maps.googleapis.com/maps/api/geocode/json?address=" + full_address + "&key=" + config.GooglePlacesAPIKey
+			resp, err := http.Get(request)
+			if err != nil {
+				fmt.Println("Error geocoding activist location", err)


if there is an error might be better to break out of this if somehow (or return early if you split this into another function)

On this note, I think we'd probably prefer to silently fail (& log it) but still go through w/ saving the activist in the db if the gecocoding service fails for w/e reason.

I feel it's pretty important that activist data is still able to be updated even if the geocoding service is failing.

Thinking about this further... maybe it would be ideal to just fire off a goroutine here that handles the things that need to happen in the background, such as updating mailing list and coords? B/c we don't really want the user to have to wait around for those things to happen before the activist is saved in the db. (This was a bit of a flaw in the original logic for updating the mailing list.)

probably the coords should still be looked synchronously because the result is used in the database update? if so, could the signup be made async in a separate PR so Jeff's change isn't held up?

just wondering, is the async idea to make it faster when an admin of ADB clicks save to update a user? did you notice the adb signup was slow?

maybe whatever is simplest/easiest is best for this probably rarely used feature. but some possible improvements mainly as thought experiments:

it would be nice to know if the signup operations fails, maybe by an email notification to our team, perhaps directly or by monitoring error logs.

it would be nice to ensure async operations complete even if the web server container is requested to shut down or even if an error in the main function causes it to return and shut down. perhaps there would be a work queue that the signup gets added to, and the main function can wait for the queue on the last line before exiting.

Fair points. Yeah, I was thinking the main reason would be so it saves faster, but I that might be a bit of an over optimization for now. I def won't block on it.

We have log drains in AWS and can set up Cloudwatch alerts for whatever.

The signup service has its own queue. That being said, I do think the background jobs that run in ADB could use a huge overhaul at some point if we have bandwidth. Right now they only run within a single instance of the app (via an environment variable that enables them). It would be great to implement a more robust job system.

Maybe walking through how the signup service works at the next meeting would be useful. A lot of stuff feeds in and out of it, and would be good for others to know how it works.

alexsapps · 2023-05-02T21:40:36Z

model/activist.go

@@ -441,6 +444,18 @@ type GetActivistOptions struct {
 	UpcomingFollowupsOnly bool   `json:"upcoming_followups_only"`
 }

+type GeocodeResponse struct {


please a comment indicating that this structure corresponds to Google's geo coding API with a link to that API's documentation

jakehobbs

I took a brief look over this and p much agree w/ what Alex pointed out. Left one comment on one of his comments, but I don't have anything else to add other than that.

alexsapps

Thanks for the updates! Just got a few more suggestions.

alexsapps · 2023-06-17T21:38:17Z

model/activist.go

+		geoInfoChanged := activist.City != origActivist.City ||
+			activist.State != origActivist.State ||
+			activist.StreetAddress != origActivist.StreetAddress
+		mailingListInfoChanged := activist.Name != origActivist.Name ||
 			activist.Email != origActivist.Email ||
 			activist.Phone != origActivist.Phone ||
 			activist.Location != origActivist.Location ||


would be nice to move this down a couple lines underneath the state != state check to keep things in the same order that they are used later on, to more easily verify correctness at a glance.

alexsapps · 2023-06-17T21:41:19Z

model/activist.go

@@ -1131,14 +1145,17 @@ func UpdateActivistData(db *sqlx.DB, activist ActivistExtra, userEmail string) (
 	// TODO: make this work for other chapters too, maybe based on Chapters table mailing list info?
 	// or maybe just passing chapter Name or ID to the signup service?
 	if origActivist.ChapterID == 47 {
-		activistInfoChanged := activist.Name != origActivist.Name ||
+		geoInfoChanged := activist.City != origActivist.City ||


please declare this where it is used, just above if geoInfoChanged && ...

alexsapps · 2023-06-17T21:45:08Z

model/geo_helpers.go

+		fmt.Printf("No geocoding results found for address %v. Not updating Lat and Lng", full_address)
+		return nil
+	} else {
+		return &Location{Lat: geocode_response.Results[0].Geometry.Location.Lat, Lng: geocode_response.Results[0].Geometry.Location.Lng}


If you wouldn't mind checking if Results[0] and Geometry and Location are non-nil, I think that could prevent a segfault. Even if the API always returns non-nil now, I worry this could change and we wouldn't want the server to crash - just log an error and carry on.

I'm a beginner at Go, but from my understanding this wouldn't be possible? Constructing a GeocodeResponse from json that doesn't contain these fields would just result in the default values being included for lat and lng, which would be 0 and 0

https://stackoverflow.com/questions/50597713/check-if-struct-field-is-empty

Oh got it. My bad :) I'm a Go beginner too. So a struct inside struct has a default value of the 0 value of the inner struct, as opposed to a struct pointer inside a struct which would be nil.

alexsapps

Looks good to me. Let's see if Jake has any suggestions since it's the first PR in this codebase for either of us, I think.

Can you select "squash" merge strategy rather than "merge"? Otherwise it will clutter the main branch log since the many commits in this branch are all for one logical change. If it lets you type a commit message, perhaps use something slightly more precise like "Geocode activist address on update"

alexsapps · 2023-06-18T03:44:38Z

model/geo_helpers.go

+	resp, err := http.Get(request)
+	if err != nil {
+		fmt.Println("Error geocoding activist location", err)
+		return nil


optional: return the error (and print it in the caller) to allow callers to handle errors as needed, thereby making this function more reusable

yeah i agree returning the error would be good as best practice, then the caller could just choose to ignore it if you don't want to actually handle it.

alexsapps · 2023-06-18T03:49:49Z

model/geo_helpers.go

+		fmt.Println("Error geocoding activist location", err)
+		return nil
+	}
+	defer resp.Body.Close()


consider adding a blank line between the HTTP fetching logic and GeocodeResponse parsing logic.

alexsapps · 2023-06-18T03:50:54Z

model/geo_helpers.go

+		fmt.Printf("No geocoding results found for address %v. Not updating Lat and Lng", full_address)
+		return nil
+	} else {
+		return &Location{Lat: geocode_response.Results[0].Geometry.Location.Lat, Lng: geocode_response.Results[0].Geometry.Location.Lng}


Oh got it. My bad :) I'm a Go beginner too. So a struct inside struct has a default value of the 0 value of the inner struct, as opposed to a struct pointer inside a struct which would be nil.

jakehobbs · 2023-06-20T03:22:55Z

model/geo_helpers.go

+	resp, err := http.Get(request)
+	if err != nil {
+		fmt.Println("Error geocoding activist location", err)
+		return nil


yeah i agree returning the error would be good as best practice, then the caller could just choose to ignore it if you don't want to actually handle it.

jefftienchow added 4 commits May 1, 2023 23:48

Added geocoding for updating activists

9f9372a

maybe this time i will correctly use git

72b25dd

put back space

3a6338f

do we have a linter

737a109

jefftienchow changed the title ~~Geocoding~~ Geocoding on activist update May 2, 2023

jefftienchow requested review from alexsapps and jakehobbs May 2, 2023 06:55

jefftienchow added 3 commits May 1, 2023 23:57

oh i think i found this linter

969a721

used wrong print :o

85b6fdb

ok now i think it actually works

288a973

alexsapps requested changes May 2, 2023

View reviewed changes

jakehobbs reviewed May 2, 2023

View reviewed changes

jefftienchow added 3 commits June 15, 2023 21:42

tried to address comments

176c93b

cleaned up

6174c63

make fmt

1e44747

jefftienchow requested review from alexsapps and jakehobbs June 16, 2023 23:07

alexsapps requested changes Jun 17, 2023

View reviewed changes

moved some code around

16d5960

jefftienchow requested a review from alexsapps June 18, 2023 03:07

make fmt

74643bc

alexsapps approved these changes Jun 18, 2023

View reviewed changes

jakehobbs approved these changes Jun 20, 2023

View reviewed changes

jefftienchow merged commit 07a0717 into master Jun 25, 2023

jefftienchow deleted the geocoding branch June 25, 2023 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geocoding on activist update #197

Geocoding on activist update #197

jefftienchow commented May 2, 2023 •

edited

Loading

jakehobbs commented May 2, 2023

alexsapps commented May 2, 2023 via email

alexsapps commented May 2, 2023

alexsapps May 2, 2023

alexsapps May 2, 2023

jefftienchow May 2, 2023

alexsapps May 2, 2023

alexsapps May 2, 2023

jakehobbs May 2, 2023

jakehobbs May 3, 2023

alexsapps May 6, 2023

jakehobbs May 6, 2023

jakehobbs May 6, 2023

alexsapps May 2, 2023

jakehobbs left a comment

alexsapps left a comment

alexsapps Jun 17, 2023

alexsapps Jun 17, 2023

alexsapps Jun 17, 2023

jefftienchow Jun 18, 2023

jefftienchow Jun 18, 2023

alexsapps Jun 18, 2023

alexsapps left a comment

alexsapps Jun 18, 2023

jakehobbs Jun 20, 2023

alexsapps Jun 18, 2023

alexsapps Jun 18, 2023

jakehobbs Jun 20, 2023

Geocoding on activist update #197

Geocoding on activist update #197

Conversation

jefftienchow commented May 2, 2023 • edited Loading

jakehobbs commented May 2, 2023

alexsapps commented May 2, 2023 via email

alexsapps commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakehobbs left a comment

Choose a reason for hiding this comment

alexsapps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsapps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefftienchow commented May 2, 2023 •

edited

Loading