[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

davidblasby · 2024-05-21T18:30:36Z

This PR removes the TestWfsPost servlet and replaces it with 100% javascript functionality (running on the browser).

I've included some MIT-licensed JS code.
SEE: demo-requests.js Line 211 to 277
See screenshots, below
Trying to incorporate andrea's and peter's feedback.
- button order change on WPS/WCS page to be consistent with Demo Request Page
- changed button text to be more consistent " in Request Builder" " in new page" (verb=Execute, Get Coverage, or Show Result)
I will add documentation (new screen shots) once everyone agrees on UX (see screenshots)

Demo Page:

WPS:

WCS:

Demo Requests:

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
[after feedback] Documentation has been updated (if change is visible to end users).
[n/a] The REST API docs have been updated (when changing configuration objects or the REST controllers).
There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

jodygarnett · 2024-05-21T19:34:18Z

@davidblasby I really like the consistent action button names + workflow.

jodygarnett

Minor feedback (missing header). When folks are happy with button names we can update the docs and get this moving.

I would appreciate squash and merge (or interactive rebase) to collapse this to a single commit for backport.

Excellent work David.

src/web/demo/src/main/resources/org/geoserver/web/demo/demo-requests.js

petersmythe

I am happy with the consistent button naming, thank you.

Additional non blocking feedback from me:

Clicking Show Result, the Response Body spills over the right edge of the bolded Response Body panel, instead of wrapping.

The (red) Busy spinner icon is not the expected green/black busy standard icon of other GeoServer pages, nor is it in the top right corner, where I expected it to be.
DemoRequests > Show Result in New Page does not open in a new tab, as expected, but rather in the current tab. Control/Shift clicking does not change this.

petersmythe · 2024-05-22T13:54:51Z

src/web/demo/src/main/resources/org/geoserver/web/demo/demo-requests.js

+// ======================================================
+// https://www.npmjs.com/package/prettify-xml
+//
+// The MIT License (MIT)


There is an MIT License right in the middle of the javascript, is that intended?

Yes - there's MIT code in the JS. Thats the "xml pretty print" code.

I can move it to its own file??

You could move it to the top of the file, we have an example for GeoTools here:

https://github.com/geotools/geotools/blob/main/modules/library/main/src/main/java/org/geotools/feature/type/DateUtil.java

aaime

Strong recommendation to remove the black boxes around the headers and responses, they are alien to the GeoServer UI, can make the output harder to read.

Instead, use horizontal line separators like in the rest of the GUI (same appearance),
they would fit better and not interfere with the output.

In addition, just thinking out loud, is the XML pretty printer able to wrap text at a certain amount of columns? Geometries may otherwise be represented as a very very very long line.

The two other builders look good, but one button label is misleading in both the WCS and WPS pages:

There is no such thing as a "Demo Request Builder", had it been a "builder" we would have had no need for the WCS/WPS one. Just say "Get Coverage in Demo requests" and "Execute in Demo requests" instead.

Finally, as @jodygarnett always reminds me, documentation should be updated any time we touch the GUI.

These pages have been affected by the change, screenshots should be re-made, and descriptive text updated accordingly:

aaime · 2024-05-27T09:59:58Z

src/web/demo/src/main/java/org/geoserver/web/demo/DemoRequestsPage.html

-          </div>
+            <div style="display:flex; flex-direction: row">
+                <div class="button-group selfclear">
+                    <a  onclick="document.getElementById('openNewWindow').checked = false; submitRequest()"  ><wicket:message key="submit">Submit</wicket:message></a>


I believe @sikeoka won't be happy about more inline javascript, he's been doing a number of PRs to avoid having it, so that CSP headers can be enabled (which will in turn lead to better protection against XSS attacks).
Just one example for reference:
#7560

See open and closed PRs in this search for more context:
https://github.com/geoserver/geoserver/pulls?q=is%3Apr+author%3Asikeoka+is%3Aclosed+inline+javascript

aaime · 2024-05-27T10:01:44Z

src/web/wcs/src/main/java/org/geoserver/wcs/web/demo/WCSRequestBuilder.html

      <div wicket:id="requestBuilder"></div>
      <ul>
        <li class="button-group selfclear">
-          <button wicket:id="execute" type="submit"><wicket:message key="execute">execute</wicket:message></button>
-          <button wicket:id="executeXML" type="submit"><wicket:message key="showXML">show xml</wicket:message></button>
+<!--            <button  onclick="getCoverage()"  type="submit"><wicket:message key="execute">execute</wicket:message></button>-->


Please don't leave commented out sections.

aaime · 2024-05-27T10:02:29Z

src/web/wcs/src/main/java/org/geoserver/wcs/web/demo/WCSRequestBuilder.java

+        String js = "";
+        js += "function getCoverage() {\n";
+        js += "    var url ='" + url + "';\n";
+        js += "    var xml = document.getElementById(\"xml\").value;\n";
+        js += "    var user ='';\n";
+        js += "    var pass= '';\n";
+        js += "    openInNewWindow(url,xml,user,pass);\n";
+        js += "}\n";


This is probably going to be a problem, CSP wise, too.

jodygarnett · 2024-05-27T17:08:49Z

Sorry I accidentally closed, was trying to take it out of draft and add a checklist

workflow feedback addressed, but docs remain pending. I will review when docs are ready

davidblasby · 2024-05-27T17:48:11Z

@aaime

Added "standard" GS Spinner
DemoRequests > Show Result in New Page does not open in a new tab, as expected, but rather in the current tab. Control/Shift clicking does not change this. - didn't do anything on this. Its doing a POST, so control/shift clicking will not work (its not a link). In the future, we can probably set the form to target="_blank". However, I like the fact it opens the requests in its own page.
I've moved the "main" javascript into two files - demo-requests.js and xml-pretty-print.js
However, there's a few lines left over;
a) on the elements (click). This is probably removable by adding the click handler with something in the demo-requests.js, but I'm not sure with the wicket page-lifestyle if it will be correctly handled.
b) in the Wicket AJAX callback.

