From e4f062b7a5e43c94fd3ee62772eec54b3e5dc427 Mon Sep 17 00:00:00 2001 From: pietervdvn Date: Mon, 14 Mar 2022 03:33:03 +0100 Subject: [PATCH] Fix dissappearing default tags, add tests --- Logic/Osm/Changes.ts | 2 +- Logic/Osm/ChangesetHandler.ts | 58 ++++++----- test/Changes.spec.ts | 51 ++++++++++ test/ChangesetHandler.spec.ts | 181 ++++++++++++++++++++++++++++++++++ test/TestAll.ts | 7 +- 5 files changed, 273 insertions(+), 26 deletions(-) create mode 100644 test/Changes.spec.ts create mode 100644 test/ChangesetHandler.spec.ts diff --git a/Logic/Osm/Changes.ts b/Logic/Osm/Changes.ts index c8cfb83bc..362d12853 100644 --- a/Logic/Osm/Changes.ts +++ b/Logic/Osm/Changes.ts @@ -363,7 +363,7 @@ export class Changes { } - private CreateChangesetObjects(changes: ChangeDescription[], downloadedOsmObjects: OsmObject[]): { + public CreateChangesetObjects(changes: ChangeDescription[], downloadedOsmObjects: OsmObject[]): { newObjects: OsmObject[], modifiedObjects: OsmObject[] deletedObjects: OsmObject[] diff --git a/Logic/Osm/ChangesetHandler.ts b/Logic/Osm/ChangesetHandler.ts index b62f9374b..5b07a0faf 100644 --- a/Logic/Osm/ChangesetHandler.ts +++ b/Logic/Osm/ChangesetHandler.ts @@ -51,13 +51,14 @@ export class ChangesetHandler { } /** + * Inplace rewrite of extraMetaTags * If the metatags contain a special motivation of the format ":node/-", this method will rewrite this negative number to the actual ID * The key is changed _in place_; true will be returned if a change has been applied * @param extraMetaTags * @param rewriteIds * @private */ - private static rewriteMetaTags(extraMetaTags: ChangesetTag[], rewriteIds: Map) { + static rewriteMetaTags(extraMetaTags: ChangesetTag[], rewriteIds: Map) { let hasChange = false; for (const tag of extraMetaTags) { const match = tag.key.match(/^([a-zA-Z0-9_]+):(node\/-[0-9])$/) @@ -92,6 +93,8 @@ export class ChangesetHandler { if (!extraMetaTags.some(tag => tag.key === "comment") || !extraMetaTags.some(tag => tag.key === "theme")) { throw "The meta tags should at least contain a `comment` and a `theme`" } + + extraMetaTags = [...extraMetaTags, ...this.defaultChangesetTags()] if (this.userDetails.data.csCount == 0) { // The user became a contributor! @@ -112,7 +115,7 @@ export class ChangesetHandler { openChangeset.setData(csId); const changeset = generateChangeXML(csId); console.trace("Opened a new changeset (openChangeset.data is undefined):", changeset); - const changes = await this.AddChange(csId, changeset) + const changes = await this.UploadChange(csId, changeset) const hasSpecialMotivationChanges = ChangesetHandler.rewriteMetaTags(extraMetaTags, changes) if(hasSpecialMotivationChanges){ // At this point, 'extraMetaTags' will have changed - we need to set the tags again @@ -139,11 +142,12 @@ export class ChangesetHandler { return; } - const rewritings = await this.AddChange( + const rewritings = await this.UploadChange( csId, generateChangeXML(csId)) - await this.RewriteTagsOf(extraMetaTags, rewritings, oldChangesetMeta) + const rewrittenTags = this.RewriteTagsOf(extraMetaTags, rewritings, oldChangesetMeta) + await this.UpdateTags(csId, rewrittenTags) } catch (e) { console.warn("Could not upload, changeset is probably closed: ", e); @@ -153,13 +157,13 @@ export class ChangesetHandler { } /** - * Updates the metatag of a changeset - + * Given an existing changeset with metadata and extraMetaTags to add, will fuse them to a new set of metatags + * Does not yet send data * @param extraMetaTags: new changeset tags to add/fuse with this changeset + * @param rewriteIds: the mapping of ids * @param oldChangesetMeta: the metadata-object of the already existing changeset - * @constructor - * @private */ - private async RewriteTagsOf(extraMetaTags: ChangesetTag[], + public RewriteTagsOf(extraMetaTags: ChangesetTag[], rewriteIds: Map, oldChangesetMeta: { open: boolean, @@ -167,9 +171,8 @@ export class ChangesetHandler { uid: number, // User ID changes_count: number, tags: any - }) { + }) : ChangesetTag[] { - const csId = oldChangesetMeta.id // Note: extraMetaTags is where all the tags are collected into @@ -214,10 +217,8 @@ export class ChangesetHandler { ChangesetHandler.rewriteMetaTags(extraMetaTags, rewriteIds) - - await this.UpdateTags(csId, extraMetaTags) - - + return extraMetaTags + } private handleIdRewrite(node: any, type: string): [string, string] { @@ -295,7 +296,7 @@ export class ChangesetHandler { }) } - private async GetChangesetMeta(csId: number): Promise<{ + async GetChangesetMeta(csId: number): Promise<{ id: number, open: boolean, uid: number, @@ -340,21 +341,30 @@ export class ChangesetHandler { }); }) } + + private defaultChangesetTags() : ChangesetTag[]{ + return [ ["created_by", `MapComplete ${Constants.vNumber}`], + ["locale", Locale.language.data], + ["host", `${window.location.origin}${window.location.pathname}`], + ["source", this.changes.state["currentUserLocation"]?.features?.data?.length > 0 ? "survey" : undefined], + ["imagery", this.changes.state["backgroundLayer"]?.data?.id]].map(([key, value]) => ({ + key, value, aggretage: false + })) + } + /** + * Opens a changeset with the specified tags + * @param changesetTags + * @constructor + * @private + */ private OpenChangeset( changesetTags: ChangesetTag[] ): Promise { const self = this; return new Promise(function (resolve, reject) { - const metadata = [ - ["created_by", `MapComplete ${Constants.vNumber}`], - ["locale", Locale.language.data], - ["host", `${window.location.origin}${window.location.pathname}`], - ["source", self.changes.state["currentUserLocation"]?.features?.data?.length > 0 ? "survey" : undefined], - ["imagery", self.changes.state["backgroundLayer"]?.data?.id], - ...changesetTags.map(cstag => [cstag.key, cstag.value]) - ] + const metadata = changesetTags.map(cstag => [cstag.key, cstag.value]) .filter(kv => (kv[1] ?? "") !== "") .map(kv => ``) .join("\n") @@ -382,7 +392,7 @@ export class ChangesetHandler { /** * Upload a changesetXML */ - private AddChange(changesetId: number, + private UploadChange(changesetId: number, changesetXML: string): Promise> { const self = this; return new Promise(function (resolve, reject) { diff --git a/test/Changes.spec.ts b/test/Changes.spec.ts new file mode 100644 index 000000000..2466be54a --- /dev/null +++ b/test/Changes.spec.ts @@ -0,0 +1,51 @@ +import T from "./TestHelper"; +import {Changes} from "../Logic/Osm/Changes"; +import {ChangeDescription, ChangeDescriptionTools} from "../Logic/Osm/Actions/ChangeDescription"; + +export default class ChangesSpec extends T { + + constructor() { + super([ + ["Generate preXML from changeDescriptions", () => { + const changeDescrs: ChangeDescription[] = [ + { + type: "node", + id: -1, + changes: { + lat: 42, + lon: -8 + }, + tags: [{k: "someKey", v: "someValue"}], + meta: { + changeType: "create", + theme: "test" + } + }, + { + type: "node", + id: -1, + tags: [{k: 'foo', v: 'bar'}], + meta: { + changeType: "answer", + theme: "test" + } + } + ] + const c = new Changes() + const descr = c.CreateChangesetObjects( + changeDescrs, + [] + ) + T.equals(0, descr.modifiedObjects.length) + T.equals(0, descr.deletedObjects.length) + T.equals(1, descr.newObjects.length) + const ch = descr.newObjects[0] + T.equals("bar", ch.tags["foo"]) + T.equals("someValue", ch.tags["someKey"]) + }] + ]); + + } + + +} \ No newline at end of file diff --git a/test/ChangesetHandler.spec.ts b/test/ChangesetHandler.spec.ts new file mode 100644 index 000000000..8e0ee713b --- /dev/null +++ b/test/ChangesetHandler.spec.ts @@ -0,0 +1,181 @@ +import T from "./TestHelper"; +import {ChangesetHandler, ChangesetTag} from "../Logic/Osm/ChangesetHandler"; +import {UIEventSource} from "../Logic/UIEventSource"; +import {OsmConnection} from "../Logic/Osm/OsmConnection"; +import {ElementStorage} from "../Logic/ElementStorage"; +import {Changes} from "../Logic/Osm/Changes"; + +export default class ChangesetHandlerSpec extends T { + + private static asDict(tags: {key: string, value: string | number}[]) : Map{ + const d= new Map() + + for (const tag of tags) { + d.set(tag.key, tag.value) + } + + return d + } + + constructor() { + super([ + [ + "Test rewrite tags", () => { + const cs = new ChangesetHandler(new UIEventSource(true), + new OsmConnection({}), + new ElementStorage(), + new Changes(), + new UIEventSource(undefined) + ); + + + const oldChangesetMeta = { + "type": "changeset", + "id": 118443748, + "created_at": "2022-03-13T19:52:10Z", + "closed_at": "2022-03-13T20:54:35Z", + "open": false, + "user": "Pieter Vander Vennet", + "uid": 3818858, + "minlat": 51.0361902, + "minlon": 3.7092939, + "maxlat": 51.0364194, + "maxlon": 3.7099520, + "comments_count": 0, + "changes_count": 3, + "tags": { + "answer": "5", + "comment": "Adding data with #MapComplete for theme #toerisme_vlaanderen", + "created_by": "MapComplete 0.16.6", + "host": "https://mapcomplete.osm.be/toerisme_vlaanderen.html", + "imagery": "osm", + "locale": "nl", + "source": "survey", + "source:node/-1":"note/1234", + "theme": "toerisme_vlaanderen", + } + } + let rewritten = cs.RewriteTagsOf( + [{ + key: "newTag", + value: "newValue", + aggregate: false + }], + new Map(), + oldChangesetMeta) + let d = ChangesetHandlerSpec.asDict(rewritten) + T.equals(10, d.size) + T.equals("5", d.get("answer")) + T.equals("Adding data with #MapComplete for theme #toerisme_vlaanderen", d.get("comment")) + T.equals("MapComplete 0.16.6", d.get("created_by")) + T.equals("https://mapcomplete.osm.be/toerisme_vlaanderen.html", d.get("host")) + T.equals("osm", d.get("imagery")) + T.equals("survey", d.get("source")) + T.equals("note/1234", d.get("source:node/-1")) + T.equals("toerisme_vlaanderen", d.get("theme")) + + T.equals("newValue", d.get("newTag")) + + + + + rewritten = cs.RewriteTagsOf( + [{ + key: "answer", + value: "37", + aggregate: true + }], + new Map(), + oldChangesetMeta) + d = ChangesetHandlerSpec.asDict(rewritten) + + T.equals(9, d.size) + T.equals("42", d.get("answer")) + T.equals("Adding data with #MapComplete for theme #toerisme_vlaanderen", d.get("comment")) + T.equals("MapComplete 0.16.6", d.get("created_by")) + T.equals("https://mapcomplete.osm.be/toerisme_vlaanderen.html", d.get("host")) + T.equals("osm", d.get("imagery")) + T.equals("survey", d.get("source")) + T.equals("note/1234", d.get("source:node/-1")) + T.equals("toerisme_vlaanderen", d.get("theme")) + + rewritten = cs.RewriteTagsOf( + [], + new Map([["node/-1", "node/42"]]), + oldChangesetMeta) + d = ChangesetHandlerSpec.asDict(rewritten) + + T.equals(9, d.size) + T.equals("5", d.get("answer")) + T.equals("Adding data with #MapComplete for theme #toerisme_vlaanderen", d.get("comment")) + T.equals("MapComplete 0.16.6", d.get("created_by")) + T.equals("https://mapcomplete.osm.be/toerisme_vlaanderen.html", d.get("host")) + T.equals("osm", d.get("imagery")) + T.equals("survey", d.get("source")) + T.equals("note/1234", d.get("source:node/42")) + T.equals("toerisme_vlaanderen", d.get("theme")) + + }, + ],[ + "Test rewrite on special motivation", () => { + + + const cs = new ChangesetHandler(new UIEventSource(true), + new OsmConnection({}), + new ElementStorage(), + new Changes(), + new UIEventSource(undefined) + ); + + + const oldChangesetMeta = { + "type": "changeset", + "id": 118443748, + "created_at": "2022-03-13T19:52:10Z", + "closed_at": "2022-03-13T20:54:35Z", + "open": false, + "user": "Pieter Vander Vennet", + "uid": 3818858, + "minlat": 51.0361902, + "minlon": 3.7092939, + "maxlat": 51.0364194, + "maxlon": 3.7099520, + "comments_count": 0, + "changes_count": 3, + "tags": { + "answer": "5", + "comment": "Adding data with #MapComplete for theme #toerisme_vlaanderen", + "created_by": "MapComplete 0.16.6", + "host": "https://mapcomplete.osm.be/toerisme_vlaanderen.html", + "imagery": "osm", + "locale": "nl", + "source": "survey", + "source:-1":"note/1234", + "theme": "toerisme_vlaanderen", + } + } + + const extraMetaTags : ChangesetTag[] = [ + { + key: "created_by", + value:"mapcomplete" + }, + { + key: "source:node/-1", + value:"note/1234" + } + ] + const changes = new Map([["node/-1","node/42"]]) + const hasSpecialMotivationChanges = ChangesetHandler.rewriteMetaTags(extraMetaTags, changes) + T.isTrue(hasSpecialMotivationChanges, "Special rewrite did not trigger") + // Rewritten inline by rewriteMetaTags + T.equals("source:node/42", extraMetaTags[1].key) + T.equals("note/1234", extraMetaTags[1].value) + T.equals("created_by", extraMetaTags[0].key) + T.equals("mapcomplete", extraMetaTags[0].value) + } + + ] + ]); + } +} \ No newline at end of file diff --git a/test/TestAll.ts b/test/TestAll.ts index 37f408220..9dc045cf9 100644 --- a/test/TestAll.ts +++ b/test/TestAll.ts @@ -21,11 +21,16 @@ import ValidatedTextFieldTranslationsSpec from "./ValidatedTextFieldTranslations import CreateCacheSpec from "./CreateCache.spec"; import CodeQualitySpec from "./CodeQuality.spec"; import ImportMultiPolygonSpec from "./ImportMultiPolygon.spec"; +import {ChangesetHandler} from "../Logic/Osm/ChangesetHandler"; +import ChangesetHandlerSpec from "./ChangesetHandler.spec"; +import ChangesSpec from "./Changes.spec"; async function main() { const allTests: T[] = [ + new ChangesSpec(), + new ChangesetHandlerSpec(), new OsmObjectSpec(), new TagSpec(), new ImageAttributionSpec(), @@ -45,7 +50,7 @@ async function main() { new ValidatedTextFieldTranslationsSpec(), new CreateCacheSpec(), new CodeQualitySpec(), - new ImportMultiPolygonSpec() + new ImportMultiPolygonSpec(), ] ScriptUtils.fixUtils(); const realDownloadFunc = Utils.externalDownloadFunction;