Created attachment 239711 [details] [review]
lib: Add error code for internal server errors

Created attachment 239712 [details] [review]
lib: geocode_ipclient_search* should return location

This API will be a lot more useful if we provide a parsed GeocodeLocation
object to the caller rather than just the raw JSON string we get as
response from server.

Review of attachment 239711 [details] [review]:

Sure.

Review of attachment 239712 [details] [review]:

::: geocode-glib/geocode-ipclient.c
@@ +46,3 @@
+        SERVER_INVALID_ENTRY,
+        SERVER_DATABASE
+} ServerError;

That needs to be moved to a common header so it can be shared with the server.

@@ +344,3 @@
+        location->longitude = json_object_get_double_member (object, "longitude");
+
+        string = g_string_new ("");

Concatenating strings like that won't work.

We should ensure that if you have the city name, you have the region and the country_name, and then you could do:
if (has_member ("city"))
  desc = g_strdup_printf ("%s, %s (%s)", city, region, country);

Created attachment 239909 [details] [review]
server: Put server errors into a separate header

Created attachment 239910 [details] [review]
lib: geocode_ipclient_search* should return location

This API will be a lot more useful if we provide a parsed GeocodeLocation
object to the caller rather than just the raw JSON string we get as
response from server.

Review of attachment 239909 [details] [review]:

::: geocode-glib/geocode-ip-server/geoip-server.h
@@ +4,3 @@
+#define GEOIP_SERVER_H
+
+#include <glib.h>

No need to include glib here.

Review of attachment 239910 [details] [review]:

Short version: fixed the bugs, needs tests added.

::: geocode-glib/geocode-ipclient.c
@@ +285,3 @@
 
+static gboolean
+parse_server_error (JsonObject *object, GError **error) {

brace on the following line. Fixed in the committed patch.

@@ +317,3 @@
+
+static GeocodeLocation *
+json_to_location (const char *json, GError **error) {

Ditto wrt the brace.

I've also made this private to the module so it can be reused in tests (feed it a json output, and check that it's parsed properly without blowing up).

You'll need to provide a few tests for this too.

@@ +339,3 @@
+        location->longitude = json_object_get_double_member (object, "longitude");
+
+        if (json_object_has_member (object, "city"))

I've reorganised this so that we don't cause problems if city is set but not region_name for example.
(we should avoid creating this in the server, but we should be resilient in the client).

@@ +353,3 @@
+
+        if (desc != NULL) {
+            geocode_location_set_description (location, desc);

8-characters indentation :)
Fixed too.

Attachment 239711 [details] pushed as e25df53 - lib: Add error code for internal server errors
Attachment 239909 [details] pushed as 4f1b35a - server: Put server errors into a separate header

Review of attachment 239910 [details] [review]:

Thanks for fixing all this. :)

Created attachment 240439 [details] [review]
Add testcases for _geocode_ip_json_to_location()

Test this function against responses from both our server and freegeoip.

Review of attachment 240439 [details] [review]:

Looks good apart from that.

::: geocode-glib/test-geoip-parse.c
@@ +30,3 @@
+                g_error_free (error);
+        }
+        g_free(contents);

space missing.

@@ +42,3 @@
+        g_test_bug_base ("http://bugzilla.gnome.org/show_bug.cgi?id=");
+
+        g_test_add_data_func ("/geoip/parse-freegeoip-response",

Merge into the existing test suite please.

Created attachment 240485 [details] [review]
Add testcases for _geocode_ip_json_to_location()

Test this function against responses from both our server and freegeoip.

Review of attachment 240485 [details] [review]:

Looks good to commit.

::: geocode-glib/test-geoip.c
@@ +28,3 @@
+        GError *error = NULL;
+
+        path = g_build_filename(TEST_SRCDIR, (const char *) data, NULL);

space.

@@ +30,3 @@
+        path = g_build_filename(TEST_SRCDIR, (const char *) data, NULL);
+        g_assert (path != NULL);
+        file = g_file_new_for_path(path);

space.

@@ +43,3 @@
+        }
+        g_free (contents);
+        g_free(path);

space.

Attachment 240485 [details] pushed as 4f1f6aa - Add testcases for _geocode_ip_json_to_location()