fixup: make sure to redraw tile after a .umap import (#2347)
Some checks are pending
Test & Docs / tests (postgresql, 3.10) (push) Waiting to run
Test & Docs / tests (postgresql, 3.12) (push) Waiting to run
Test & Docs / lint (push) Waiting to run
Test & Docs / docs (push) Waiting to run

Broken since
20b2290d00

Since started as a simple fix, but:
- I first thought my previous fix of the failing test
`test_import_umap_from_textarea` was not a real fix, so I changed a bit
the way we mock tiles URL in tests, but at the end the test was failing
for good reasons
- since 20b2290d00 the reset of tilelayer
was not called anymore after importing a umap file, so I first made a
quick fix for this
- then I decided to refactor a bit more render and propagate, so
`importRaw` would pass the exact imported properties (instead of trying
to blindly target with some properties), and to remove a call to
`propagate`, which at the end should disappear in favor of `render` with
better targeting
This commit is contained in:
Yohan Boniface 2024-12-09 13:12:56 +01:00 committed by GitHub
commit d0156bc7a6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 16 additions and 18 deletions

View file

@ -55,7 +55,6 @@ export class DataLayerUpdater extends BaseUpdater {
upsert({ value }) { upsert({ value }) {
// Upsert only happens when a new datalayer is created. // Upsert only happens when a new datalayer is created.
this._umap.createDataLayer(value, false) this._umap.createDataLayer(value, false)
this._umap.render([])
} }
update({ key, metadata, value }) { update({ key, metadata, value }) {

View file

@ -1261,9 +1261,10 @@ export default class Umap extends ServerStored {
} }
} }
render(fields) { render(fields = []) {
const impacted = this.propagate(fields) // Propagate will remove the fields it has already
if (impacted) return // No need to run a wider reflow // processed
fields = this.propagate(fields)
const impacts = Utils.getImpactsFromSchema(fields) const impacts = Utils.getImpactsFromSchema(fields)
for (const impact of impacts) { for (const impact of impacts) {
@ -1327,14 +1328,13 @@ export default class Umap extends ServerStored {
}) })
}, },
} }
let impacted = false
for (const [field, impact] of Object.entries(impacts)) { for (const [field, impact] of Object.entries(impacts)) {
if (!fields.length || fields.includes(field)) { if (!fields.length || fields.includes(field)) {
impact() impact()
impacted = true fields = fields.filter((item) => item !== field)
} }
} }
return impacted return fields
} }
// TODO: allow to control the default datalayer // TODO: allow to control the default datalayer
@ -1537,7 +1537,6 @@ export default class Umap extends ServerStored {
importRaw(rawData) { importRaw(rawData) {
const importedData = JSON.parse(rawData) const importedData = JSON.parse(rawData)
const mustReindex = 'sortKey' in Object.keys(importedData.properties)
this.setProperties(importedData.properties) this.setProperties(importedData.properties)
@ -1554,12 +1553,12 @@ export default class Umap extends ServerStored {
dataLayer.fromUmapGeoJSON(geojson) dataLayer.fromUmapGeoJSON(geojson)
} }
this._leafletMap.renderUI() // For now render->propagate expect a `properties.` prefix.
this.eachDataLayer((datalayer) => { // Remove this when we have refactored schema and render.
if (mustReindex) datalayer.reindex() const fields = Object.keys(importedData.properties).map(
datalayer.redraw() (field) => `properties.${field}`
}) )
this.propagate() this.render(fields)
this._leafletMap._setDefaultCenter() this._leafletMap._setDefaultCenter()
this.isDirty = true this.isDirty = true
} }

View file

@ -42,7 +42,7 @@ class LicenceFactory(factory.django.DjangoModelFactory):
class TileLayerFactory(factory.django.DjangoModelFactory): class TileLayerFactory(factory.django.DjangoModelFactory):
name = "Test zoom layer" name = "Test zoom layer"
url_template = "https://{s}.test.org/osmfr/{z}/{x}/{y}.png" url_template = "https://tile.openstreetmap.org/{z}/{x}/{y}.png"
attribution = "Test layer attribution" attribution = "Test layer attribution"
class Meta: class Meta:
@ -156,5 +156,6 @@ def login_required(response):
def mock_tiles(route): def mock_tiles(route):
print("Intercepted route", route.request.url)
path = Path(__file__).parent / "fixtures/empty_tile.png" path = Path(__file__).parent / "fixtures/empty_tile.png"
route.fulfill(path=path) route.fulfill(path=path)

View file

@ -1,4 +1,5 @@
import os import os
import re
import subprocess import subprocess
import time import time
from pathlib import Path from pathlib import Path
@ -25,7 +26,7 @@ def set_timeout(context):
@pytest.fixture(autouse=True) @pytest.fixture(autouse=True)
def mock_osm_tiles(page): def mock_osm_tiles(page):
if not bool(os.environ.get("PWDEBUG", False)): if not bool(os.environ.get("PWDEBUG", False)):
page.route("*/**/osmfr/**", mock_tiles) page.route(re.compile(r".*tile\..*"), mock_tiles)
@pytest.fixture @pytest.fixture

View file

@ -72,8 +72,6 @@ def test_umap_import_from_file(live_server, tilelayer, page):
def test_umap_import_from_textarea(live_server, tilelayer, page, settings): def test_umap_import_from_textarea(live_server, tilelayer, page, settings):
page.route("https://tile.openstreetmap.fr/hot/**", mock_tiles)
settings.UMAP_ALLOW_ANONYMOUS = True settings.UMAP_ALLOW_ANONYMOUS = True
page.goto(f"{live_server.url}/map/new/") page.goto(f"{live_server.url}/map/new/")
page.get_by_role("button", name="Open browser").click() page.get_by_role("button", name="Open browser").click()