From: Frederik Ramm <frederik@remote.org>
Date: Fri, 1 Mar 2019 00:11:19 +0000 (+0100)
Subject: Move changeset XML generation to a view
X-Git-Tag: live~2652^2~3
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/538bfed8a61a576e44d8cc71d7727c0310bcf238

Move changeset XML generation to a view
---

diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb
index 6093f529e..9ecd22b45 100644
--- a/app/controllers/api/changeset_comments_controller.rb
+++ b/app/controllers/api/changeset_comments_controller.rb
@@ -41,7 +41,8 @@ module Api
       changeset.subscribers << current_user unless changeset.subscribers.exists?(current_user.id)
 
       # Return a copy of the updated changeset
-      render :xml => changeset.to_xml.to_s
+      @changeset = changeset
+      render "changesets/changeset"
     end
 
     ##
@@ -60,7 +61,8 @@ module Api
       comment.update(:visible => false)
 
       # Return a copy of the updated changeset
-      render :xml => comment.changeset.to_xml.to_s
+      @changeset = comment.changeset
+      render "changesets/changeset"
     end
 
     ##
@@ -79,7 +81,8 @@ module Api
       comment.update(:visible => true)
 
       # Return a copy of the updated changeset
-      render :xml => comment.changeset.to_xml.to_s
+      @changeset = comment.changeset
+      render "changesets/changeset"
     end
   end
 end
diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb
index 853ee389f..92690a75e 100644
--- a/app/controllers/api/changesets_controller.rb
+++ b/app/controllers/api/changesets_controller.rb
@@ -41,9 +41,9 @@ module Api
     # Return XML giving the basic info about the changeset. Does not
     # return anything about the nodes, ways and relations in the changeset.
     def show
-      changeset = Changeset.find(params[:id])
-
-      render :xml => changeset.to_xml(params[:include_discussion].presence).to_s
+      @changeset = Changeset.find(params[:id])
+      @include_discussion = params[:include_discussion].presence
+      render "changesets/changeset"
     end
 
     ##
@@ -104,7 +104,8 @@ module Api
       # save the larger bounding box and return the changeset, which
       # will include the bigger bounding box.
       cs.save!
-      render :xml => cs.to_xml.to_s
+      @changeset = cs
+      render "changesets/changeset"
     end
 
     ##
@@ -219,18 +220,9 @@ module Api
       # sort and limit the changesets
       changesets = changesets.order("created_at DESC").limit(100)
 
-      # preload users, tags and comments
-      changesets = changesets.preload(:user, :changeset_tags, :comments)
-
-      # create the results document
-      results = OSM::API.new.get_xml_doc
-
-      # add all matching changesets to the XML results document
-      changesets.order("created_at DESC").limit(100).each do |cs|
-        results.root << cs.to_xml_node
-      end
-
-      render :xml => results.to_s
+      # preload users, tags and comments, and render result
+      @changesets = changesets.preload(:user, :changeset_tags, :comments)
+      render "changesets/changesets"
     end
 
     ##
@@ -245,12 +237,12 @@ module Api
       # request *must* be a PUT.
       assert_method :put
 
-      changeset = Changeset.find(params[:id])
+      @changeset = Changeset.find(params[:id])
       new_changeset = Changeset.from_xml(request.raw_post)
 
-      check_changeset_consistency(changeset, current_user)
-      changeset.update_from(new_changeset, current_user)
-      render :xml => changeset.to_xml.to_s
+      check_changeset_consistency(@changeset, current_user)
+      @changeset.update_from(new_changeset, current_user)
+      render "changesets/changeset"
     end
 
     ##
@@ -270,7 +262,8 @@ module Api
       changeset.subscribers << current_user
 
       # Return a copy of the updated changeset
-      render :xml => changeset.to_xml.to_s
+      @changeset = changeset
+      render "changesets/changeset"
     end
 
     ##
@@ -290,7 +283,8 @@ module Api
       changeset.subscribers.delete(current_user)
 
       # Return a copy of the updated changeset
-      render :xml => changeset.to_xml.to_s
+      @changeset = changeset
+      render "changesets/changeset"
     end
 
     private
diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index 5cdfeb994..3ca719f6c 100644
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -196,67 +196,6 @@ class Changeset < ActiveRecord::Base
     end
   end
 
-  def to_xml(include_discussion = false)
-    doc = OSM::API.new.get_xml_doc
-    doc.root << to_xml_node(nil, include_discussion)
-    doc
-  end
-
-  def to_xml_node(user_display_name_cache = nil, include_discussion = false)
-    el1 = XML::Node.new "changeset"
-    el1["id"] = id.to_s
-
-    user_display_name_cache = {} if user_display_name_cache.nil?
-
-    if user_display_name_cache&.key?(user_id)
-      # use the cache if available
-    elsif user.data_public?
-      user_display_name_cache[user_id] = user.display_name
-    else
-      user_display_name_cache[user_id] = nil
-    end
-
-    el1["user"] = user_display_name_cache[user_id] unless user_display_name_cache[user_id].nil?
-    el1["uid"] = user_id.to_s if user.data_public?
-
-    tags.each do |k, v|
-      el2 = XML::Node.new("tag")
-      el2["k"] = k.to_s
-      el2["v"] = v.to_s
-      el1 << el2
-    end
-
-    el1["created_at"] = created_at.xmlschema
-    el1["closed_at"] = closed_at.xmlschema unless is_open?
-    el1["open"] = is_open?.to_s
-
-    bbox.to_unscaled.add_bounds_to(el1, "_") if bbox.complete?
-
-    el1["comments_count"] = comments.length.to_s
-    el1["changes_count"] = num_changes.to_s
-
-    if include_discussion
-      el2 = XML::Node.new("discussion")
-      comments.includes(:author).each do |comment|
-        el3 = XML::Node.new("comment")
-        el3["date"] = comment.created_at.xmlschema
-        el3["uid"] = comment.author.id.to_s if comment.author.data_public?
-        el3["user"] = comment.author.display_name.to_s if comment.author.data_public?
-        el4 = XML::Node.new("text")
-        el4.content = comment.body.to_s
-        el3 << el4
-        el2 << el3
-      end
-      el1 << el2
-    end
-
-    # NOTE: changesets don't include the XML of the changes within them,
-    # they are just structures for tagging. to get the osmChange of a
-    # changeset, see the download method of the controller.
-
-    el1
-  end
-
   ##
   # update this instance from another instance given and the user who is
   # doing the updating. note that this method is not for updating the
diff --git a/app/views/changesets/_changeset.builder b/app/views/changesets/_changeset.builder
new file mode 100644
index 000000000..47c494d9b
--- /dev/null
+++ b/app/views/changesets/_changeset.builder
@@ -0,0 +1,43 @@
+# basic attributes
+
+attrs = {
+  "id" => changeset.id,
+  "created_at" => changeset.created_at.xmlschema,
+  "open" => changeset.is_open?,
+  "comments_count" => changeset.comments.length,
+  "changes_count" => changeset.num_changes
+}
+attrs["closed_at"] = changeset.closed_at unless changeset.is_open?
+changeset.bbox.to_unscaled.add_bounds_to(attrs, "_") if changeset.bbox.complete?
+
+# user attributes
+
+if changeset.user.data_public?
+  attrs["uid"] = changeset.user_id
+  attrs["user"] = changeset.user.display_name
+end
+
+xml.changeset(attrs) do |changeset_xml_node|
+  changeset.tags.each do |k, v|
+    changeset_xml_node.tag(:k => k, :v => v)
+  end
+
+  # include discussion if requested
+
+  if @include_discussion
+    changeset_xml_node.discussion do |discussion_xml_node|
+      changeset.comments.includes(:author).each do |comment|
+        cattrs = {
+          "date" => comment.created_at.xmlschema
+        }
+        if comment.author.data_public?
+          cattrs["uid"] = comment.author.id
+          cattrs["user"] = comment.author.display_name
+        end
+        discussion_xml_node.comment(cattrs) do |comment_xml_node|
+          comment_xml_node.text(comment.body)
+        end
+      end
+    end
+  end
+end
diff --git a/app/views/changesets/changeset.builder b/app/views/changesets/changeset.builder
new file mode 100644
index 000000000..a2faaac9f
--- /dev/null
+++ b/app/views/changesets/changeset.builder
@@ -0,0 +1,7 @@
+xml.instruct! :xml, :version => "1.0"
+
+# basic attributes
+
+xml.osm(OSM::API.new.xml_root_attributes) do |osm|
+  osm << render(:partial => "changesets/changeset.builder", :locals => { :changeset => @changeset })
+end
diff --git a/app/views/changesets/changesets.builder b/app/views/changesets/changesets.builder
new file mode 100644
index 000000000..80037e4b7
--- /dev/null
+++ b/app/views/changesets/changesets.builder
@@ -0,0 +1,9 @@
+xml.instruct! :xml, :version => "1.0"
+
+# basic attributes
+
+xml.osm(OSM::API.new.xml_root_attributes) do |osm|
+  @changesets.each do |changeset|
+     osm << render(:partial => "changesets/changeset.builder", :locals => { :changeset => changeset })
+  end
+end
diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb
index 75896b202..b62235a39 100644
--- a/test/controllers/api/changesets_controller_test.rb
+++ b/test/controllers/api/changesets_controller_test.rb
@@ -1674,7 +1674,7 @@ CHANGESET
       changeset = create(:changeset, :user => user)
 
       ## First try with a non-public user
-      new_changeset = private_changeset.to_xml
+      new_changeset = create_changeset_xml(user: private_user)
       new_tag = XML::Node.new "tag"
       new_tag["k"] = "tagtesting"
       new_tag["v"] = "valuetesting"
@@ -1695,8 +1695,7 @@ CHANGESET
       assert_require_public_data "user with their data non-public, shouldn't be able to edit their changeset"
 
       ## Now try with the public user
-      create(:changeset_tag, :changeset => changeset)
-      new_changeset = changeset.to_xml
+      new_changeset = create_changeset_xml(id: 1)
       new_tag = XML::Node.new "tag"
       new_tag["k"] = "tagtesting"
       new_tag["v"] = "valuetesting"
@@ -1718,7 +1717,7 @@ CHANGESET
       assert_response :success
 
       assert_select "osm>changeset[id='#{changeset.id}']", 1
-      assert_select "osm>changeset>tag", 2
+      assert_select "osm>changeset>tag", 1
       assert_select "osm>changeset>tag[k='tagtesting'][v='valuetesting']", 1
     end
 
@@ -1729,7 +1728,7 @@ CHANGESET
       basic_authorization create(:user).email, "test"
 
       changeset = create(:changeset)
-      new_changeset = changeset.to_xml
+      new_changeset = create_changeset_xml(user: changeset.user, id: changeset.id)
       new_tag = XML::Node.new "tag"
       new_tag["k"] = "testing"
       new_tag["v"] = "testing"
@@ -1959,5 +1958,22 @@ CHANGESET
       xml.find("//osm/way").first[name] = value.to_s
       xml
     end
+
+    ##
+    # build XML for changesets
+    def create_changeset_xml(user: nil, id: nil)
+      root = XML::Document.new
+      root.root = XML::Node.new "osm"
+      cs = XML::Node.new "changeset"
+      if user
+        cs["user"] = user.display_name
+        cs["uid"] = user.id.to_s
+      end
+      if id
+        cs["id"] = id.to_s
+      end
+      root.root << cs
+      root
+    end
   end
 end