I'm not sure how to address this - I'm not sure if its possible. Wicket puts JS in the page - and definitely in the ajax callbacks (search the GS codebase for appendJavaScript). The Wicket page-lifestyle model makes this difficult as well.

I've done what I can - not sure how to move forward on this. Like I said, I'm not sure if its possible (look at the source for a wicket page and you will see a bunch of inline script tags) - but perhaps someone with a lot more wicket knowledge than I can move this forward.
I removed the headers/response border and used the standard to do a horizontal line
I changed the output so it does a word-wrap. Not 100% sure if I like this (i.e. a big geometry now takes up a lot of vertical space) or not, but its in and I think useful.
changed to "Demo Requests" (from Demo Requests Builder).

I actually went back to the old Demo Requests - and the new one is very much an improvement.

jodygarnett · 2024-05-28T00:19:03Z

doc/en/user/source/services/wps/requestbuilder.rst

@@ -1 +1,68 @@
-.. _wps_request_builder:WPS Request Builder===================The GeoServer WPS extension includes a request builder for testing out WPS processes through the :ref:`web_admin`. This tool can also be used to demonstrate processes, and construct your own examples.Accessing the request builder---------------------------------To access the WPS Request Builder:#. Navigate to the main :ref:`web_admin`.#. Click on the :guilabel:`Demos` link on the left side.#. Select :guilabel:`WPS Request Builder` from the list of demos... figure:: images/demospage.png   :align: center   *WPS request builder in the list of demos*Using the request builder-------------------------The WPS Request Builder primarily consists of a selection box listing all of the available processes, and two buttons, one to submit the WPS request, and another to display what the POST request looks like... figure:: images/requestbuilderblank.png   :align: center   *Blank WPS request builder form*The display changes depending on the process and input selected.  JTS processes have available as inputs any of a GML/WKT-based feature collection, URL reference, or subprocess.  GeoServer-specific processes have all these as options and also includes the ability to choose a GeoServer layer as input.For each process, a form will display based on the required and optional parameters associated with that process, if any... figure:: images/requestbuildertoppstates.png   :align: center   *WPS request builder form to determine the bounds of topp:states*To see the process as a POST request, click the :guilabel:`Generate XML from process inputs/outputs` button... figure:: images/requestbuilderrequest.png   :align: center   *Raw WPS POST request for the above process*To execute the process, click the :guilabel:`Execute Process` button.  The response will be displayed in a window or.. figure:: images/requestbuilderresponse.png   :align: center   *WPS server response* 


Did this really have windows line formatting before? Wow ...

jodygarnett

Thanks for adding documentation and addressing feedback.

jodygarnett · 2024-05-28T15:36:53Z

With documentation included I have marked this as "ready for review".

I believe Andrea's changes have been addressed and this can now go ahead.

jodygarnett · 2024-05-29T17:36:27Z

@aaime if I understand correctly David is at the limits of what he can do with respect to inline javascript and forthcoming CSP restrictions in Wicket 9. Reading wicket 9 guidance the result is to restrict, but a setting can be defined per page to allow pages like this to function.

Reference:

https://stackoverflow.com/questions/73783715/using-csp-rules-per-page

aaime · 2024-05-29T17:43:32Z

@jodygarnett I was just pointing out the conflict with @sikeoka work, if this gets merged as is, I guess Steve is going (unhappily maybe) to pick it up and move the JS out of the HTML pages.

