DigitalCommons / land-explorer-front-end

React app for the Land Explorer front end
http://landexplorer.cc
GNU Affero General Public License v3.0
2 stars 0 forks source link

LX-247 Fix lingering polygons and featureId exception #290

Closed rogup closed 11 months ago

rogup commented 11 months ago

What? Why?

Closes #247

We are not redrawing the polygons properly when switching maps or opening new maps. This is causing old polygons to linger but not be selectable/editable. And it means that polygons in the new map throw an error when trying to edit them, since their feature IDs are not registered with mapbox.

I was able to reproduce this on both development and production by following these steps:

  1. Draw a polygon
  2. Open a map containing a different polygon
  3. The first polygon lingers but is not selectable. The other polygon throws an error if you select it then click 'Edit'. (see logs in production since it throws an exception gracefully)

I'm pretty sure this is from all the refactoring work a few months ago (replacing component props with Redux and functionalising the Mapbox component), maybe mine oops. It's pretty serious, it seems basically most polygon functionality is broken...

To fix, I have ensured that we redraw polygons when the current map ID changes. And to cover the case when an unsaved map is replaced with a new map (so map ID remains null), I have introduced a temporary 'unsavedMapUuid' to distinguish between unsaved maps and also trigger a redraw. It feels a bit clanky to have such a variable in the redux store for just this purpose, but I couldn't think of a cleaner way to achieve this without adding a complicated sequence of Redux state changes.

What should we test?

Release notes

I think this should go into the next release so that polygon functionality in prod is fixed ASAP

Deployment notes

Documentation updates

rogup commented 11 months ago

Hey @ms0ur1s good spot for this bug - please could you give my fix a review?

Also, maybe once it's merged, it would be worth you trying to repro the original scenario that found the bug on staging? I'm pretty sure I didn't follow all the steps completely accurately but ended up hitting the featureId exception by just playing around with some of my own maps.

ms0ur1s commented 11 months ago

Nice one @rogup, great fix. Good to get some insight into the causes of the error, as maps and polygons are still new to me.

I'll give this a review now and will defo give a test once it's merged in.

rogup commented 11 months ago

Thanks for reviewing @ms0ur1s ! I've just merged so it will be ready to test on staging in a few mins. Also, just to let you know, feel free to merge and move to QA any PRs that I send to you for review, once they're approved

ms0ur1s commented 11 months ago

NIce one @rogup , I'll give it a test.

Cool, I'll follow that procedure from now on.

ms0ur1s commented 11 months ago

@rogup, tested on development branch using three different accounts (two existing and one new), each with multiple saved maps with custom polygons.

Works perfectly, all tests passed on dev, ready for staging.

lin-d-hop commented 10 months ago

Great work team, spotting and fixing this! LGTM :)