Changeset refactor

(closes #2633)

* move osmChangeJXON from osm service to osmChangeset
* cleanup putChangeset for code clarity
* adjust params for callbacks (pass changeset around instead of changeset_id)
* add commit.reset() to reset changeset object after successful save
* improve checks for changeset tags (trim whitespace, etc)

modules/modes/save.js

@@ -2,6 +2,17 @@ import * as d3 from 'd3';
 import _ from 'lodash';
 
 import { t } from '../util/locale';
+import { JXON } from '../util/jxon';
+
+import {
+    actionDiscardTags,
+    actionMergeRemoteChanges,
+    actionNoop,
+    actionRevert
+} from '../actions';
+
+import { coreGraph } from '../core';
+import { modeBrowse } from './index';
 
 import {
     uiConflicts,
@@ -9,23 +20,13 @@ import {
     uiCommit,
     uiLoading,
     uiSuccess
-} from '../ui/index';
-
-import {
-    actionDiscardTags,
-    actionMergeRemoteChanges,
-    actionNoop,
-    actionRevert
-} from '../actions/index';
+} from '../ui';
 
 import {
     utilDisplayName,
     utilDisplayType
-} from '../util/index';
+} from '../util';
 
-import { modeBrowse } from './index';
-import { coreGraph } from '../core/index';
-import { JXON } from '../util/jxon';
 
 
 export function modeSave(context) {
@@ -33,7 +34,7 @@ export function modeSave(context) {
         id: 'save'
     };
 
-    var ui = uiCommit(context)
+    var commit = uiCommit(context)
             .on('cancel', cancel)
             .on('save', save);
 
@@ -65,7 +66,7 @@ export function modeSave(context) {
         if (toCheck.length) {
             context.connection().loadMultiple(toLoad, loaded);
         } else {
-            finalize();
+            upload();
         }
 
 
@@ -187,11 +188,11 @@ export function modeSave(context) {
                 });
             });
 
-            finalize();
+            upload();
         }
 
 
-        function finalize() {
+        function upload() {
             if (conflicts.length) {
                 conflicts.sort(function(a,b) { return b.id.localeCompare(a.id); });
                 showConflicts();
@@ -200,29 +201,7 @@ export function modeSave(context) {
             } else {
                 var changes = history.changes(actionDiscardTags(history.difference()));
                 if (changes.modified.length || changes.created.length || changes.deleted.length) {
-                    context.connection().putChangeset(
-                        changes,
-                        context.version,
-                        changeset.tags.comment,
-                        history.imageryUsed(),
-                        function(err, changeset_id) {
-                            if (err) {
-                                errors.push({
-                                    msg: err.responseText,
-                                    details: [ t('save.status_code', { code: err.status }) ]
-                                });
-                                showErrors();
-                            } else {
-                                history.clearSaved();
-                                success(changeset, changeset_id);
-                                // Add delay to allow for postgres replication #1646 #2678
-                                window.setTimeout(function() {
-                                    d3.select('.inspector-wrap *').remove();
-                                    loading.close();
-                                    context.flush();
-                                }, 2500);
-                            }
-                        });
+                    context.connection().putChangeset(changeset, changes, uploadCallback);
                 } else {        // changes were insignificant or reverted by user
                     d3.select('.inspector-wrap *').remove();
                     loading.close();
@@ -233,6 +212,26 @@ export function modeSave(context) {
         }
 
 
+        function uploadCallback(err, changeset) {
+            if (err) {
+                errors.push({
+                    msg: err.responseText,
+                    details: [ t('save.status_code', { code: err.status }) ]
+                });
+                showErrors();
+            } else {
+                history.clearSaved();
+                success(changeset);
+                // Add delay to allow for postgres replication #1646 #2678
+                window.setTimeout(function() {
+                    d3.select('.inspector-wrap *').remove();
+                    loading.close();
+                    context.flush();
+                }, 2500);
+            }
+        }
+
+
         function showConflicts() {
             var selection = context.container()
                 .select('#sidebar')
@@ -244,7 +243,7 @@ export function modeSave(context) {
             selection.call(uiConflicts(context)
                 .list(conflicts)
                 .on('download', function() {
-                    var data = JXON.stringify(context.connection().osmChangeJXON('CHANGEME', origChanges)),
+                    var data = JXON.stringify(changeset.update({ id: 'CHANGEME' }).osmChangeJXON(origChanges)),
                         win = window.open('data:text/xml,' + encodeURIComponent(data), '_blank');
                     win.focus();
                 })
@@ -340,13 +339,11 @@ export function modeSave(context) {
     }
 
 
-    function success(changeset, changeset_id) {
+    function success(changeset) {
+        commit.reset();
         context.enter(modeBrowse(context)
             .sidebar(uiSuccess(context)
-                .changeset({
-                    id: changeset_id,
-                    comment: changeset.tags.comment
-                })
+                .changeset(changeset)
                 .on('cancel', function() {
                     context.ui().sidebar.hide();
                 })
@@ -357,7 +354,7 @@ export function modeSave(context) {
 
     mode.enter = function() {
         function done() {
-            context.ui().sidebar.show(ui);
+            context.ui().sidebar.show(commit);
         }
 
         context.container().selectAll('#content')

modules/osm/changeset.js

@@ -46,6 +46,90 @@ _.extend(osmChangeset.prototype, {
     },
 
 
+    // Generate [osmChange](http://wiki.openstreetmap.org/wiki/OsmChange)
+    // XML. Returns a string.
+    osmChangeJXON: function(changes) {
+        var changeset_id = this.id;
+
+        function nest(x, order) {
+            var groups = {};
+            for (var i = 0; i < x.length; i++) {
+                var tagName = Object.keys(x[i])[0];
+                if (!groups[tagName]) groups[tagName] = [];
+                groups[tagName].push(x[i][tagName]);
+            }
+            var ordered = {};
+            order.forEach(function(o) {
+                if (groups[o]) ordered[o] = groups[o];
+            });
+            return ordered;
+        }
+
+
+        // sort relations in a changeset by dependencies
+        function sort(changes) {
+
+            // find a referenced relation in the current changeset
+            function resolve(item) {
+                return _.find(relations, function(relation) {
+                    return item.keyAttributes.type === 'relation'
+                        && item.keyAttributes.ref === relation['@id'];
+                });
+            }
+
+            // a new item is an item that has not been already processed
+            function isNew(item) {
+                return !sorted[ item['@id'] ] && !_.find(processing, function(proc) {
+                    return proc['@id'] === item['@id'];
+                });
+            }
+
+            var processing = [],
+                sorted = {},
+                relations = changes.relation;
+
+            if (!relations) return changes;
+
+            for (var i = 0; i < relations.length; i++) {
+                var relation = relations[i];
+
+                // skip relation if already sorted
+                if (!sorted[relation['@id']]) {
+                    processing.push(relation);
+                }
+
+                while (processing.length > 0) {
+                    var next = processing[0],
+                    deps = _.filter(_.compact(next.member.map(resolve)), isNew);
+                    if (deps.length === 0) {
+                        sorted[next['@id']] = next;
+                        processing.shift();
+                    } else {
+                        processing = deps.concat(processing);
+                    }
+                }
+            }
+
+            changes.relation = _.values(sorted);
+            return changes;
+        }
+
+        function rep(entity) {
+            return entity.asJXON(changeset_id);
+        }
+
+        return {
+            osmChange: {
+                '@version': 0.6,
+                '@generator': 'iD',
+                'create': sort(nest(changes.created.map(rep), ['node', 'way', 'relation'])),
+                'modify': nest(changes.modified.map(rep), ['node', 'way', 'relation']),
+                'delete': _.extend(nest(changes.deleted.map(rep), ['relation', 'way', 'node']), { '@if-unused': true })
+            }
+        };
+    },
+
+
     asGeoJSON: function() {
         return {};
     }

modules/services/osm.js

@@ -3,17 +3,15 @@ import _ from 'lodash';
 import osmAuth from 'osm-auth';
 import { JXON } from '../util/jxon';
 import { d3geoTile } from '../lib/d3.geo.tile';
-import { geoExtent } from '../geo/index';
+import { geoExtent } from '../geo';
 import {
-    osmChangeset,
     osmEntity,
     osmNode,
     osmRelation,
     osmWay
-} from '../osm/index';
+} from '../osm';
 
-import { utilDetect } from '../util/detect';
-import { utilRebind } from '../util/rebind';
+import { utilRebind } from '../util';
 
 
 var dispatch = d3.dispatch('authLoading', 'authDone', 'change', 'loading', 'loaded'),
@@ -288,132 +286,47 @@ export default {
     },
 
 
-    // Generate [osmChange](http://wiki.openstreetmap.org/wiki/OsmChange)
-    // XML. Returns a string.
-    osmChangeJXON: function(changeset_id, changes) {
-        function nest(x, order) {
-            var groups = {};
-            for (var i = 0; i < x.length; i++) {
-                var tagName = Object.keys(x[i])[0];
-                if (!groups[tagName]) groups[tagName] = [];
-                groups[tagName].push(x[i][tagName]);
-            }
-            var ordered = {};
-            order.forEach(function(o) {
-                if (groups[o]) ordered[o] = groups[o];
-            });
-            return ordered;
-        }
-        // sort relations in a changeset by dependencies
-        function sort(changes) {
-          // find a referenced relation in the current changeset
-          function resolve(item){
-            return _.find(relations, function(relation) {
-              return item.keyAttributes.type === 'relation'
-                && item.keyAttributes.ref === relation['@id'];
-            });
-          }
-          // a new item is an item that has not been already processed
-          function isNew(item) {
-            return !sorted[ item['@id'] ] && !_.find(processing, function(proc){
-             return proc['@id'] === item['@id'];
-            });
-          }
-          var processing = [],
-              sorted = {},
-              relations = changes.relation;
-
-          if (!relations) return changes;
-
-          for (var i = 0; i < relations.length; i++) {
-
-            var relation = relations[i];
-
-            // skip relation if already sorted
-            if ( !sorted[relation['@id']] ) {
-              processing.push( relation );
-            }
-
-            while ( processing.length > 0 ){
-              var next = processing[0],
-                  deps = _.filter( _.compact(next.member.map(resolve)), isNew);
-
-              if ( deps.length === 0 ){
-                sorted[ next['@id'] ] = next;
-                processing.shift();
-              } else {
-                processing = deps.concat( processing );
-              }
-            }
-
-          }
-
-          changes.relation = _.values(sorted);
-          return changes;
-        }
-
-        function rep(entity) {
-            return entity.asJXON(changeset_id);
-        }
-
-        return {
-            osmChange: {
-                '@version': 0.6,
-                '@generator': 'iD',
-                'create': sort(nest(changes.created.map(rep), ['node', 'way', 'relation'])),
-                'modify': nest(changes.modified.map(rep), ['node', 'way', 'relation']),
-                'delete': _.extend(nest(changes.deleted.map(rep), ['relation', 'way', 'node']), {'@if-unused': true})
-            }
-        };
-    },
-
-
-    changesetTags: function(version, comment, imageryUsed) {
-        var detected = utilDetect(),
-            tags = {
-                created_by: ('iD ' + version).substr(0, 255),
-                imagery_used: imageryUsed.join(';').substr(0, 255),
-                host: detected.host.substr(0, 255),
-                locale: detected.locale.substr(0, 255)
-            };
-
-        if (comment) {
-            tags.comment = comment.substr(0, 255);
-        }
-
-        return tags;
-    },
-
-
-    putChangeset: function(changes, version, comment, imageryUsed, callback) {
-        var that = this,
-            changeset = new osmChangeset({ tags: this.changesetTags(version, comment, imageryUsed) });
+    putChangeset: function(changeset, changes, callback) {
 
+        // Create the changeset..
         oauth.xhr({
-                method: 'PUT',
-                path: '/api/0.6/changeset/create',
+            method: 'PUT',
+            path: '/api/0.6/changeset/create',
+            options: { header: { 'Content-Type': 'text/xml' } },
+            content: JXON.stringify(changeset.asJXON())
+        }, createdChangeset);
+
+
+        function createdChangeset(err, changeset_id) {
+            if (err) return callback(err);
+            changeset = changeset.update({ id: changeset_id });
+
+            // Upload the changeset..
+            oauth.xhr({
+                method: 'POST',
+                path: '/api/0.6/changeset/' + changeset_id + '/upload',
                 options: { header: { 'Content-Type': 'text/xml' } },
-                content: JXON.stringify(changeset.asJXON())
-            }, function(err, changeset_id) {
-                if (err) return callback(err);
-                oauth.xhr({
-                    method: 'POST',
-                    path: '/api/0.6/changeset/' + changeset_id + '/upload',
-                    options: { header: { 'Content-Type': 'text/xml' } },
-                    content: JXON.stringify(that.osmChangeJXON(changeset_id, changes))
-                }, function(err) {
-                    if (err) return callback(err);
-                    // POST was successful, safe to call the callback.
-                    // Still attempt to close changeset, but ignore response because #2667
-                    // Add delay to allow for postgres replication #1646 #2678
-                    window.setTimeout(function() { callback(null, changeset_id); }, 2500);
-                    oauth.xhr({
-                        method: 'PUT',
-                        path: '/api/0.6/changeset/' + changeset_id + '/close',
-                        options: { header: { 'Content-Type': 'text/xml' } }
-                    }, function() { return true; });
-                });
-            });
+                content: JXON.stringify(changeset.osmChangeJXON(changes))
+            }, uploadedChangeset);
+        }
+
+
+        function uploadedChangeset(err) {
+            if (err) return callback(err);
+
+            // Upload was successful, safe to call the callback.
+            // Add delay to allow for postgres replication #1646 #2678
+            window.setTimeout(function() {
+                callback(null, changeset);
+            }, 2500);
+
+            // Still attempt to close changeset, but ignore response because #2667
+            oauth.xhr({
+                method: 'PUT',
+                path: '/api/0.6/changeset/' + changeset.id + '/close',
+                options: { header: { 'Content-Type': 'text/xml' } }
+            }, function() { return true; });
+        }
     },
 
 

modules/ui/commit.js

@@ -214,7 +214,6 @@ export function uiCommit(context) {
             })
             .on('click.save', function() {
                 dispatch.call('save', this, changeset);
-                changeset = null;
             });
 
         saveButton
@@ -383,11 +382,11 @@ export function uiCommit(context) {
             var tags = _.clone(changeset.tags);
 
             _.forEach(changed, function(v, k) {
-                k = k.trim();
+                k = k.trim().substr(0, 255);
                 if (readOnlyTags.indexOf(k) !== -1) return;
 
                 if (k !== '' && v !== undefined) {
-                    tags[k] = v.trim();
+                    tags[k] = v.trim().substr(0, 255);
                 } else {
                     delete tags[k];
                 }
@@ -402,5 +401,11 @@ export function uiCommit(context) {
 
     }
 
+
+    commit.reset = function() {
+        changeset = null;
+    };
+
+
     return utilRebind(commit, dispatch, 'on');
 }

modules/ui/success.js

@@ -11,9 +11,6 @@ export function uiSuccess(context) {
 
 
     function success(selection) {
-        var message = (changeset.comment || t('success.edited_osm')).substring(0, 130) +
-            ' ' + context.connection().changesetURL(changeset.id);
-
         var header = selection
             .append('div')
             .attr('class', 'header fillL');
@@ -55,6 +52,10 @@ export function uiSuccess(context) {
             .attr('href', changesetURL)
             .text(t('success.view_on_osm'));
 
+
+        var message = (changeset.tags.comment || t('success.edited_osm')).substring(0, 130) +
+            ' ' + context.connection().changesetURL(changeset.id);
+
         var sharing = {
             facebook: 'https://facebook.com/sharer/sharer.php?u=' + encodeURIComponent(changesetURL),
             twitter: 'https://twitter.com/intent/tweet?source=webclient&text=' + encodeURIComponent(message),

test/spec/osm/changeset.js

@@ -12,6 +12,7 @@ describe('iD.osmChangeset', function () {
         expect(iD.osmChangeset({tags: {foo: 'bar'}}).tags).to.eql({foo: 'bar'});
     });
 
+
     describe('#asJXON', function () {
         it('converts a node to jxon', function() {
             var node = iD.osmChangeset({tags: {'comment': 'hello'}});
@@ -26,4 +27,100 @@ describe('iD.osmChangeset', function () {
             });
         });
     });
+
+
+    describe('#osmChangeJXON', function() {
+        it('converts change data to JXON', function() {
+            var changeset = iD.osmChangeset(),
+                jxon = changeset.osmChangeJXON({ created: [], modified: [], deleted: [] });
+
+            expect(jxon).to.eql({
+                osmChange: {
+                    '@version': 0.6,
+                    '@generator': 'iD',
+                    'create': {},
+                    'modify': {},
+                    'delete': { '@if-unused': true }
+                }
+            });
+        });
+
+        it('includes creations ordered by nodes, ways, relations', function() {
+            var n = iD.osmNode({ loc: [0, 0] }),
+                w = iD.osmWay(),
+                r = iD.osmRelation(),
+                c = iD.osmChangeset({ id: '1234' }),
+                changes = { created: [r, w, n], modified: [], deleted: [] },
+                jxon = c.osmChangeJXON(changes);
+
+            expect(d3.entries(jxon.osmChange.create)).to.eql([
+                { key: 'node', value: [n.asJXON('1234').node] },
+                { key: 'way', value: [w.asJXON('1234').way] },
+                { key: 'relation', value: [r.asJXON('1234').relation] }
+            ]);
+        });
+
+        it('includes creations ordered by dependencies', function() {
+            var n = iD.osmNode({ loc: [0, 0] }),
+                w = iD.osmWay({nodes: [n.id]}),
+                r1 = iD.osmRelation({ members: [{ id: w.id, type: 'way' }] }),
+                r2 = iD.osmRelation({ members: [{ id: r1.id, type: 'relation' }] }),
+                c = iD.osmChangeset({ id: '1234' }),
+                changes = { created: [r2, r1, w, n], modified: [], deleted: [] },
+                jxon = c.osmChangeJXON(changes);
+
+            expect(d3.entries(jxon.osmChange.create)).to.eql([
+                { key: 'node', value: [n.asJXON('1234').node] },
+                { key: 'way', value: [w.asJXON('1234').way] },
+                { key: 'relation', value: [r1.asJXON('1234').relation, r2.asJXON('1234').relation] },
+            ]);
+        });
+
+        it('includes creations ignoring circular dependencies', function() {
+            var r1 = iD.osmRelation(),
+                r2 = iD.osmRelation(),
+                c = iD.osmChangeset({ id: '1234' }),
+                changes, jxon;
+            r1.addMember({ id: r2.id, type: 'relation' });
+            r2.addMember({ id: r1.id, type: 'relation' });
+            changes = { created: [r1,r2], modified: [], deleted: [] };
+            jxon = c.osmChangeJXON(changes);
+
+            expect(d3.entries(jxon.osmChange.create)).to.eql([
+                { key: 'relation', value: [r1.asJXON('1234').relation, r2.asJXON('1234').relation] },
+            ]);
+        });
+
+        it('includes modifications', function() {
+            var n = iD.osmNode({ loc: [0, 0] }),
+                w = iD.osmWay(),
+                r = iD.osmRelation(),
+                c = iD.osmChangeset({ id: '1234' }),
+                changes = { created: [], modified: [r, w, n], deleted: [] },
+                jxon = c.osmChangeJXON(changes);
+
+            expect(jxon.osmChange.modify).to.eql({
+                node: [n.asJXON('1234').node],
+                way: [w.asJXON('1234').way],
+                relation: [r.asJXON('1234').relation]
+            });
+        });
+
+        it('includes deletions ordered by relations, ways, nodes', function() {
+            var n = iD.osmNode({ loc: [0, 0] }),
+                w = iD.osmWay(),
+                r = iD.osmRelation(),
+                c = iD.osmChangeset({ id: '1234' }),
+                changes = { created: [], modified: [], deleted: [n, w, r] },
+                jxon = c.osmChangeJXON(changes);
+
+            expect(d3.entries(jxon.osmChange.delete)).to.eql([
+                { key: 'relation', value: [r.asJXON('1234').relation] },
+                { key: 'way', value: [w.asJXON('1234').way] },
+                { key: 'node', value: [n.asJXON('1234').node] },
+                { key: '@if-unused', value: true }
+            ]);
+        });
+    });
+
 });

test/spec/services/osm.js

@@ -333,95 +333,6 @@ describe('iD.serviceOsm', function () {
     });
 
 
-    describe('#osmChangeJXON', function() {
-        it('converts change data to JXON', function() {
-            var jxon = connection.osmChangeJXON('1234', {created: [], modified: [], deleted: []});
-
-            expect(jxon).to.eql({
-                osmChange: {
-                    '@version': 0.6,
-                    '@generator': 'iD',
-                    'create': {},
-                    'modify': {},
-                    'delete': {'@if-unused': true}
-                }
-            });
-        });
-
-        it('includes creations ordered by nodes, ways, relations', function() {
-            var n = iD.Node({loc: [0, 0]}),
-                w = iD.Way(),
-                r = iD.Relation(),
-                changes = {created: [r, w, n], modified: [], deleted: []},
-                jxon = connection.osmChangeJXON('1234', changes);
-
-            expect(d3.entries(jxon.osmChange.create)).to.eql([
-                {key: 'node', value: [n.asJXON('1234').node]},
-                {key: 'way', value: [w.asJXON('1234').way]},
-                {key: 'relation', value: [r.asJXON('1234').relation]}
-            ]);
-        });
-
-        it('includes creations ordered by dependencies', function() {
-          var n = iD.Node({loc: [0, 0]}),
-              w = iD.Way({nodes: [n.id]}),
-              r1 = iD.Relation({members: [{id: w.id, type: 'way'}]}),
-              r2 = iD.Relation({members: [{id: r1.id, type: 'relation'}]}),
-              changes = {created: [r2, r1, w, n], modified: [], deleted: []},
-              jxon = connection.osmChangeJXON('1234', changes);
-
-          expect(d3.entries(jxon.osmChange.create)).to.eql([
-              {key: 'node', value: [n.asJXON('1234').node]},
-              {key: 'way', value: [w.asJXON('1234').way]},
-              {key: 'relation', value: [r1.asJXON('1234').relation, r2.asJXON('1234').relation]},
-          ]);
-        });
-
-        it('includes creations ignoring circular dependencies', function() {
-          var r1 = iD.Relation(),
-              r2 = iD.Relation(),
-              changes, jxon;
-          r1.addMember({id: r2.id, type: 'relation'});
-          r2.addMember({id: r1.id, type: 'relation'});
-          changes = {created: [r1,r2], modified: [], deleted: []};
-          jxon = connection.osmChangeJXON('1234', changes);
-
-          expect(d3.entries(jxon.osmChange.create)).to.eql([
-              {key: 'relation', value: [r1.asJXON('1234').relation, r2.asJXON('1234').relation]},
-          ]);
-        });
-
-        it('includes modifications', function() {
-            var n = iD.Node({loc: [0, 0]}),
-                w = iD.Way(),
-                r = iD.Relation(),
-                changes = {created: [], modified: [r, w, n], deleted: []},
-                jxon = connection.osmChangeJXON('1234', changes);
-
-            expect(jxon.osmChange.modify).to.eql({
-                node: [n.asJXON('1234').node],
-                way: [w.asJXON('1234').way],
-                relation: [r.asJXON('1234').relation]
-            });
-        });
-
-        it('includes deletions ordered by relations, ways, nodes', function() {
-            var n = iD.Node({loc: [0, 0]}),
-                w = iD.Way(),
-                r = iD.Relation(),
-                changes = {created: [], modified: [], deleted: [n, w, r]},
-                jxon = connection.osmChangeJXON('1234', changes);
-
-            expect(d3.entries(jxon.osmChange.delete)).to.eql([
-                {key: 'relation', value: [r.asJXON('1234').relation]},
-                {key: 'way', value: [w.asJXON('1234').way]},
-                {key: 'node', value: [n.asJXON('1234').node]},
-                {key: '@if-unused', value: true}
-            ]);
-        });
-    });
-
-
     describe('#userChangesets', function() {
         var server, userDetailsFn;
 
@@ -527,13 +438,6 @@ describe('iD.serviceOsm', function () {
     });
 
 
-    describe('#changesetTags', function() {
-        it('omits comment when empty', function() {
-            expect(connection.changesetTags('2.0.0', '', [])).not.to.have.property('comment');
-        });
-    });
-
-
     describe('API capabilities', function() {
         var server,
             capabilitiesXML = '<?xml version="1.0" encoding="UTF-8"?><osm>' +