I know @aaime I was just hunting down the link to #7154 to share with David who is not aware of that work. I was looking in that PR for examples and I mostly see him picking up inline styles.

Since David is at the limits of his wicket ability we need an example.

Update:

David you asked to search for script=".." and yes there are lots of examples
Searching Steve's wicket update work I see style="..." being replaced, but not script="..." being replace
Andrea do you know if only style is being a problem? Or is script being a problem also? As David points out it is very common in our codebase and a supported interaction in the wicket design.

sikeoka · 2024-05-29T18:21:54Z

I actually don't care about inline JavaScript in Wicket pages. Wicket 9 has its own complex and strict way of handling Content-Security-Policy and my work just assumes that it will cover everything else in GeoServer while letting Wicket do its own thing.

jodygarnett · 2024-05-29T18:45:22Z

I see @sikeoka has now commented.

Here was my research:

I do not know how to search just on the https://github.com/bradh/geoserver/tree/wicket8 branch.

jodygarnett · 2024-05-29T18:47:19Z

@sikeoka perhaps you could look at this PR; it would make us feel a lot better if someone with your wicket experience is comfortable with the change.

jodygarnett · 2024-05-30T17:58:32Z

@aaime I think with @sikeoka 's comment all your feedback has been addressed.

aaime · 2024-05-30T17:59:30Z

Yes yes, my comment was meant to catch Steve's attention, not to raise a blocker.

jodygarnett · 2024-05-31T17:18:59Z

I will manually backport given the squash and merge approach for clean history.

…#7672) * Remove Demo Request proxy server - TestWFSPost * update WCS request page to piggy back on top of DemoRequests page * handle WPS request builder * Remove demo request, fix test cases, add new test cases * Feeback from peter and andrea * fix QA warning * Add header * feedback from andrea * update documentation with new screenshots and functionality --------- Authored-by: david.blasby <david.blasby@geocat.net> # Conflicts: # src/wfs/src/main/java/org/vfny/geoserver/wfs/servlets/TestWfsPost.java # src/wfs/src/test/java/org/vfny/geoserver/wfs/servlets/TestWfsPostTest.java

* Remove Demo Request proxy server - TestWFSPost * update WCS request page to piggy back on top of DemoRequests page * handle WPS request builder * Remove demo request, fix test cases, add new test cases * Feeback from peter and andrea * fix QA warning * Add header * feedback from andrea * update documentation with new screenshots and functionality --------- Authored-by: david.blasby <david.blasby@geocat.net> # Conflicts: # src/wfs/src/main/java/org/vfny/geoserver/wfs/servlets/TestWfsPost.java # src/wfs/src/test/java/org/vfny/geoserver/wfs/servlets/TestWfsPostTest.java

jodygarnett · 2024-06-14T22:22:12Z

src/web/demo/src/main/java/org/geoserver/web/demo/DemoRequestsPage.java

