Move generation of gpx_file XML from model to builder

app/controllers/api/traces_controller.rb

@@ -19,7 +19,8 @@ module Api
       trace = Trace.visible.find(params[:id])
 
       if trace.public? || trace.user == current_user
-        render :xml => trace.to_xml.to_s
+        @traces = [trace]
+        render "trace"
       else
         head :forbidden
       end

app/controllers/api/users_controller.rb

@@ -37,11 +37,8 @@ module Api
     end
 
     def gpx_files
-      doc = OSM::API.new.get_xml_doc
+      @traces = current_user.traces.reload
-      current_user.traces.reload.each do |trace|
+      render "api/traces/trace", :content_type => "application/xml"
-        doc.root << trace.to_xml_node
-      end
-      render :xml => doc.to_s
     end
 
     private

app/models/trace.rb

@@ -169,36 +169,6 @@ class Trace < ActiveRecord::Base
     extension
   end
 
-  def to_xml
-    doc = OSM::API.new.get_xml_doc
-    doc.root << to_xml_node
-    doc
-  end
-
-  def to_xml_node
-    el1 = XML::Node.new "gpx_file"
-    el1["id"] = id.to_s
-    el1["name"] = name.to_s
-    el1["lat"] = latitude.to_s if inserted
-    el1["lon"] = longitude.to_s if inserted
-    el1["user"] = user.display_name
-    el1["visibility"] = visibility
-    el1["pending"] = inserted ? "false" : "true"
-    el1["timestamp"] = timestamp.xmlschema
-
-    el2 = XML::Node.new "description"
-    el2 << description
-    el1 << el2
-
-    tags.each do |tag|
-      el2 = XML::Node.new("tag")
-      el2 << tag.tag
-      el1 << el2
-    end
-
-    el1
-  end
-
   def update_from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse

app/views/api/traces/_trace.builder

@@ -0,0 +1,22 @@
+# basic attributes
+
+attrs = {
+  "id" => trace.id,
+  "name" => trace.name,
+  "user" => trace.user.display_name,
+  "visibility" => trace.visibility,
+  "pending" => trace.inserted ? "false" : "true",
+  "timestamp" => trace.timestamp.xmlschema
+}
+
+if trace.inserted
+  attrs["lat"] = trace.latitude.to_s
+  attrs["lon"] = trace.longitude.to_s
+end
+
+xml.gpx_file(attrs) do |trace_xml_node|
+  trace_xml_node.description(trace.description)
+  trace.tags.each do |t|
+    trace_xml_node.tag(t.tag)
+  end
+end

app/views/api/traces/trace.builder

@@ -0,0 +1,9 @@
+xml.instruct! :xml, :version => "1.0"
+
+# basic attributes
+
+xml.osm(OSM::API.new.xml_root_attributes) do |osm|
+  @traces.each do |trace|
+    osm << render(:partial => "api/traces/trace.builder", :locals => { :trace => trace })
+  end
+end

test/controllers/api/traces_controller_test.rb

@@ -250,27 +250,27 @@ module Api
       anon_trace_file = create(:trace, :visibility => "private")
 
       # First with no auth
-      put :update, :params => { :id => public_trace_file.id }, :body => public_trace_file.to_xml.to_s
+      put :update, :params => { :id => public_trace_file.id }, :body => create_trace_xml(public_trace_file)
       assert_response :unauthorized
 
       # Now with some other user, which should fail
       basic_authorization create(:user).display_name, "test"
-      put :update, :params => { :id => public_trace_file.id }, :body => public_trace_file.to_xml.to_s
+      put :update, :params => { :id => public_trace_file.id }, :body => create_trace_xml(public_trace_file)
       assert_response :forbidden
 
       # Now with a trace which doesn't exist
       basic_authorization create(:user).display_name, "test"
-      put :update, :params => { :id => 0 }, :body => public_trace_file.to_xml.to_s
+      put :update, :params => { :id => 0 }, :body => create_trace_xml(public_trace_file)
       assert_response :not_found
 
       # Now with a trace which did exist but has been deleted
       basic_authorization deleted_trace_file.user.display_name, "test"
-      put :update, :params => { :id => deleted_trace_file.id }, :body => deleted_trace_file.to_xml.to_s
+      put :update, :params => { :id => deleted_trace_file.id }, :body => create_trace_xml(deleted_trace_file)
       assert_response :not_found
 
       # Now try an update with the wrong ID
       basic_authorization public_trace_file.user.display_name, "test"
-      put :update, :params => { :id => public_trace_file.id }, :body => anon_trace_file.to_xml.to_s
+      put :update, :params => { :id => public_trace_file.id }, :body => create_trace_xml(anon_trace_file)
       assert_response :bad_request,
                       "should not be able to update a trace with a different ID from the XML"
 
@@ -279,7 +279,7 @@ module Api
       t = public_trace_file
       t.description = "Changed description"
       t.visibility = "private"
-      put :update, :params => { :id => t.id }, :body => t.to_xml.to_s
+      put :update, :params => { :id => t.id }, :body => create_trace_xml(t)
       assert_response :success
       nt = Trace.find(t.id)
       assert_equal nt.description, t.description
@@ -292,7 +292,7 @@ module Api
       trace = tracetag.trace
       basic_authorization trace.user.display_name, "test"
 
-      put :update, :params => { :id => trace.id }, :body => trace.to_xml.to_s
+      put :update, :params => { :id => trace.id }, :body => create_trace_xml(trace)
       assert_response :success
 
       updated = Trace.find(trace.id)
@@ -339,5 +339,27 @@ module Api
       assert_equal content_type, response.content_type
       assert_equal "attachment; filename=\"#{trace.id}.#{extension}\"", @response.header["Content-Disposition"]
     end
+
+    ##
+    # build XML for traces
+    # this builds a minimum viable XML for the tests in this suite
+    def create_trace_xml(trace)
+      root = XML::Document.new
+      root.root = XML::Node.new "osm"
+      trc = XML::Node.new "gpx_file"
+      trc["id"] = trace.id.to_s
+      trc["visibility"] = trace.visibility
+      trc["visible"] = trace.visible.to_s
+      desc = XML::Node.new "description"
+      desc << trace.description
+      trc << desc
+      trace.tags.each do |tag|
+        t = XML::Node.new "tag"
+        t << tag.tag
+        trc << t
+      end
+      root.root << trc
+      root.to_s
+    end
   end
 end