diff --git a/CHANGELOG.md b/CHANGELOG.md index def18f73d..f065f2f0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * Render `*_link` roads narrower than their "regular" counterpart ([#10722]) * Remove `noexit=yes` and `fixme=continue` when you continue a line ([#9634], thanks [@k-yle]) * Don't zoom out when interacting with notes ([#10807], thanks [@hlfan]) +* Add a changeset tag if a merge conflict occurred ([#9636], thanks [@k-yle]) #### :scissors: Operations * Fix splitting of closed ways (or areas) when two or more split-points are selected * Keep `red_turn:left/right` tags unchanged when reversing a way ([#10737], thanks [@burrscurr]) @@ -84,6 +85,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [#9586]: https://github.com/openstreetmap/iD/pull/9586 [#9634]: https://github.com/openstreetmap/iD/pull/9634 [#9635]: https://github.com/openstreetmap/iD/pull/9635 +[#9636]: https://github.com/openstreetmap/iD/pull/9636 [#10003]: https://github.com/openstreetmap/iD/pull/10003 [#10618]: https://github.com/openstreetmap/iD/pull/10618 [#10646]: https://github.com/openstreetmap/iD/pull/10646 diff --git a/modules/core/uploader.js b/modules/core/uploader.js index 2a438e6d8..e01849034 100644 --- a/modules/core/uploader.js +++ b/modules/core/uploader.js @@ -11,6 +11,7 @@ import { t } from '../core/localizer'; import { utilArrayUnion, utilArrayUniq, utilDisplayName, utilDisplayType, utilRebind } from '../util'; +/** @param {iD.Context} context */ export function coreUploader(context) { var dispatch = d3_dispatch( @@ -30,6 +31,7 @@ export function coreUploader(context) { var _isSaving = false; + let _anyConflictsAutomaticallyResolved = false; var _conflicts = []; var _errors = []; var _origChanges; @@ -72,6 +74,7 @@ export function coreUploader(context) { var history = context.history(); + _anyConflictsAutomaticallyResolved = false; _conflicts = []; _errors = []; @@ -251,7 +254,10 @@ export function coreUploader(context) { history.replace(merge); var mergeConflicts = merge.conflicts(); - if (!mergeConflicts.length) return; // merged safely + if (!mergeConflicts.length) { + _anyConflictsAutomaticallyResolved = true; + return; // merged safely + } var forceLocal = actionMergeRemoteChanges(id, localGraph, remoteGraph, _discardTags).withOption('force_local'); var forceRemote = actionMergeRemoteChanges(id, localGraph, remoteGraph, _discardTags).withOption('force_remote'); @@ -273,7 +279,7 @@ export function coreUploader(context) { } - function upload(changeset) { + async function upload(changeset) { var osm = context.connection(); if (!osm) { _errors.push({ msg: 'No OSM Service' }); @@ -286,6 +292,11 @@ export function coreUploader(context) { didResultInErrors(); } else { + if (_anyConflictsAutomaticallyResolved) { + // add a changeset tag to aid reviewers + changeset.tags.merge_conflict_resolved = 'automatically'; + await osm.updateChangesetTags(changeset); + } var history = context.history(); var changes = history.changes(actionDiscardTags(history.difference(), _discardTags)); if (changes.modified.length || changes.created.length || changes.deleted.length) { @@ -339,6 +350,9 @@ export function coreUploader(context) { function didResultInConflicts(changeset) { + // add a changeset tag to aid reviewers + changeset.tags.merge_conflict_resolved = 'manually'; + context.connection().updateChangesetTags(changeset); _conflicts.sort(function(a, b) { return b.id.localeCompare(a.id); }); diff --git a/modules/services/osm.js b/modules/services/osm.js index 11fd1422c..bb329d2ef 100644 --- a/modules/services/osm.js +++ b/modules/services/osm.js @@ -876,6 +876,16 @@ export default { } }, + /** updates the tags on an existing unclosed changeset */ + // PUT /api/0.6/changeset/#id + updateChangesetTags: (changeset) => { + return oauth.fetch(`${oauth.options().apiUrl}/api/0.6/changeset/${changeset.id}`, { + method: 'PUT', + headers: { 'Content-Type': 'text/xml' }, + body: JXON.stringify(changeset.asJXON()) + }); + }, + // Load multiple users in chunks // (note: callback may be called multiple times) diff --git a/test/spec/actions/merge_remote_changes.js b/test/spec/actions/merge_remote_changes.js index 77dad0777..d4ce449b8 100644 --- a/test/spec/actions/merge_remote_changes.js +++ b/test/spec/actions/merge_remote_changes.js @@ -79,6 +79,7 @@ describe('iD.actionMergeRemoteChanges', function () { var result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('doesn\'t merge tags if conflict (local change, remote delete)', function () { @@ -92,6 +93,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('doesn\'t merge tags if conflict (local delete, remote change)', function () { @@ -105,6 +107,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('doesn\'t merge tags if conflict (local add, remote add)', function () { @@ -118,6 +121,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('merges tags if no conflict (remote delete)', function () { @@ -133,6 +137,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('a').version).to.eql('2'); expect(result.entity('a').tags).to.eql(mergedTags); + expect(action.conflicts()).toHaveLength(0); }); it('merges tags if no conflict (local delete)', function () { @@ -148,6 +153,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('a').version).to.eql('2'); expect(result.entity('a').tags).to.eql(mergedTags); + expect(action.conflicts()).toHaveLength(0); }); }); @@ -184,6 +190,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('a').version).to.eql('2'); expect(result.entity('a').tags).to.eql(mergedTags); expect(result.entity('a').loc).to.eql([2, 2]); + expect(action.conflicts()).toHaveLength(0); }); }); @@ -202,6 +209,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').version).to.eql('2'); expect(result.entity('w1').tags).to.eql(mergedTags); + expect(action.conflicts()).toHaveLength(0); }); it('merges ways if nodelist changed only remotely', function () { @@ -222,6 +230,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').nodes).to.eql(remoteNodes); expect(result.hasEntity('r2')).to.eql(r2); expect(result.hasEntity('r3')).to.eql(r3); + expect(action.conflicts()).toHaveLength(0); }); it('merges ways if nodelist changed only locally', function () { @@ -240,6 +249,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').version).to.eql('2'); expect(result.entity('w1').tags).to.eql(mergedTags); expect(result.entity('w1').nodes).to.eql(localNodes); + expect(action.conflicts()).toHaveLength(0); }); it('merges ways if nodelist changes don\'t overlap', function () { @@ -261,6 +271,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').nodes).to.eql(mergedNodes); expect(result.hasEntity('r3')).to.eql(r3); expect(result.hasEntity('r4')).to.eql(r4); + expect(action.conflicts()).toHaveLength(0); }); it('doesn\'t merge ways if nodelist changes overlap', function () { @@ -276,6 +287,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('merges ways if childNode location is same', function () { @@ -290,6 +302,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('p1').version).to.eql('2'); expect(result.entity('p1').loc).to.eql(remoteLoc); + expect(action.conflicts()).toHaveLength(0); }); it('doesn\'t merge ways if childNode location is different', function () { @@ -303,6 +316,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); }); @@ -321,6 +335,7 @@ describe('iD.actionMergeRemoteChanges', function () { result = action(localGraph); expect(result).to.eql(localGraph); + expect(action.conflicts()).toHaveLength(1); }); it('merges relations if members are same and changed tags don\'t conflict', function () { @@ -338,6 +353,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('r').version).to.eql('2'); expect(result.entity('r').tags).to.eql(mergedTags); + expect(action.conflicts()).toHaveLength(0); }); }); @@ -377,6 +393,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('a').version).to.eql('2'); expect(result.entity('a').tags).to.eql(localTags); expect(result.entity('a').loc).to.eql(localLoc); + expect(action.conflicts()).toHaveLength(0); }); it('merges nodes with \'force_remote\' option', function () { @@ -394,6 +411,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('a').version).to.eql('2'); expect(result.entity('a').tags).to.eql(remoteTags); expect(result.entity('a').loc).to.eql(remoteLoc); + expect(action.conflicts()).toHaveLength(0); }); }); @@ -414,6 +432,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').version).to.eql('2'); expect(result.entity('w1').tags).to.eql(localTags); expect(result.entity('w1').nodes).to.eql(localNodes); + expect(action.conflicts()).toHaveLength(0); }); it('merges ways with \'force_remote\' option', function () { @@ -433,6 +452,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').nodes).to.eql(remoteNodes); expect(result.hasEntity('r3')).to.eql(r3); expect(result.hasEntity('r4')).to.eql(r4); + expect(action.conflicts()).toHaveLength(0); }); it('merges way childNodes with \'force_local\' option', function () { @@ -447,6 +467,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('p1').version).to.eql('2'); expect(result.entity('p1').loc).to.eql(localLoc); + expect(action.conflicts()).toHaveLength(0); }); it('merges way childNodes with \'force_remote\' option', function () { @@ -477,6 +498,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('w1').nodes).to.eql(remoteNodes); expect(result.hasEntity('r1')).to.eql(localr1); expect(result.hasEntity('r2')).to.be.not.ok; + expect(action.conflicts()).toHaveLength(0); }); }); @@ -497,6 +519,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('r').version).to.eql('2'); expect(result.entity('r').tags).to.eql(localTags); expect(result.entity('r').members).to.eql(localMembers); + expect(action.conflicts()).toHaveLength(0); }); it('merges relations with \'force_remote\' option', function () { @@ -514,6 +537,7 @@ describe('iD.actionMergeRemoteChanges', function () { expect(result.entity('r').version).to.eql('2'); expect(result.entity('r').tags).to.eql(remoteTags); expect(result.entity('r').members).to.eql(remoteMembers); + expect(action.conflicts()).toHaveLength(0); }); }); });