DigitalCommons / mykomap

A web application for mapping initiatives in the Solidarity Economy
3 stars 0 forks source link

Mykomap filter selection change appears to unselect everything #176

Closed wu-lee closed 1 year ago

wu-lee commented 1 year ago

Version: mykomap@3.0.4_677b5e7-modified-dev Deployment: Owned By Oxford public - https://dev.maps.solidarityeconomy.coop/temp/

Browsers:

To replicate:

Note: If you select 'Education" a second time, it seems to work.

ColmDC commented 1 year ago

This is still an issue in mykomap@3.0.4_0fcdb90-dev

wu-lee commented 1 year ago

Delving into the code, it looks like the text search has to be applied to the vocab filter as a separate step afterwards. If the vocab filter is changed, the text search is re-applied, but if this is for the empty string, this resets the filter!

There seems to be other bugs in the text search / vocab filter logic. The problem seems to related that the text search and vocab filters being done in separate steps, and sometimes the former zaps the latter. e.g. On the temp Coops UK demo here;

Another bug:

These interactions can be complex, and we need a way to test and validate the logic. Currently the logic is interleaved with the UI code, so untestable except via the web page, needing manual testing, or some heavyweight testing framework like Selenium (which tend to be a bit slow and unreliable).

I think what we need is to make the logic for this self-contained and testable without the UI. This means it can be automated more easily. And have that self-contained thing the model driving the UI - there might still be bugs in the UI but at least then the underlying logic is validated.

However, that would need some fairly major refactoring, somewhat along the lines of what I've been doing.

Without that, however, I worry that the map is quite bug-prone, liable to regressions.

ColmDC commented 1 year ago

Currently the logic is interleaved with the UI code,

That's frustrating.

However, that would need some fairly major refactoring, somewhat along the lines of what I've been doing.

Not sure we can afford the time for another refactor at this point. How much complexity does the history add to the code? I wonder if we disable it for the moment and got a history free version working reliably without a refactor, and reintroduce it later when we have some time.

wu-lee commented 1 year ago

Possibly - but even to do that needs a bit of figuring out and refactoring. I'll see what I can do without sending myself on another mission.

wu-lee commented 1 year ago

Ok, this bug seems to be fixed...

MapPresenter.applyFilter() checks for filters, and if it thinks there are none, calls .removeFilters(). Unclear why - the logic is a bit loopy to follow - but in this case, the check is wrong - it calls .length on an object, which isn't an array, so it gets 0 even if it isn't empty and always removes the filters.

The correct thing to do is call the right method which gets something easy to get the emptiness of (without explicitly calling Object.keys). .getFilters() does that already and returns an array.

wu-lee commented 1 year ago

This fixed version deployed here:

You should see the version mykomap@3.0.4_7152cd6-dev

ColmDC commented 1 year ago

I'm seeing no change in behaviour on https://dev.maps.solidarityeconomy.coop/temp/obo-public/ when replicating the original bug test steps.

ColmDC commented 1 year ago

Seems to work well now on mykomap@3.0.4_c0ffa8f-dev