From ab966722f92fa3a8459bfee5ea3aa3e3b83da62e Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 25 Jan 2024 12:19:41 +0100 Subject: [PATCH] wip: rework request error flow --- umap/models.py | 7 ++ umap/static/umap/js/modules/global.js | 4 +- umap/static/umap/js/modules/request.js | 144 ++++++++++++------------- umap/static/umap/js/umap.js | 10 +- umap/static/umap/js/umap.layer.js | 42 ++++++-- umap/tests/test_views.py | 10 -- umap/views.py | 6 +- 7 files changed, 115 insertions(+), 108 deletions(-) diff --git a/umap/models.py b/umap/models.py index d63f00fd..f8c31238 100644 --- a/umap/models.py +++ b/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 diff --git a/umap/static/umap/js/modules/global.js b/umap/static/umap/js/modules/global.js index 049a18ed..f58b8c50 100644 --- a/umap/static/umap/js/modules/global.js +++ b/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 } diff --git a/umap/static/umap/js/modules/request.js b/umap/static/umap/js/modules/request.js index 54347394..5a06a27e 100644 --- a/umap/static/umap/js/modules/request.js +++ b/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) - return response + + _onNok: function (error) { + 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) { - return [{}, null] + 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' }) + return this._onError(new Error(data.error)) } return [data, response, null] } catch (error) { - this._onError(error) - return [{}, null, error] + return this._onError(error) } }, - _handle_server_instructions: function (data) { - // Generic cases, let's deal with them once - 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 - } + _onError: function (error) { + return [{}, null, error] }, - onNok: function (status, message) { - if (status === 403) { + _onNok: function (error) { + 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] }, }) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index f5ce6c88..a794b778 100644 --- a/umap/static/umap/js/umap.js +++ b/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 } }, diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index cbb17f20..3c6877d9 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/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( - saveUrl, - headers, - formData - ) + await this._trySave(saveUrl, headers, formData) + this._geojson = geojson + }, + + _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['Last-Modified'] + this._last_modified = response.headers.get('last-modified') this.setUmapId(data.id) this.updateOptions(data) this.backupOptions() diff --git a/umap/tests/test_views.py b/umap/tests/test_views.py index 8a1fa724..86904cdc 100644 --- a/umap/tests/test_views.py +++ b/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") diff --git a/umap/views.py b/umap/views.py index b01eedc0..c6a4cede 100644 --- a/umap/views.py +++ b/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("/")