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

feat: allow admins to select earth engine layers to show in the app (DHIS2-16098) [ee-dev] #3198

Merged
merged 23 commits into from
Jul 11, 2024

Conversation

turban
Copy link
Contributor

@turban turban commented May 8, 2024

Implements: https://dhis2.atlassian.net/browse/DHIS2-16098

As we plan add support for additional layers from Google Earth Engine, we should allow system administrators to select which layers that should be visible on their DHIS2 instance. Instead of showing all EE layers in the Add layer dialog, system administrators can select the layers that should be visible from a predefined collection.

TODO:

  • Get the right authority ID from @janhenrikoverland (should have been included in DHIS2 core 2.41)
  • Write some cypress tests
  • Gather info about data store and make any relevant changes to this implementation
  • Determine whether feature toggling is needed (if 2.40 is supported, then app needs to work with 2.40.0)

With this PR we will show no EE layers in the Add layer dialog by default. Admins having the right authority will see a "Manage available layers"-button:

Screenshot 2024-05-08 at 17 03 47

When the user clicks this dialog, the supported EE layers will be shown in a modal:

Screenshot 2024-05-08 at 16 50 48

The visibility of the individual layers can be toggled on and off. The checked layers will now show in the Add layer dialog for all users of the app:

Screenshot 2024-05-08 at 16 52 09

@dhis2-bot
Copy link
Contributor

dhis2-bot commented May 8, 2024

🚀 Deployed on https://pr-3198--dhis2-maps.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify May 8, 2024 15:01 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify May 15, 2024 11:05 Inactive
import { Checkbox } from '../core/index.js'
import styles from './styles/EarthEngineLayer.module.css'

const EarthEngineLayer = ({ layer, isAdded, onShow, onHide }) => {
Copy link
Collaborator

@jenniferarnesen jenniferarnesen May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a component named EarthEngineLayer (in src/components/map/layers). Is there a different name that could be used that would better represent what this component is?

Something like "AdditionalLayerOption". (this could be improved upon)

Copy link
Collaborator

@BRaimbault BRaimbault Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LayerTypeOption / LayerSourceOption ?

Tag: Renaming

.filter((l) => !l.legacy)
.sort((a, b) => a.name.localeCompare(b.name))

const EarthEngineModal = ({ onClose }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this component future proof, rename it. Perhaps "AdditionalLayersModal". Or some other name that reflects this modal is where the manager is choosing layers to make available in addition to the default layers.

Copy link
Collaborator

@BRaimbault BRaimbault Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LayerTypesModal / LayerSourcesManagementModal?

Tag: Renaming

editLayer,
onClose,
}) => {
const includeEarthEngineLayers = (layerTypes, addedLayers) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider more generic naming here as well.

Copy link
Collaborator

@BRaimbault BRaimbault Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

includeMoreLayerTypes / includeMoreLayerSources / includeManagedLayerSources ?

Tag: Renaming

getLayers()
}, [engine, getLayers])

return { addedLayers, loading, addLayer, removeLayer, error }
Copy link
Collaborator

@jenniferarnesen jenniferarnesen May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error is returned but I can't see that it is used anywhere. Maybe there should be some error handling, like showing a notice in the case that an add/remove operation failed.

This is also a good cypress test scenario. I can point you to some examples.

@dhis2-bot dhis2-bot temporarily deployed to netlify June 17, 2024 14:03 Inactive
Add data-test tags where required.
Add cypress tests for layer types toggling logic:
- The modal is behaving as expected
- Showing and hiding layer types
- Viewing selection without maps app admin authority
- Behaviour with "broken" dataStore namespace/key
Reserve DHIS_WEB_APP namespace in app config
Use redux store to keep track of layer types visibility setting
Add initial step to check data store integrity and restore if needed
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 09:11 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 09:39 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 09:47 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 12:31 Inactive
@BRaimbault BRaimbault added e2e record Apply this label to a pull request to trigger recording of E2E tests on Cypress Cloud labels Jun 18, 2024
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 14:18 Inactive
Copy link

cypress bot commented Jun 18, 2024

1 flaky test on run #3275 ↗︎

0 132 32 0 Flakiness 1

Details:

Merge 409f109 into f1c46b8...
Project: maps Commit: 318a8c65d3 ℹ️
Status: Passed Duration: 05:32 💡
Started: Jul 9, 2024 11:49 AM Ended: Jul 9, 2024 11:55 AM
Flakiness  cypress/integration/systemsettings.cy.js • 1 flaky test • e2e-chrome-parallel-2.40-5

View Output

Test Artifacts
systemSettings > does not include Weekly period type when weekly periods hidden in system settings Screenshots

Review all test suite changes for PR #3198 ↗︎

@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 16:07 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 18, 2024 16:36 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 19, 2024 10:47 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify June 19, 2024 14:39 Inactive
@jenniferarnesen jenniferarnesen changed the title feat: allow admins to select earth engine layers to show in the app (DHIS2-16098) feat: allow admins to select earth engine layers to show in the app (DHIS2-16098) [ee-dev] Jun 21, 2024
@dhis2-bot dhis2-bot temporarily deployed to netlify July 4, 2024 12:35 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 8, 2024 14:08 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 11:45 Inactive
@BRaimbault BRaimbault merged commit 52ed092 into ee-dev Jul 11, 2024
31 checks passed
@BRaimbault BRaimbault deleted the feat/earth-engine-modal branch July 11, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e record Apply this label to a pull request to trigger recording of E2E tests on Cypress Cloud
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants