wip: rework request error flow

umap/models.py

@@ -242,6 +242,13 @@ class Map(NamedModel):
             has_anonymous_cookie = False
         return has_anonymous_cookie
 
+    def can_delete(self, user=None, request=None):
+        if self.owner and user != self.owner:
+            return False
+        if not self.owner and not self.is_anonymous_owner(request):
+            return False
+        return True
+
     def can_edit(self, user=None, request=None):
         """
         Define if a user can edit or not the instance, according to his account

umap/static/umap/js/modules/global.js

@@ -1,9 +1,9 @@
 import * as L from '../../vendors/leaflet/leaflet-src.esm.js'
 import URLs from './urls.js'
-import { Request, ServerRequest } from './request.js'
+import { Request, ServerRequest, HTTPError, NokError } from './request.js'
 // Import modules and export them to the global scope.
 // For the not yet module-compatible JS out there.
 
 // Copy the leaflet module, it's expected by leaflet plugins to be writeable.
 window.L = { ...L }
-window.umap = { URLs, Request, ServerRequest }
+window.umap = { URLs, Request, ServerRequest, HTTPError, NokError }

umap/static/umap/js/modules/request.js

@@ -1,6 +1,22 @@
 // Uses `L._`` from Leaflet.i18n which we cannot import as a module yet
 import { Evented, DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js'
 
+export class HTTPError extends Error {
+  constructor(message) {
+    super(message)
+    this.name = this.constructor.name
+  }
+}
+
+export class NokError extends Error {
+  constructor(response) {
+    super(response.status)
+    this.response = response
+    this.status = response.status
+    this.name = this.constructor.name
+  }
+}
+
 const BaseRequest = Evented.extend({
   _fetch: async function (method, uri, headers, data) {
     const id = Math.random()
@@ -15,13 +31,13 @@ const BaseRequest = Evented.extend({
         body: data,
       })
     } catch (error) {
-      this._onError(error)
+      console.error(error)
       this.fire('dataload', { id: id })
-      return null
+      throw new HTTPError(error.message)
     }
     if (!response.ok) {
       this.fire('dataload', { id: id })
-      return this.onNok(response.status, response)
+      throw new NokError(response.status)
     }
     // TODO
     // - error handling
@@ -30,6 +46,32 @@ const BaseRequest = Evented.extend({
     this.fire('dataload', { id: id })
     return response
   },
+})
+
+// Basic class to issue request
+// It returns a response, or null in case of error
+// In case of error, an alert is sent, but non 20X status are not handled
+// The consumer must check the response status by hand
+export const Request = BaseRequest.extend({
+  initialize: function (ui) {
+    this.ui = ui
+  },
+
+  _fetch: async function (method, uri, headers, data) {
+    try {
+      const response = await BaseRequest.prototype._fetch.call(
+        this,
+        method,
+        uri,
+        headers,
+        data
+      )
+      return response
+    } catch (error) {
+      if (error instanceof NokError) return this._onNok(error)
+      return this._onError(error)
+    }
+  },
 
   get: async function (uri, headers) {
     return await this._fetch('GET', uri, headers)
@@ -40,31 +82,21 @@ const BaseRequest = Evented.extend({
   },
 
   _onError: function (error) {
-    console.error(error)
-    this.onError(error)
-  },
-  onError: function (error) {},
-  onNok: function (status, reponse) {
-    return response
-  },
-})
-
-export const Request = BaseRequest.extend({
-  initialize: function (ui) {
-    this.ui = ui
-  },
-  onError: function (error) {
-    console.error(error)
     this.ui.alert({ content: L._('Problem in the response'), level: 'error' })
   },
-  onNok: function (status, response) {
+
-    this.onError(message)
+  _onNok: function (error) {
-    return response
+    this._onError(error)
+    return error.response
   },
+
 })
 
 // Adds uMap specifics to requests handling
 // like logging, CSRF, etc.
+// It expects only json responses.
+// Returns an array of three elements: [data, response, error]
+// The consumer must check the error to proceed or not with using the data or response
 export const ServerRequest = Request.extend({
   _fetch: async function (method, uri, headers, data) {
     // Add a flag so backend can know we are in ajax and adapt the response
@@ -93,77 +125,33 @@ export const ServerRequest = Request.extend({
   },
 
   _as_json: async function (response) {
-    if (!response) return [{}, null, new Error("Undefined error")]
+    if (Array.isArray(response)) return response
     try {
       const data = await response.json()
-      if (this._handle_server_instructions(data) !== false) {
+      if (data.info) {
-        return [{}, null]
+        this.ui.alert({ content: data.info, level: 'info' })
+        this.ui.closePanel()
+      } else if (data.error) {
+        this.ui.alert({ content: data.error, level: 'error' })
+        return this._onError(new Error(data.error))
       }
       return [data, response, null]
     } catch (error) {
-      this._onError(error)
+      return this._onError(error)
-      return [{}, null, error]
     }
   },
 
-  _handle_server_instructions: function (data) {
+  _onError: function (error) {
-    // Generic cases, let's deal with them once
+    return [{}, null, error]
-    if (data.redirect) {
-      const newPath = data.redirect
-      if (window.location.pathname == newPath) {
-        window.location.reload() // Keep the hash, so the current view
-      } else {
-        window.location = newPath
-      }
-    } else if (data.info) {
-      this.ui.alert({ content: data.info, level: 'info' })
-      this.ui.closePanel()
-    } else if (data.error) {
-      this.ui.alert({ content: data.error, level: 'error' })
-    } else if (data.login_required) {
-      // TODO: stop flow and run request again when user
-      // is logged in
-      const win = window.open(data.login_required)
-      window.umap_proceed = () => {
-        console.log('logged in')
-        this.fire('login')
-        win.close()
-      }
-    } else {
-      // Nothing to do, we can let the response proceed
-      return false
-    }
   },
 
-  onNok: function (status, message) {
+  _onNok: function (error) {
-    if (status === 403) {
+    if (error.status === 403) {
       this.ui.alert({
         content: message || L._('Action not allowed :('),
         level: 'error',
       })
-    } else if (status === 412) {
-      const msg = L._(
-        'Woops! Someone else seems to have edited the data. You can save anyway, but this will erase the changes made by others.'
-      )
-      const actions = [
-        {
-          label: L._('Save anyway'),
-          callback: function () {
-            // TODO
-            delete settings.headers['If-Match']
-            this._fetch(settings)
-          },
-        },
-        {
-          label: L._('Cancel'),
-        },
-      ]
-      this.ui.alert({
-        content: msg,
-        level: 'error',
-        duration: 100000,
-        actions: actions,
-      })
     }
+    return [{}, error.response, error]
   },
 })

umap/static/umap/js/umap.js

@@ -1797,19 +1797,21 @@ L.U.Map.include({
     return this.editTools.startPolygon()
   },
 
-  del: function () {
+  del: async function () {
     if (confirm(L._('Are you sure you want to delete this map?'))) {
       const url = this.urls.get('map_delete', { map_id: this.options.umap_id })
-      this.post(url)
+      const [data, response, error] = await this.server.post(url)
+      if (data.redirect) window.location = data.redirect
     }
   },
 
-  clone: function () {
+  clone: async function () {
     if (
       confirm(L._('Are you sure you want to clone this map and all its datalayers?'))
     ) {
       const url = this.urls.get('map_clone', { map_id: this.options.umap_id })
-      this.post(url)
+      const [data, response, error] = await this.server.post(url)
+      if (data.redirect) window.location = data.redirect
     }
   },
 

umap/static/umap/js/umap.layer.js

@@ -676,7 +676,7 @@ L.U.DataLayer = L.Evented.extend({
     this._loading = true
     const [geojson, response, error] = await this.map.server.get(this._dataUrl())
     if (!error) {
-      this._last_modified = response.headers['Last-Modified']
+      this._last_modified = response.headers.get('last-modified')
       // FIXME: for now this property is set dynamically from backend
       // And thus it's not in the geojson file in the server
       // So do not let all options to be reset
@@ -1557,21 +1557,45 @@ L.U.DataLayer = L.Evented.extend({
     const headers = this._last_modified
       ? { 'If-Unmodified-Since': this._last_modified }
       : {}
-    const [data, response, error] = await this.map.server.post(
+    await this._trySave(saveUrl, headers, formData)
-      saveUrl,
+    this._geojson = geojson
-      headers,
+  },
-      formData
+
-    )
+  _trySave: async function (url, headers, formData) {
+    const [data, response, error] = await this.map.server.post(url, headers, formData)
     if (!error) {
+      if (response.status === 412) {
+        const msg = L._(
+          'Woops! Someone else seems to have edited the data. You can save anyway, but this will erase the changes made by others.'
+        )
+        const actions = [
+          {
+            label: L._('Save anyway'),
+            callback: async () => {
+              // Save again,
+              // but do not pass If-Unmodified-Since this time
+              await this._trySave(url, {}, formData)
+            },
+          },
+          {
+            label: L._('Cancel'),
+          },
+        ]
+        this.ui.alert({
+          content: msg,
+          level: 'error',
+          duration: 100000,
+          actions: actions,
+        })
+      }
       // Response contains geojson only if save has conflicted and conflicts have
-      // been resolved. So we need to reload to get extra data (saved from someone else)
+      // been resolved. So we need to reload to get extra data (added by someone else)
       if (data.geojson) {
         this.clear()
         this.fromGeoJSON(data.geojson)
         delete data.geojson
       }
-      this._geojson = geojson
+      this._last_modified = response.headers.get('last-modified')
-      this._last_modified = response.headers['Last-Modified']
       this.setUmapId(data.id)
       this.updateOptions(data)
       this.backupOptions()

umap/tests/test_views.py

@@ -304,16 +304,6 @@ def test_user_dashboard_display_user_maps_distinct(client, map):
     assert body.count(anonymap.name) == 0
 
 
-@pytest.mark.django_db
-def test_logout_should_return_json_in_ajax(client, user, settings):
-    client.login(username=user.username, password="123123")
-    response = client.get(
-        reverse("logout"), headers={"X_REQUESTED_WITH": "XMLHttpRequest"}
-    )
-    data = json.loads(response.content.decode())
-    assert data["redirect"] == "/"
-
-
 @pytest.mark.django_db
 def test_logout_should_return_redirect(client, user, settings):
     client.login(username=user.username, password="123123")

umap/views.py

@@ -830,10 +830,8 @@ class MapDelete(DeleteView):
 
     def form_valid(self, form):
         self.object = self.get_object()
-        if self.object.owner and self.request.user != self.object.owner:
+        if not self.object.can_delete(self.request.user, self.request):
             return HttpResponseForbidden(_("Only its owner can delete the map."))
-        if not self.object.owner and not self.object.is_anonymous_owner(self.request):
-            return HttpResponseForbidden()
         self.object.delete()
         return simple_json_response(redirect="/")
 
@@ -1178,8 +1176,6 @@ def webmanifest(request):
 
 def logout(request):
     do_logout(request)
-    if is_ajax(request):
-        return simple_json_response(redirect="/")
     return HttpResponseRedirect("/")