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: add paramter "resolvedepth" in UI when downloading data via WFS #1174

Merged
merged 1 commit into from
May 27, 2024

Conversation

emanuelaepure10
Copy link
Contributor

@emanuelaepure10 emanuelaepure10 commented May 15, 2024

Add in the dialog for the WFS GetCapability the possibility to choose if you want to add a resolveDepth for a request and how much would that be. Has been added a spinner to choose a number above 0 or the '*' for all the possible depths to be resolved.

ING-4129
Closes #1085

@emanuelaepure10
Copy link
Contributor Author

The text for the buttons will be updated in the next days.

@@ -34,4 +34,5 @@ class WFSGetFeatureConfig {
BBox bbox
String bboxCrsUri
Integer maxFeatures
String resolvedepth
Copy link
Member

Choose a reason for hiding this comment

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

Code style: this should be called resolveDepth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually created it like that and then when I tested I got a URL like:

.......SERVICE=WFS&VERSION=2.0.0&REQUEST=GetFeature&NAMESPACES=xmlns....TYPENAMES=sd%3ASpeciesDistributionDataSet&resolveDepth=2

and I thought that is not what we wanted, as I have always seen URIs with resolvedepth.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand, the name of the member variable here is not related to the name of the parameter that is added to the query, right?

In any case, WFS query parameters are not case-sensitive, resolveDepth, resolvedepth and even RESOLVEDEPTH work just the same (see sec. 6.2.5.2 of the WFS specification).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but from the wizard when creating the parameters that will be added to the query I need to create the parameter in a way.

@florianesser
Copy link
Member

The commit message and the PR should also reference the related GitHub issue #1085

@emanuelaepure10 emanuelaepure10 force-pushed the feat/ING-4129 branch 2 times, most recently from 89b51d6 to 7b4845a Compare May 23, 2024 09:24
@florianesser
Copy link
Member

The commit message and the PR should also reference the related GitHub issue #1085

The closing keyword (e.g. "Closes" or "Fixes") is still missing (see discussion here)

@emanuelaepure10 emanuelaepure10 force-pushed the feat/ING-4129 branch 3 times, most recently from 809978b to 8c9e64f Compare May 23, 2024 12:06
Copy link
Member

@stempler stempler left a comment

Choose a reason for hiding this comment

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

One minor thing related to the commit message, the message should start lower case (i.e. add instead of Add)

@emanuelaepure10 emanuelaepure10 changed the title feat: Add paramter "resolvedepth" in UI when downloading data via WFS feat: add paramter "resolvedepth" in UI when downloading data via WFS May 24, 2024
Copy link
Member

@florianesser florianesser left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, LGTM now

Add in the dialog for the WFS GetCapability the possibility to choose if you want to add a resolveDepth for a request and how much would that be. Has been added a spinner to choose a number above 0 or the '*' for all the possible depths to be resolved.

ING-4129
Closes halestudio#1085
@emanuelaepure10
Copy link
Contributor Author

@stempler I just update with rebase and suddenly the build doesn't pass anymore. Any thoughts on that could cause this? Thank you

@stempler
Copy link
Member

@emanuelaepure10
I think you remember these kind of errors from setting up the GitHub Actions workflows.

Request to http://build-artifacts.wetransform.to/p2/mirror/2020-03-releases/plugins/org.eclipse.equinox.log.stream_1.0.200.v20191017-2055.jar failed, trying cache instead...
Some attempts to read artifact osgi.bundle,org.eclipse.equinox.log.stream,1.0.200.v20191017-2055 failed:
   An error occurred while transferring artifact canonical: osgi.bundle,org.eclipse.equinox.log.stream,1.0.200.v20191017-2055 from repository http://build-artifacts.wetransform.to/p2/mirror/2020-03-releases:
      download from http://build-artifacts.wetransform.to/p2/mirror/2020-03-releases/plugins/org.eclipse.equinox.log.stream_1.0.200.v20191017-2055.jar failed

...

An error occurred while installing the items
Installation failed.
	session context was:(profile=DefaultProfile, phase=org.eclipse.equinox.internal.p2.engine.phases.Install, operand=null --> [R]org.eclipse.equinox.log.stream 1.0.200.v20191017-2055, action=org.eclipse.equinox.internal.p2.touchpoint.eclipse.actions.InstallBundleAction).
	The artifact file for osgi.bundle,org.eclipse.equinox.log.stream,1.0.200.v20191017-2055 was not found.
[a76044af-dd05-42e5-950f-2a17029a4fd0][extension>org.eclipse.tycho:tycho-maven-plugin:3.0.1] An error occurred while installing the items

The issue is not really resolved, just usually/mostly works, at least when a build cache is available. I don't know what the reason is when it fails despite having access to the cache.

But as you see this is not a required check. If you need the artifacts build, you can open the details and trigger a rerun of the failed job.

@emanuelaepure10
Copy link
Contributor Author

I think you remember these kind of errors from setting up the GitHub Actions workflows.
I do remember.
My has just cannot click the rebase and merge when above is something red :-D

@emanuelaepure10 emanuelaepure10 merged commit 693b8b8 into halestudio:master May 27, 2024
3 checks passed
@florianesser
Copy link
Member

Shouldn't PRs only be merged after the corresponding ticket is challenged? I'm wondering because the merge now closed #1085 even though the challenge on ING-4129 was not done yet.

@stempler
Copy link
Member

Shouldn't PRs only be merged after the corresponding ticket is challenged

Yes, that would be the desired approach. @emanuelaepure10 maybe you remember when we discussed the autoclosing of issues here.

@emanuelaepure10
Copy link
Contributor Author

@stempler I remember very well the discussion, but I see no conclusion.
Really sorry but for me this

Using the Closes #issue notation would be beneficial here, because (1) references to the issues could be automatically added to the changelog and (2) issues would be automatically commented in which release they were included (see related code in semantic-release/github)

didn't sound like THE way to go ahead from then on. But if that was the intention, please accept my apologies, and now I will know how to act for the future.
I would say that we should also add to the internal issue a note for the challenger to inform that the issue is ready to be challenged.

@stempler
Copy link
Member

But if that was the intention, please accept my apologies, and now I will know how to act for the future.

@emanuelaepure10 No need to apology, I just wanted reference the discussion as a reminder on what was discussed.

As you already had added the closes keyword after the feedback by @florianesser I don't see the relevance of the quote you stated, since you basically already applied that "way" by using the keyword.

Copy link

we-release bot commented Jun 19, 2024

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@we-release we-release bot added the released label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add paramter "resolvedepth" in UI when downloading data via WFS
3 participants