+    }
+
+    @Override
+    public void renderHead(IHeaderResponse response) {


Re-opening https://osgeo-org.atlassian.net/browse/GEOS-11390 due to missing call to super.render(response) here, required to ensure header contributions are included.

david.blasby added 9 commits May 15, 2024 12:17

WIP: remove Demo Request proxy server - TestWFSPost

4a4aa7d

wip: wicket side stuff

9bafb2e

wip: more wicket side stuff

b5c1018

wip

d17606d

update WCS request page to piggy back on top of DemoRequests page

ad0eee1

handle WPS request builder

cad932a

wip: remove demo request, fix test cases, add new test cases

4a31b7f

feeback from peter and andrea

7c86a4c

merge from origin/main

2df22c7

davidblasby marked this pull request as draft May 21, 2024 18:36

davidblasby requested review from aaime, prushforth and petersmythe and removed request for prushforth May 21, 2024 18:37

fix QA warning

5019d4e

jodygarnett changed the title ~~[GEOS-11390] Remove test wfs post servlet~~ [GEOS-11390] Replace TestWfsPost with Javascript Demo Page May 21, 2024

jodygarnett requested changes May 21, 2024

View reviewed changes

src/web/demo/src/main/resources/org/geoserver/web/demo/demo-requests.js Show resolved Hide resolved

Add header

c54952e

petersmythe approved these changes May 22, 2024

View reviewed changes

aaime requested changes May 27, 2024

View reviewed changes

jodygarnett closed this May 27, 2024

jodygarnett reopened this May 27, 2024

jodygarnett previously approved these changes May 27, 2024

View reviewed changes

david.blasby added 2 commits May 27, 2024 10:50

feedback from andrea

861fecc

update documentation with new screenshots and functionality

6228298

jodygarnett reviewed May 28, 2024

View reviewed changes

jodygarnett approved these changes May 28, 2024

View reviewed changes

jodygarnett marked this pull request as ready for review May 28, 2024 15:34

jodygarnett added backport 2.24.x Instructs the bot to create a 2.24.x backport PR on merge backport 2.25.x labels May 31, 2024

jodygarnett merged commit eb13121 into geoserver:main May 31, 2024
13 checks passed

aaime added the failed backport label May 31, 2024

This was referenced May 31, 2024

[Backport 2.24.x] [GEOS-11390] Replace TestWfsPost with Javascript Demo Page (#7672) #7701

Merged

[2.24.x][GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7702

Closed

jodygarnett mentioned this pull request May 31, 2024

[Backport 2.25.x] [GEOS-11390] Replace TestWfsPost with Javascript Demo Page (#7672) #7703

Merged

geoserver deleted a comment from aaime May 31, 2024

jodygarnett reviewed Jun 14, 2024

View reviewed changes

davidblasby mentioned this pull request Jun 14, 2024

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7721

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

davidblasby commented May 21, 2024 •

edited by pull-request-badge bot

jodygarnett commented May 21, 2024

jodygarnett left a comment

petersmythe left a comment •

edited

petersmythe May 22, 2024

davidblasby May 22, 2024

jodygarnett May 27, 2024

aaime left a comment

aaime May 27, 2024

aaime May 27, 2024

aaime May 27, 2024

jodygarnett commented May 27, 2024

davidblasby commented May 27, 2024

jodygarnett May 28, 2024

jodygarnett left a comment

jodygarnett commented May 28, 2024

jodygarnett commented May 29, 2024 •

edited

aaime commented May 29, 2024 •

edited by jodygarnett

sikeoka commented May 29, 2024

jodygarnett commented May 29, 2024

jodygarnett commented May 29, 2024

jodygarnett commented May 30, 2024

aaime commented May 30, 2024

jodygarnett commented May 31, 2024

jodygarnett Jun 14, 2024

		@@ -1 +1,68 @@
		.. _wps_request_builder:WPS Request Builder===================The GeoServer WPS extension includes a request builder for testing out WPS processes through the :ref:`web_admin`. This tool can also be used to demonstrate processes, and construct your own examples.Accessing the request builder---------------------------------To access the WPS Request Builder:#. Navigate to the main :ref:`web_admin`.#. Click on the :guilabel:`Demos` link on the left side.#. Select :guilabel:`WPS Request Builder` from the list of demos... figure:: images/demospage.png :align: center WPS request builder in the list of demosUsing the request builder-------------------------The WPS Request Builder primarily consists of a selection box listing all of the available processes, and two buttons, one to submit the WPS request, and another to display what the POST request looks like... figure:: images/requestbuilderblank.png :align: center Blank WPS request builder formThe display changes depending on the process and input selected. JTS processes have available as inputs any of a GML/WKT-based feature collection, URL reference, or subprocess. GeoServer-specific processes have all these as options and also includes the ability to choose a GeoServer layer as input.For each process, a form will display based on the required and optional parameters associated with that process, if any... figure:: images/requestbuildertoppstates.png :align: center WPS request builder form to determine the bounds of topp:statesTo see the process as a POST request, click the :guilabel:`Generate XML from process inputs/outputs` button... figure:: images/requestbuilderrequest.png :align: center Raw WPS POST request for the above processTo execute the process, click the :guilabel:`Execute Process` button. The response will be displayed in a window or.. figure:: images/requestbuilderresponse.png :align: center WPS server response

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

Conversation

davidblasby commented May 21, 2024 • edited by pull-request-badge bot

Checklist

jodygarnett commented May 21, 2024

jodygarnett left a comment

Choose a reason for hiding this comment

petersmythe left a comment • edited

Choose a reason for hiding this comment

Additional non blocking feedback from me:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodygarnett commented May 27, 2024

davidblasby commented May 27, 2024

Choose a reason for hiding this comment

jodygarnett left a comment

Choose a reason for hiding this comment

jodygarnett commented May 28, 2024

jodygarnett commented May 29, 2024 • edited

aaime commented May 29, 2024 • edited by jodygarnett

sikeoka commented May 29, 2024

jodygarnett commented May 29, 2024

jodygarnett commented May 29, 2024

jodygarnett commented May 30, 2024

aaime commented May 30, 2024

jodygarnett commented May 31, 2024

Choose a reason for hiding this comment

davidblasby commented May 21, 2024 •

edited by pull-request-badge bot

petersmythe left a comment •

edited

jodygarnett commented May 29, 2024 •

edited

aaime commented May 29, 2024 •

edited by jodygarnett