Cleanup of changeset handler, prep for #2082

scripts/handleErrors.ts

@@ -106,7 +106,7 @@ class HandleErrors extends Script {
                 deletedObjects: OsmObject[]
             } = changesObj.CreateChangesetObjects(toUpload, objects)
 
-            const changeset = Changes.createChangesetFor("", changes)
+            const changeset = Changes.buildChangesetXML("", changes)
             const path =
                 "error_changeset_" + parsed.index + "_" + e.layout + "_" + e.username + ".osc"
             if (

scripts/importscripts/cleanRepair.ts

@@ -313,7 +313,7 @@ export default class CleanRepair extends Script {
 
         const changedObjects = changes.CreateChangesetObjects(changesToMake, objects)
 
-        const osc = Changes.createChangesetFor("", changedObjects)
+        const osc = Changes.buildChangesetXML("", changedObjects)
 
         writeFileSync("Cleanup.osc", osc, "utf8")
     }

src/Logic/Osm/Changes.ts

@@ -41,7 +41,7 @@ export class Changes {
     private readonly previouslyCreated: OsmObject[] = []
     private readonly _leftRightSensitive: boolean
     public readonly _changesetHandler: ChangesetHandler
-    private readonly _reportError?: (string: string | Error) => void
+    private readonly _reportError?: (string: string | Error, extramessage?: string) => void
 
     constructor(
         state: {
@@ -53,7 +53,7 @@ export class Changes {
             featureSwitches?: FeatureSwitchState
         },
         leftRightSensitive: boolean = false,
-        reportError?: (string: string | Error) => void
+        reportError?: (string: string | Error, extramessage?: string) => void
     ) {
         this._leftRightSensitive = leftRightSensitive
         // We keep track of all changes just as well
@@ -68,7 +68,7 @@ export class Changes {
             state.osmConnection,
             state.featurePropertiesStore,
             this,
-            (e) => this._reportError(e)
+            (e, extramessage: string) => this._reportError(e, extramessage)
         )
         this.historicalUserLocations = state.historicalUserLocations
 
@@ -76,7 +76,7 @@ export class Changes {
         // This doesn't matter however, as the '-1' is per piecewise upload, not global per changeset
     }
 
-    static createChangesetFor(
+    static buildChangesetXML(
         csId: string,
         allChanges: {
             modifiedObjects: OsmObject[]
@@ -618,14 +618,15 @@ export class Changes {
         openChangeset: UIEventSource<number>
     ): Promise<ChangeDescription[]> {
         const neededIds = Changes.GetNeededIds(pending)
-        // We _do not_ pass in the Changes object itself - we want the data from OSM directly in order to apply the changes
+        /* Download the latest version of the OSM-objects
+        *  We _do not_ pass in the Changes object itself - we want the data from OSM directly in order to apply the changes
+        */
         const downloader = new OsmObjectDownloader(this.backend, undefined)
-        let osmObjects = await Promise.all<{ id: string; osmObj: OsmObject | "deleted" }>(
+        const osmObjects = Utils.NoNull(await Promise.all<{ id: string; osmObj: OsmObject | "deleted" }>(
             neededIds.map((id) => this.getOsmObject(id, downloader))
-        )
+        ))
-
-        osmObjects = Utils.NoNull(osmObjects)
 
+        // Drop changes to deleted items
         for (const { osmObj, id } of osmObjects) {
             if (osmObj === "deleted") {
                 pending = pending.filter((ch) => ch.type + "/" + ch.id !== id)
@@ -645,20 +646,56 @@ export class Changes {
             return undefined
         }
 
+        const metatags = this.buildChangesetTags(pending)
+
+        let { toUpload, refused } = this.fragmentChanges(pending, objects)
+
+        if (toUpload.length === 0) {
+            return refused
+        }
+        await this._changesetHandler.UploadChangeset(
+            (csId, remappings) => {
+                if (remappings.size > 0) {
+                    toUpload = toUpload.map((ch) =>
+                        ChangeDescriptionTools.rewriteIds(ch, remappings)
+                    )
+                }
+
+                const changes: {
+                    newObjects: OsmObject[]
+                    modifiedObjects: OsmObject[]
+                    deletedObjects: OsmObject[]
+                } = this.CreateChangesetObjects(toUpload, objects)
+
+                return Changes.buildChangesetXML("" + csId, changes)
+            },
+            metatags,
+            openChangeset
+        )
+
+        console.log("Upload successful! Refused changes are", refused)
+        return refused
+    }
+
+    /**
+     * Builds all the changeset tags, such as `theme=cyclofix; answer=42; add-image: 5`, ...
+     */
+    private buildChangesetTags(pending: ChangeDescription[]) {
+        // Build statistics for the changeset tags
         const perType = Array.from(
             Utils.Hist(
                 pending
                     .filter(
                         (descr) =>
-                            descr.meta.changeType !== undefined && descr.meta.changeType !== null
+                            descr.meta.changeType !== undefined && descr.meta.changeType !== null,
                     )
-                    .map((descr) => descr.meta.changeType)
+                    .map((descr) => descr.meta.changeType),
             ),
             ([key, count]) => ({
                 key: key,
                 value: count,
                 aggregate: true,
-            })
+            }),
         )
         const motivations = pending
             .filter((descr) => descr.meta.specialMotivation !== undefined)
@@ -697,7 +734,7 @@ export class Changes {
                     value: count,
                     aggregate: true,
                 }
-            })
+            }),
         )
 
         // This method is only called with changedescriptions for this theme
@@ -720,34 +757,7 @@ export class Changes {
             ...motivations,
             ...perBinMessage,
         ]
-
+        return metatags
-        let { toUpload, refused } = this.fragmentChanges(pending, objects)
-
-        if (toUpload.length === 0) {
-            return refused
-        }
-        await this._changesetHandler.UploadChangeset(
-            (csId, remappings) => {
-                if (remappings.size > 0) {
-                    toUpload = toUpload.map((ch) =>
-                        ChangeDescriptionTools.rewriteIds(ch, remappings)
-                    )
-                }
-
-                const changes: {
-                    newObjects: OsmObject[]
-                    modifiedObjects: OsmObject[]
-                    deletedObjects: OsmObject[]
-                } = this.CreateChangesetObjects(toUpload, objects)
-
-                return Changes.createChangesetFor("" + csId, changes)
-            },
-            metatags,
-            openChangeset
-        )
-
-        console.log("Upload successful! Refused changes are", refused)
-        return refused
     }
 
     private async flushChangesAsync(): Promise<void> {

src/Logic/Osm/ChangesetHandler.ts

@@ -13,6 +13,19 @@ export interface ChangesetTag {
     aggregate?: boolean
 }
 
+export type ChangesetMetadata = {
+    type: "changeset"
+    id: number
+    created_at: string
+    open: boolean
+    uid: number
+    user: string
+    changes_count: number
+    tags: Record<string, string>,
+    minlat: number, minlon: number, maxlat: number, maxlon: number
+    comments_count: number
+}
+
 export class ChangesetHandler {
     private readonly allElements: FeaturePropertiesStore
     private osmConnection: OsmConnection
@@ -26,7 +39,7 @@ export class ChangesetHandler {
      * @private
      */
     public readonly _remappings = new Map<string, string>()
-    private readonly _reportError: (e: string | Error) => void
+    private readonly _reportError: (e: string | Error, extramsg: string) => void
 
     constructor(
         dryRun: Store<boolean>,
@@ -36,7 +49,7 @@ export class ChangesetHandler {
             | { addAlias: (id0: string, id1: string) => void }
             | undefined,
         changes: Changes,
-        reportError: (e: string | Error) => void
+        reportError: (e: string | Error, extramessage: string) => void,
     ) {
         this.osmConnection = osmConnection
         this._reportError = reportError
@@ -94,6 +107,27 @@ export class ChangesetHandler {
         return hasChange
     }
 
+    private async UploadWithNew(generateChangeXML: (csid: number, remappings: Map<string, string>) => string, openChangeset: UIEventSource<number>, extraMetaTags: ChangesetTag[]) {
+        const csId = await this.OpenChangeset(extraMetaTags)
+        openChangeset.setData(csId)
+        const changeset = generateChangeXML(csId, this._remappings)
+        console.log(
+            "Opened a new changeset (openChangeset.data is undefined):",
+            changeset,
+            extraMetaTags,
+        )
+        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
+            await this.UpdateTags(csId, extraMetaTags)
+        }
+    }
+
+
     /**
      * The full logic to upload a change to one or more elements.
      *
@@ -107,7 +141,7 @@ export class ChangesetHandler {
     public async UploadChangeset(
         generateChangeXML: (csid: number, remappings: Map<string, string>) => string,
         extraMetaTags: ChangesetTag[],
-        openChangeset: UIEventSource<number>
+        openChangeset: UIEventSource<number>,
     ): Promise<void> {
         if (
             !extraMetaTags.some((tag) => tag.key === "comment") ||
@@ -130,83 +164,58 @@ export class ChangesetHandler {
             return
         }
 
-        if (openChangeset.data === undefined) {
+        if (openChangeset.data) {
-            // We have to open a new changeset
-            try {
-                const csId = await this.OpenChangeset(extraMetaTags)
-                openChangeset.setData(csId)
-                const changeset = generateChangeXML(csId, this._remappings)
-                console.log(
-                    "Opened a new changeset (openChangeset.data is undefined):",
-                    changeset,
-                    extraMetaTags
-                )
-                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
-                    await this.UpdateTags(csId, extraMetaTags)
-                }
-            } catch (e) {
-                if (this._reportError) {
-                    this._reportError(e)
-                }
-                if ((<XMLHttpRequest>e).status === 400) {
-                    // This request is invalid. We simply drop the changes and hope that someone will analyze what went wrong with it in the upload; we pretend everything went fine
-                    return
-                }
-                console.warn(
-                    "Could not open/upload changeset due to ",
-                    e,
-                    "trying again with a another fresh changeset "
-                )
-                openChangeset.setData(undefined)
-
-                throw e
-            }
-        } else {
-            // There still exists an open changeset (or at least we hope so)
-            // Let's check!
-            const csId = openChangeset.data
             try {
+                const csId = openChangeset.data
                 const oldChangesetMeta = await this.GetChangesetMeta(csId)
-                if (!oldChangesetMeta.open) {
+                if (oldChangesetMeta.open) {
-                    // Mark the CS as closed...
+                    // We can hopefully reuse the changeset
-                    console.log("Could not fetch the metadata from the already open changeset")
+
-                    openChangeset.setData(undefined)
+                    try {
-                    // ... and try again. As the cs is closed, no recursive loop can exist
+
-                    await this.UploadChangeset(generateChangeXML, extraMetaTags, openChangeset)
+                        const rewritings = await this.UploadChange(
-                    return
+                            csId,
+                            generateChangeXML(csId, this._remappings),
+                        )
+
+                        const rewrittenTags = this.RewriteTagsOf(
+                            extraMetaTags,
+                            rewritings,
+                            oldChangesetMeta,
+                        )
+                        await this.UpdateTags(csId, rewrittenTags)
+                        return // We are done!
+                    } catch (e) {
+                        this._reportError(e, "While reusing a changeset " + openChangeset.data)
+                    }
+
                 }
-
-                const rewritings = await this.UploadChange(
-                    csId,
-                    generateChangeXML(csId, this._remappings)
-                )
-
-                const rewrittenTags = this.RewriteTagsOf(
-                    extraMetaTags,
-                    rewritings,
-                    oldChangesetMeta
-                )
-                await this.UpdateTags(csId, rewrittenTags)
             } catch (e) {
-                if (this._reportError) {
+                this._reportError(e, "While getting metadata from a changeset " + openChangeset.data)
-                    this._reportError(
-                        "Could not reuse changeset " +
-                            csId +
-                            ", might be closed: " +
-                            (e.stacktrace ?? e.status ?? "" + e)
-                    )
-                }
-                console.warn("Could not upload, changeset is probably closed: ", e)
-                openChangeset.setData(undefined)
-                throw e
             }
         }
+
+
+        // We have to open a new changeset
+        try {
+            return await this.UploadWithNew(generateChangeXML, openChangeset, extraMetaTags)
+        } catch (e) {
+            if (this._reportError) {
+                this._reportError(e, "While opening a new changeset")
+            }
+            if ((<XMLHttpRequest>e).status === 400) {
+                // This request is invalid. We simply drop the changes and hope that someone will analyze what went wrong with it in the upload; we pretend everything went fine
+                return
+            }
+            console.warn(
+                "Could not open/upload changeset due to ",
+                e,
+                "trying again with a another fresh changeset ",
+            )
+            openChangeset.setData(undefined)
+
+            throw e
+        }
     }
 
     /**
@@ -227,7 +236,7 @@ export class ChangesetHandler {
             uid: number // User ID
             changes_count: number
             tags: any
-        }
+        },
     ): ChangesetTag[] {
         // Note: extraMetaTags is where all the tags are collected into
 
@@ -346,15 +355,9 @@ export class ChangesetHandler {
         console.log("Closed changeset ", changesetId)
     }
 
-    private async GetChangesetMeta(csId: number): Promise<{
+    private async GetChangesetMeta(csId: number): Promise<ChangesetMetadata> {
-        id: number
-        open: boolean
-        uid: number
-        changes_count: number
-        tags: any
-    }> {
         const url = `${this.backend}/api/0.6/changeset/${csId}`
-        const csData = await Utils.downloadJson(url)
+        const csData = await Utils.downloadJson<{ elements: ChangesetMetadata[] }>(url)
         return csData.elements[0]
     }
 
@@ -370,7 +373,7 @@ export class ChangesetHandler {
                 tag.key !== undefined &&
                 tag.value !== undefined &&
                 tag.key !== "" &&
-                tag.value !== ""
+                tag.value !== "",
         )
         const metadata = tags.map((kv) => `<tag k="${kv.key}" v="${escapeHtml(kv.value)}"/>`)
         const content = [`<osm><changeset>`, metadata, `</changeset></osm>`].join("")
@@ -410,7 +413,7 @@ export class ChangesetHandler {
         const csId = await this.osmConnection.put(
             "changeset/create",
             [`<osm><changeset>`, metadata, `</changeset></osm>`].join(""),
-            { "Content-Type": "text/xml" }
+            { "Content-Type": "text/xml" },
         )
         return Number(csId)
     }
@@ -420,12 +423,12 @@ export class ChangesetHandler {
      */
     private async UploadChange(
         changesetId: number,
-        changesetXML: string
+        changesetXML: string,
     ): Promise<Map<string, string>> {
         const response = await this.osmConnection.post<XMLDocument>(
             "changeset/" + changesetId + "/upload",
             changesetXML,
-            { "Content-Type": "text/xml" }
+            { "Content-Type": "text/xml" },
         )
         const changes = this.parseUploadChangesetResponse(response)
         console.log("Uploaded changeset ", changesetId)

src/Models/ThemeViewState.ts

@@ -278,7 +278,7 @@ export default class ThemeViewState implements SpecialVisualizationState {
                     featureSwitches: this.featureSwitches,
                 },
                 layout?.isLeftRightSensitive() ?? false,
-                (e) => this.reportError(e),
+                (e, extraMsg) => this.reportError(e, extraMsg),
             )
             this.historicalUserLocations = this.geolocation.historicalUserLocations
             this.newFeatures = new NewGeometryFromChangesFeatureSource(
@@ -650,9 +650,9 @@ export default class ThemeViewState implements SpecialVisualizationState {
                         available,
                         category,
                         current.data,
-                        skipLayers
+                        skipLayers,
                     )
-                    if(!best){
+                    if (!best) {
                         return
                     }
                     console.log("Best layer for category", category, "is", best?.properties?.id)
@@ -680,19 +680,19 @@ export default class ThemeViewState implements SpecialVisualizationState {
             Hotkeys.RegisterHotkey(
                 { shift: "O" },
                 Translations.t.hotkeyDocumentation.selectOsmbasedmap,
-                () => setLayerCategory("osmbasedmap",2),
+                () => setLayerCategory("osmbasedmap", 2),
             )
 
             Hotkeys.RegisterHotkey(
                 { shift: "M" },
                 Translations.t.hotkeyDocumentation.selectMap,
-                () => setLayerCategory("map",2),
+                () => setLayerCategory("map", 2),
             )
 
             Hotkeys.RegisterHotkey(
                 { shift: "P" },
                 Translations.t.hotkeyDocumentation.selectAerial,
-                () => setLayerCategory("photo",2),
+                () => setLayerCategory("photo", 2),
             )
             Hotkeys.RegisterHotkey(
                 { nomod: "L" },
@@ -907,7 +907,7 @@ export default class ThemeViewState implements SpecialVisualizationState {
         this.selectedElement.setData(this.currentView.features?.data?.[0])
     }
 
-    public async reportError(message: string | Error | XMLHttpRequest) {
+    public async reportError(message: string | Error | XMLHttpRequest, extramessage: string = "") {
         const isTesting = this.featureSwitchIsTesting.data
         console.log(
             isTesting
@@ -922,7 +922,17 @@ export default class ThemeViewState implements SpecialVisualizationState {
 
         if ("" + message === "[object XMLHttpRequest]") {
             const req = <XMLHttpRequest>message
-            message = "XMLHttpRequest with status code " + req.status + ", " + req.statusText
+            let body = ""
+            try {
+                body = req.responseText
+            } catch (e) {
+                // pass
+            }
+            message = "XMLHttpRequest with status code " + req.status + ", " + req.statusText + ", received: " + body
+        }
+
+        if (extramessage) {
+            message += "(" + extramessage + ")"
         }
 
         const stacktrace: string = new Error().stack