Skip to content
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

fix(scenarios): wdpa filters #504

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

kgajowy
Copy link
Contributor

@kgajowy kgajowy commented Aug 27, 2021

image

image

@vercel
Copy link

vercel bot commented Aug 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/8E7Nzbk1oeeXHFE5hCLnf7RmTr8t
✅ Preview: https://marxan-git-fix-marxan-706-find-wpda-in-plann-3d7dbd-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/7vBMVhVrv5twzYMUUXTPibdGmxcz
✅ Preview: https://marxan-storybook-git-fix-marxan-706-find-wpd-3ebf05-vizzuality1.vercel.app

@kgajowy kgajowy requested a review from aagm August 27, 2021 11:04
@hotzevzl
Copy link
Member

hotzevzl commented Sep 2, 2021

I am really unsure about this one - the tests are ok (consistently, for me), but testing things manually gives different results, and not always.

I have created one project in BWA, one in NAM, and got the same set of protected area ids (in fact the PAs visually seem to be all across the border between these countries, so even though unlikely that they're all across the border, this may even be).

Then one project in EGY, and this gave a different set of protected areas.

Then one in BRA, and this resulted in the same set of PA ids as the BWA and NAM ones. Another scenario within this project resulted to the same set as the previous scenario in BRA.

Then ESP, which resulted in a new set of ids.

Then BRA again, which resulted in the same set of ids as the previous one in ESP.

Then BRA again, same as previous one.

But whatever I try in the e2e test (rotating some of these countries in the test spec), the test always passes.

Interestingly (predictably?) the UI does show the protected areas when loading tiles for the country/study area when getting to the first step of protected area selection, but once I choose the relevant IUCN categories and move to step 2/2, only the PU grid appears, with no protected areas anymore (whereas when the set of ids varies when creating a new project, these are shown and I can see the threshold slider affecting the selection of PUs that will be considered "protected").

@hotzevzl
Copy link
Member

hotzevzl commented Sep 2, 2021

further to the previous notes, I have run a bot first with BRA as planning area (and it picked the same set of PA ids as the previous, UI-created Brazil projects/scenarios), then a bot with ESP as planning area, and this picked a different set of PA ids than anything else before. I'm officially 🤯 at this point 😬

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 3, 2021

@hotzevzl so may be this issue is somehow related to UI (or tiles?)

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 3, 2021

@hotzevzl or something happens in asynchronous way (as a side effect) for what, most likely, tests do not wait?

@hotzevzl
Copy link
Member

hotzevzl commented Sep 3, 2021

@hotzevzl so may be this issue is somehow related to UI (or tiles?)

@kgajowy for a moment yesterday I hoped so, but the requests all make sense, it's the sets of calculated PA ids that don't, and the fact that they do so inconsistently (or at least according to a logic/pattern I haven't been able to understand yet) doesn't help. Not ruling this out, but seems unlikely at this stage.

As for side effects... could be, though even here, I haven't been able to identify what may be happening, as after all the core query is a rather simple intersect between planning area and wdpa records 🤔

@hotzevzl
Copy link
Member

@kgajowy see if you think it's worth updating these tests to cover the update case, or what to do with this PR.

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 17, 2021

@kgajowy see if you think it's worth updating these tests to cover the update case, or what to do with this PR.

@hotzevzl updated: b375552

however, the spec is not that much reliable... sometimes if fails, sometimes passes fine (locally as well)

will rebase to develop once we can observer its fail on the CI (as in the current PR's base does not contain fix)

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 17, 2021

Before rebase:
image

@Dyostiq
Copy link
Contributor

Dyostiq commented Sep 20, 2021

maybe waitForExpect could help if there is something asynchronous

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 23, 2021

@hotzevzl after rebasing to develop (which includes fixes) seems to work fine.

@kgajowy kgajowy merged commit fad8ec8 into develop Sep 24, 2021
@kgajowy kgajowy deleted the fix/MARXAN-706-find-wpda-in-planning-area branch September 24, 2021 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants