-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LX-247 Fix lingering polygons and featureId exception #290
Conversation
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. |
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. |
Thanks for reviewing @ms0ur1s ! I've just merged so it will be ready to test on staging in a few mins. |
NIce one @rogup , I'll give it a test. Cool, I'll follow that procedure from now on. |
@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. |
Great work team, spotting and fixing this! |
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:
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