[openstreetmap/openstreetmap-website] Add "Set home location to here" button on context menu (#1868)

[Tom Hughes]

tomhughes requested changes on this pull request.

In addition to the inline comments this needs some tests - at the very least the new route needs adding to the route tests in the user controller tests and there should be a test of the controller method as well.

> @@ -72,6 +72,22 @@ OSM.initializeContextMenu = function (map) {
     }
   });
 
+  map.contextmenu.addItem({
+    text: I18n.t("javascripts.context.set_home_location"),
+    callback: function setHomeLocation(e) {
+      var precision = OSM.zoomPrecision(map.getZoom()),
+          latlng = e.latlng.wrap(),
+          lat = latlng.lat.toFixed(precision),
+          lng = latlng.lng.toFixed(precision);
+      
+      $.ajax({url: "/set_home_loc?lat=" + lat + "&lon=" + lng, success: function(){

I wonder if this should be a `POST` request? It is technically idempotent I guess, but having `GET` requests modify state doesn't really seem right.

> @@ -256,6 +256,9 @@
   # directions
   get "/directions" => "directions#search"
 
+  # set home location
+  match "/set_home_loc" => "user#set_home_location", :via => [:get, :post]

Whatever the decision is about the request type to use, this should only match what is needed and not the other one.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1868#pullrequestreview-120859003
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20180516/e3a60b07/attachment.html>