Analysis Module

[Ry-DS]

Test Deployment on: http://prism-analysis-ui-test.surge.sh/

Closes #86
Continuation: #98

Possible Discussion points

• Clicking a table row opens a popup at the corresponding part of the map. What data should it show? Should we keep it?
• I added the color code to the legend since its easily accessible. Should it be kept? If not, should we try to make a more accurate color code or remove it entirely in favor of just the names of the hazard and baseline layers?
• We don't flip any switches off when the analysis layer is shown. Should we?
• AnalysisUI test still needs LayerDefinitions to be mocked otherwise it might fail if layers.json is changed due to mismatching snapshots. fixed by removing defaults

QOL Changes

• Made a universal LayerKey type definition. Most places have been changed to rely on this type instead of generic string
• Add admin_level_local_names with admin_level_names in layers.json
• Properly typed and validated admin_level in NSO to be a number
• Rename fetchNsoLayerData->fetch*NSO*LayerData
• Changed a few small things for better readablility such as comments and refactoring (example) (I'll link all instances here later for review just in case I made a mistake!)
• Made AggregationOperations an enum for easier typing
• Updated dependencies to try solve a few issues during development
• Made a flexible LayerDropdown, made into an individual component for easier testing and readability. Could be used elsewhere
• Add description to package.json, doesn't really do anything but its used in some IDEs when displaying personal project lists

[ericboucher]

@SimplyBallistic @laveesh I just merged #94 which should make it a bit clearer / easier to see what's happening when we merge the data

[Ry-DS]

Update: The below cycles have been fixed

New dependency cycles to fix introduced before lint check on #99

There's a really big loop:

analysisResultStateSlice->layer-data->impact->analysis-utils->analysisResultStateSlice

analysisResultStateSlice->layer-data->impact is unbreakable - strongly coupled.

breaking impact->analysis-utils or analysis-utils->analysisResultStateSlice needs to somehow be done

also:
analysisResultStateSlice->analysis-utils->analysisResultStateSlice

[ericboucher]

@SimplyBallistic nice work! Here are a few answers to your questions:

• Clicking a table row opens a popup at the corresponding part of the map. What data should it show? Should we keep it?

-> it should show the same data as clicking on the map

I added the color code to the legend since its easily accessible. Should it be kept? If not, should we try to make a more accurate color code or remove it entirely in favor of just the names of the hazard and baseline layers?

-> This is actually the baseline layer, so it should reflect that be the same as putting a baseline layer only

We don't flip any switches off when the analysis layer is shown. Should we?

-> maybe switch the impact layer if there is one?

[Ry-DS]

it should show the same data as clicking on the map\

We could add this in #98 as an enhancement to keep this PR manageable

This is actually the baseline layer, so it should reflect that be the same as putting a baseline layer only

If I understand correctly, I believe the legend already does this, so we're all good here! 👍

maybe switch the impact layer if there is one?

I'll make a quick commit to add this, or if @laveesh is interested in doing this you could do it here or on #98

#98 is purely for enhancements for now; we hope to get a fully functional Analysis UI out within this PR. However, if you believe point 1, 3 or anything else is needed for proper use of the UI let us know and we'll continue working here :)

[ericboucher]

I agree that we should wrap up this PR @SimplyBallistic :). I would just push on the legend and make sure that it is clear that we are seeing the baseline data.

This is what we currently see:

I think we should see something like this:

with a mention of the filtering if a threshold is activated. For example, just add with ${impactLayerName} below ${belowThreshold} // above ${aboveThreshold} // between ${aboveThreshold} and ${belowThreshold}

[Ry-DS]

@ericboucher

Tried to make it match closely with the pre-existing impact layer legends. Let me know if there are any more changes needed

[bencpeters]

This is getting close! I had a few clean-up suggestions, but I think the important ones are:

• Refactor the runAnalysis function a bit to move as much of the logic into Redux as possible - the async thunk should handle clearing out existing data. For input validation, we should either enforce them with a combination of Typescript & the UI (e.g. use Typescript and buttons being disabled to make sure that values exist before runAnalysis is ever called), or have the validation live inside the async thunk itself to ensure that invalid values generate UI errors using our redux error handling infrastructure.

• Move LayerDropdown into the src/components tree.

[bencpeters]

Do these errors display UI errors when they get thrown? This function is happening outside of a Redux thunk, so it seems like our error handling that we have set up there wouldn't apply here?

I think we basically want to move all the logic in this function into the redux thunk itself so that it can properly take advantage of Redux and the error handling we've set up there.

[Ry-DS]

these errors are more so to make TS happy, as TS already ensures non-null values are passed into the thunk. If these are thrown, then it highlights a clear developer-side issue as the fields weren't validated properly before runAnalysis was called. (The button is disabled if the fields are invalid/empty).

Do you think it should be done a different way? What we could do instead is dispatch a notification to redux directly and print the error instead of throwing an error only a dev would notice.

[bencpeters]

Gotcha, I see the issue. I think in a perfect world we'd handle these with form validations, and make sure that TS knows that this function will never be called without the form validations being run. But as long as we have form validations/user-friendly warnings there, it's probably fine for these to live behind the scenes. If we're relying on these to actually enforce the conditions (rather than just to make TS happy), we should make the errors visible though...

[bencpeters]

Any reason not to just use the builtin map here?

[Ry-DS]

are you referring to the Array#map here? layersList is an object, hence the use of map as a shorthand to Object.entries().map() which is the only vanilla methodology I can think of for this case. If you know a better vanilla approach let me know :)

[bencpeters]

Ya, it'd require Object.entries (or Object.keys/Object.values) and then map. But generally I don't think that's a big deal to add that one call, and the (theoretical at this point) idea is to only use Lodash where it's strictly necessary/a big savings over what is provided in the standard library. In theory this means that we could use tree-shaking to selectively include only parts of Lodash in the build, rather than the whole library.

[Ry-DS]

@laveesh rebased your PR into this branch as they helped fix a few concerns Ben raised in our PR while also fixing a few bugs 👍
Commit 1
Commit 2

outdated

[bencpeters]

I think this should be good to merge at this point!