Skip to content

Commit

Permalink
fixup: make sure to redraw tile after a .umap import (#2347)
Browse files Browse the repository at this point in the history
Broken since
20b2290

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 20b2290 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
  • Loading branch information
yohanboniface authored Dec 9, 2024
2 parents a7b714c + f3c95bd commit d0156bc
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 18 deletions.
1 change: 0 additions & 1 deletion umap/static/umap/js/modules/sync/updaters.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ export class DataLayerUpdater extends BaseUpdater {
upsert({ value }) {
// Upsert only happens when a new datalayer is created.
this._umap.createDataLayer(value, false)
this._umap.render([])
}

update({ key, metadata, value }) {
Expand Down
25 changes: 12 additions & 13 deletions umap/static/umap/js/modules/umap.js
Original file line number Diff line number Diff line change
Expand Up @@ -1261,9 +1261,10 @@ export default class Umap extends ServerStored {
}
}

render(fields) {
const impacted = this.propagate(fields)
if (impacted) return // No need to run a wider reflow
render(fields = []) {
// Propagate will remove the fields it has already
// processed
fields = this.propagate(fields)

const impacts = Utils.getImpactsFromSchema(fields)
for (const impact of impacts) {
Expand Down Expand Up @@ -1327,14 +1328,13 @@ export default class Umap extends ServerStored {
})
},
}
let impacted = false
for (const [field, impact] of Object.entries(impacts)) {
if (!fields.length || fields.includes(field)) {
impact()
impacted = true
fields = fields.filter((item) => item !== field)
}
}
return impacted
return fields
}

// TODO: allow to control the default datalayer
Expand Down Expand Up @@ -1537,7 +1537,6 @@ export default class Umap extends ServerStored {

importRaw(rawData) {
const importedData = JSON.parse(rawData)
const mustReindex = 'sortKey' in Object.keys(importedData.properties)

this.setProperties(importedData.properties)

Expand All @@ -1554,12 +1553,12 @@ export default class Umap extends ServerStored {
dataLayer.fromUmapGeoJSON(geojson)
}

this._leafletMap.renderUI()
this.eachDataLayer((datalayer) => {
if (mustReindex) datalayer.reindex()
datalayer.redraw()
})
this.propagate()
// For now render->propagate expect a `properties.` prefix.
// Remove this when we have refactored schema and render.
const fields = Object.keys(importedData.properties).map(
(field) => `properties.${field}`
)
this.render(fields)
this._leafletMap._setDefaultCenter()
this.isDirty = true
}
Expand Down
3 changes: 2 additions & 1 deletion umap/tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class Meta:

class TileLayerFactory(factory.django.DjangoModelFactory):
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"

class Meta:
Expand Down Expand Up @@ -156,5 +156,6 @@ def login_required(response):


def mock_tiles(route):
print("Intercepted route", route.request.url)
path = Path(__file__).parent / "fixtures/empty_tile.png"
route.fulfill(path=path)
3 changes: 2 additions & 1 deletion umap/tests/integration/conftest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import re
import subprocess
import time
from pathlib import Path
Expand All @@ -25,7 +26,7 @@ def set_timeout(context):
@pytest.fixture(autouse=True)
def mock_osm_tiles(page):
if not bool(os.environ.get("PWDEBUG", False)):
page.route("*/**/osmfr/**", mock_tiles)
page.route(re.compile(r".*tile\..*"), mock_tiles)


@pytest.fixture
Expand Down
2 changes: 0 additions & 2 deletions umap/tests/integration/test_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
page.route("https://tile.openstreetmap.fr/hot/**", mock_tiles)

settings.UMAP_ALLOW_ANONYMOUS = True
page.goto(f"{live_server.url}/map/new/")
page.get_by_role("button", name="Open browser").click()
Expand Down

0 comments on commit d0156bc

Please sign in to comment.