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

#8545 LayerDownload: service selector #8607

Merged

Conversation

belom88
Copy link
Contributor

@belom88 belom88 commented Sep 23, 2022

Description

New selector to select service.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#8545

What is the new behavior?
New selector for download dialog has been added to select service from list: "WPS", "WFS".
If "WPS" is unavailable, the selector is disabled with selected option "WFS".

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

@belom88 belom88 self-assigned this Sep 23, 2022
Copy link
Contributor

@allyoucanmap allyoucanmap left a comment

Choose a reason for hiding this comment

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

We should add a cfg option called defaultSelectedService that defines the initial selected service in the dropdown when a user opens the download dialog and they have the possibility to select the two options.
We can add a new argument to checkWPSAvailability(url, selectedService) action.
The defaultSelectedService will be wps by default as the current behaviour.
ecample of the expected configuration aviability in mapstore

{
  "name": "LayerDownload",
  "cfg": {
    "defaultSelectedService": "wfs"
  }
}

Comment on lines 59 to 66
constructor(props) {
super(props);

this.services = [
{ value: "wps", label: "WPS" },
{ value: "wfs", label: "WFS" }
];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this and move this.services as props this.props.services and in defaultProps

services: [
            { value: "wps", label: "WPS" },
            { value: "wfs", label: "WFS" }
        ]

Comment on lines 77 to 82
<label><Message msgId="layerdownload.service" /></label>
<Select
value={this.props.service}
disabled={!this.props.wpsAvailable}
onChange={(sel) => this.props.setService(sel.value)}
options={this.services} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to hide the select input when wps is not available Instead of disabled it

@@ -1732,6 +1732,7 @@
},
"layerdownload": {
"title": "Exportar los datos",
"service": "Service",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"service": "Service",
"service": "Servicio",

@belom88 belom88 marked this pull request as ready for review September 29, 2022 14:30
Copy link
Contributor

@allyoucanmap allyoucanmap left a comment

Choose a reason for hiding this comment

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

  • Please complete the PR with unit tests on actions, reducers, epics and components of the implemented parts, thanks

this.props.onCheckWPSAvailability(this.props.url || this.props.layer.url);
this.props.onCheckWPSAvailability(
this.props.url || this.props.layer.url,
this.props.defaultSelectedService || 'wps'
Copy link
Contributor

@allyoucanmap allyoucanmap Sep 29, 2022

Choose a reason for hiding this comment

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

Please add defaultSelectedService to propTypes and the default value wps in defaultProps

@@ -358,7 +358,12 @@
}
}
}, "Home", "FeatureEditor",
"LayerDownload",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore "LayerDownload" config because by default is already "wps"

@allyoucanmap allyoucanmap added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 3, 2022
@allyoucanmap
Copy link
Contributor

@ElenaGallo please test on dev and let us know when we can backport on the stable branch 2022.02.xx, thanks

@allyoucanmap allyoucanmap merged commit 6f75ca9 into geosolutions-it:master Oct 3, 2022
@ElenaGallo
Copy link
Contributor

@belom88 the English translation of the option title is incorrect. It should be Service instead of layerdownload.service.
service

@belom88
Copy link
Contributor Author

belom88 commented Oct 3, 2022

@ElenaGallo that should be browser cache issue. Please, clean the cache in your web browser.

@ElenaGallo
Copy link
Contributor

ElenaGallo commented Oct 3, 2022

@belom88 Now I see the right title.

@ElenaGallo
Copy link
Contributor

@tdipisa @belom88 I also noticed that for raster layers there is the possibility to change from WPS to WFS but then the WFS service is not enabled and the note report 'The server doesn't support WFS or WPS download for this layer!' (the message is incorrect because WPS is supported).

How to reproduce
For the test used sfdem layer form the demo map
raster

@tdipisa
Copy link
Member

tdipisa commented Oct 5, 2022

@tdipisa @belom88 I also noticed that for raster layers there is the possibility to change from WPS to WFS but then the WFS service is not enabled and the note report 'The server doesn't support WFS or WPS download for this layer!' (the message is incorrect because WPS is supported).

@belom88 (FYI @allyoucanmap) this should not be allowed. A fix is needed for this.

@belom88 belom88 mentioned this pull request Oct 6, 2022
12 tasks
belom88 added a commit to belom88/MapStore2 that referenced this pull request Oct 11, 2022
tdipisa pushed a commit that referenced this pull request Oct 11, 2022
* #8545 LayerDownload: service selector (#8607)

* #8545 Layer download: WPS only case (#8668)
@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants