[openstreetmap-website] Changeset discussion feature (#772)

Matt Amos notifications at github.com
Fri Aug 8 10:28:02 UTC 2014


> @@ -0,0 +1,26 @@
> +changeset_attributes = { :id => changeset.id, :created_at => changeset.created_at.xmlschema, :closed_at => changeset.closed_at, :open => changeset.is_open? }
> +changeset_attributes[:uid] = changeset.user.id if changeset.user.data_public?
> +changeset_attributes[:user] = changeset.user.display_name if changeset.user.data_public?
> +changeset.bbox.to_unscaled.add_bounds_to(changeset_attributes, '_') if changeset.bbox.complete?
> +
> +xml.changeset(changeset_attributes) do |asterx|
> +  changeset.tags.each do |k,v|
> +    xml.tag :k => k, :v => v
> +  end
> +  if @comments
> +    xml.discussion do 

First impressions:

Having a builder for the returned changeset XML from `:read` is a departure from the way we've done things in the past, and means we now have changeset XML defined in two places. This isn't a big deal, but might lead to inconsistencies between the output of `:read` and anywhere `Changeset.to_xml` is used.

The `:text` parameter of `:comment` is from the url-encoded POST body, whereas typically we would take API input as an XML document. I don't think this is a problem, but could lead to inclusion of characters [not representable in XML](http://www.w3.org/TR/REC-xml/#charsets) which will need to be scanned for and a 400 Bad Request returned if any are present. I also note there's no length restriction on the text, which might be worth having.

I didn't notice (perhaps I missed it) what the document type of the text is? Is it HTML or Markdown or just plain text?

What's the rationale for delaying commenting on or subscribing to a changeset until after it has closed? I can see that it complicates matters (as in this github thread) when discussion begins and comments are then "outdated" by subsequent changes, but it also has the side-effect of potentially making users wait an hour for changesets to close from time-out if the editor used doesn't close them.


---
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/772/files#r15986417
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20140808/3c77e1f5/attachment.html>


More information about the rails-dev mailing list