[FEATURE] QGIS - GeoNode Integration #5153

nyalldawson · 2017-09-08T02:11:47Z

This is a reworked version of #4995 - implementing integration of GeoNode connections and servers into the browser panel and unified data source manager, building off @ismailsunni's original work.

Changes from #4995 include:

refactoring GeoNode source select to fully take advantage of @elpaso's outstanding recent work on abstracting away provider source select dialogs. Now, there are no geonode related changes in the gui library and the "source select" and "new connection" dialogs have been pushed down to the app library.
GeoNode now uses the QgsDataItemProviderRegistry to integrate with browser
All GeoNode specific changes have been removed from QgsDataSourceManagerDialog
Some non-blocking methods have been added to QgsGeoNodeRequest, and used to avoid blocking the data source manager when connecting to GeoNode servers.
As discussed in previous PRs/during the hackfest - no interface has been added for handling other geoCMS servers. I've made some basic steps toward this (mostly be restructing the code), but I'm happy to leave the QgsGeoNodeRequest and QgsGeoNodeConnection classes in core for 3.0 and we can revisit when there's interest in integrating other CMS providers.
There's some GeoNode specific changes to the WFS/WMS provider data items - Ideally these should be removed, but given that they are isolated to providers and don't affect the 3.0 API I think it's acceptable to leave these for now.

Assuming Travis is happy, I consider this ready to merge, but would value other opinions (@elpaso, @wonder-sk), and obviously testing by @timlinux and @ismailsunni !

timlinux · 2017-09-08T06:25:09Z

Thanks so much for helping us with this @nyalldawson ! We will test now!

timlinux · 2017-09-08T14:06:35Z

Just come feedback that I tested under Mac OS and it is working very nicely!

elpaso

Thanks for the improvements here! I've left a couple of comments.

elpaso · 2017-09-09T15:18:13Z

src/providers/wfs/qgswfsdataitems.cpp

+
+      if ( !encodedUris.isEmpty() )
+      {
+        Q_FOREACH ( QString encodedUri, encodedUris )


@nyalldawson Do we prefer for range loops here?

Fixed (Also optimised some map/hash based loops, where the keys where being iterated and then the values retrieved immediately. In this case it's much more efficient to iterate over the original container and avoid the multiple value lookups)

elpaso · 2017-09-09T15:18:28Z

src/providers/wfs/qgswfsdataitems.cpp

@@ -192,6 +195,84 @@ void QgsWfsRootItem::newConnection()
 }
 #endif

+
+//////


Fixed - it was just used throughout this file as an indicator of "new class"

elpaso · 2017-09-09T15:27:46Z

src/app/geocms/geonode/qgsgeonodenewconnection.cpp

@@ -0,0 +1,272 @@
+/***************************************************************************


All this class looks very much like QgsNewHttpConnection can you extend that class instead of creating a new one?

Good suggestion thanks - lets take a look if we can do that...

@elpaso - done. There certainly was a lot of duplicate code there!

@timlinux I noticed when doing this that none of the WFS/WMS specific settings (all those options like 'invert axis orientation','wfs version', etc) are actually utilised when loading geonode layers. They were being stored but only used when reopening this dialog. Accordingly I've removed them from the geonode server connection settings dialog (since it made this change must easier!).

My question is - should these even be required here? In my view they are all settings designed to handle specific quirks of different WFS/WMS server implementations. Given that we know in this case the connection is to a geonode server, couldn't we avoid showing these options and instead just hardcode whatever options are required for optimal performance with geonode instances?

It's a lot less options for users to mess with, and in my opinion dropping these options actually increases the user perception that QGIS/GeoNode are tightly integrated.

@nyalldawson I've a limited knowledge of geonode, but AFAIK geonode supports different backends (GeoServer and QGIS Server, if I'm not wrong), so, if geonode just proxies the requests to WFS/WMS providers, it might makes sense to keep the options.

@elpaso - right. I've fixed this now, so the settings are exposed again and respected when adding layers via the source select dialog. I've tightened up some of the code surrounding this and added some tests too.

However... in the process I found a bigger issue! (Yay! Coding! Gotta love it!) Turns out these service specific settings are ignored for ALL providers when adding layers from the browser. (e.g. adding a WMS layer doesn't respect the axis/dpi/referer/etc settings, which are correctly respected when adding the same from the source select dialog).

That bug's WAY out of scope here though, so I've run away from it.

@nyalldawson

That bug's WAY out of scope here though, so I've run away from it.

I'd recommend that you file a ticked for that issue, maybe we can fix it in the next pre-release bugfix round.

Done- https://issues.qgis.org/issues/17123

Thanks @nyalldawson for picking up on that. 👍 to deal with it in a separate ticket.

elpaso · 2017-09-09T15:38:04Z

tests/src/core/testqgsgeonodeconnection.cpp

+  QgsSettings settings;
+
+  // Testing real server, demo.geonode.org. Need to be changed later.
+  settings.setValue( QgsGeoNodeConnectionUtils::pathGeoNodeConnection() + QStringLiteral( "/%1/url" ).arg( mDemoGeoNodeName ), mDemoGeoNodeURL, QgsSettings::Providers );


I'd prefer if we could avoid pinging the network here: I think that all tests should be able to run on disconnected environments (not only for latency but for consistancy and reliability), why don't you follow the same route that WFS tests do? I.e. store the response in the test data folder and fake the connection to load responses from disk? @nyalldawson @m-kuhn do we have any good practice here?

Thank you so much for you're review @elpaso. Yes I think having mocked responses would be nicer.

@myarjunar Can you take care of doing this second request - I will get @nyalldawson to do the first. Thanks!

elpaso · 2017-09-12T05:54:11Z

@nyalldawson I've got some (negative) feedback about the HTTP blocking calls scattered here and there in this PR: I also had concerns but I assumed that the blocking calls are only used in browser tree threads and for this reason they wouldn't block the GUI even with large datasets, is my assumption correct?

nyalldawson · 2017-09-12T06:00:09Z

@elpaso I've got some (negative) feedback about the HTTP blocking calls scattered here and there in this PR: I also had concerns but I assumed that the blocking calls are only used in browser tree threads and for this reason they wouldn't block the GUI even with large datasets, is my assumption correct?

Correct. QgsGeoNodeRequest has a number of blocking/non blocking methods. Some of these duplicate each other with a blocking and non-blocking version. I made sure everything which could block the UI (i.e. through the source select dialog) uses the non-blocking method. The others (e.g. those which are only used through the browser) I've left as the blocking methods. I also thought for Python code having these blocking methods still available (even when there's the non-blocking alternative) is probably a good move, since it makes things a bit easier for simple scripts.

elpaso · 2017-09-12T11:47:32Z

@nyalldawson @timlinux
This looks much better now, thanks for all the efforts! +1 for merge.

There is just a (hopefully) last potential issue that I want to share, I've got some more feedback from overseas, the exchange team has concerns about the assumption that the WMS/WFS layers returned by geonode are from the same domain of the geonode API server. That might not always be the case and could lead to problems with OAuth2 authentication. I didn't check the details, but maybe it's worth considering.

Quoting @amirahav :

On the authentication side, I think things are going to be a bit more complicated - on any call to geonode itself the bearer token will have to be added as a header (like you would expect), however the current implementation of oauth2 in in Geoserver only accepts the bearer token as as query param (this makes it arguable if the implementation is within the spec, but, still that's how it's working).
Thinking back to how we implemented it back for some customers, we didn't have to worry about it because we didn't connect directly to Geoserver, however currently exchange/geonode exposes Geoserver directly, and is dependent on Geoserver authenticating against geonode.

I think the geonode API needs to be extended. Currently the QGIS code assume that Geoserver is running under the same domain as geonode, and constructs the query by parsing the geonode featurtype name. However I think the geonode API should include the full path of the service it's pointing to.

Gustry · 2017-09-12T11:57:27Z

Thanks @elpaso for the comment. In this PR, we support both QGIS Server backend and Geoserver. We are using the Geonode api to retrieve URLs, so it might be fine because the geoserver URL will be given by Geonode. I can't speak about the OAuth2 authentication, I don't know.

For QGIS Server, we don't expose it to the public. All requests go into Geonode application as a proxy for QGIS Server.

timlinux · 2017-09-12T19:45:44Z

Thanks everyone for your inputs and patience! Great to see this merged!

…el (qgis#4816) * add Geonode connection menu to the toolbar * add header files for geonode-qgis client * add action to launch geonode connection dialog from menubar * Move to proper directory * Add geonodeconnection class. * Add unit test for geonode connection. * Use const static to avoid typo. * Get list layers from geonode. * Add get maps method. * Geonode connection dialog (#13) * add new geonode connection dialog * apply functionality to the geonode connection manager dialog * add save and load geonode connection functionality * edit baseKey and credentialBaseKey * remove auto-connect slots * Add unit test for geonode connection. * Add wms url getter. * Add uuid and layer name in the table. * Add handler for the list layer clicked. WIP. * Use new style connect, better hacky to get wms url. * update gitignore * Make QGIS able to add WMS layer from geonode. With hacky code. * Fix Docstring. * Show web service type (WMS/WFS) in layer table. * fix http and toolbar menu * add geonode data item to the browser panel as an extention of ows provider * [WIP] Add WFS. * Add geonode get service url. * combobox functionality and test geonode connection * Add WFS. * Disable add button if it's a map. Currently we can't do anything for map. * Add busy cursor when add layer. * get service uri capabilitites * add available layers to the geonode browser panel * remove debugging footprint and replace old style connect * add actions (new, edit, delete) to geonode browser panel * fix getLayers by WMS url * add Geonode connection menu to the toolbar * Filter out invalid layer / map. * Fix service url method. * Add service url for XYZ for GeoNode QGIS Server backend. * Add XYZ url to geonode connection dialog. * Add XYZ layer to QGIS. * fix double geonode submenu * add wfs/wms layers from browser panel using its native provider * comply with qgis3 new class naming * Handle different prefix for layer in GeoNode QGIS Server backend. * base class for cms connection * make geonode connection as a derived class from cms connection * update cmakelists * move geonode connection to geocms dir * update CMakeLists * Handle geonode 2.7 with new API. * Handle multiple geoserver url in one geonode. * Fix add xyz for qgis server. Fix add wms, wfs, xyz for geoserver in geonode 2.7 * Refactor serviceURL to return QStringList. * add 'add geonode layer' icon * add geonode to the data source manager dialog * [GeoNode-Client] Fix add WFS layer. * fix wms url parameter * add xyz dataitems * Use new style connect. * [GeoNode Client] Handle qgis server specific typename to make add WFS works. * Code improvement. * [GeoNode Client] Make geonode dialog in add universal layer can add layer. * Open universal add layer when click Add GeoNode layer. * Make sure the geonode url has protocol. * Handle geonode version in a better way. * make sure the serviceUrl method has scheme in its urls * add services option to the dialog * remove version label if not wfs * construct wms url with parameters for geonode connection * handle layer from multi service urls for every wfs, wms, & xyz services * fix new style connect using static_cast * hode close button if dialog is in embedded mode * fix xyz layer naming in the browser tree * create base class for geocms dataitems * fix compiling warning * Use struct instead QVariantMap. * tidy up code * Tidy up code, use QgsStringMap instead QVariantMap. * Add spellok for catalogue. * update sip * update sip * Use naming convention for QgsGeoCmsConnection and use QUuid. * Async by using GeoNodeRequest class. * Move geonode to src/gui. * Use stack not heap. * Remove unused includes. * Use signal to handle request. * Use QStringLiteral. * Switch to use Q_FOREACH. * Use Q_FOREACH and addressing PR's review. * Set private for data members. * update sip * Update sip. * Update sip. * Fix sip problem to make it buildable again. * Remove geocms * Tidy up code. * Use QgsSetting Scope::Provider. * Fix missing zip.h

- avoid container detachments - use QStringLiteral

Move utility functions to QgsGeoNodeConnectionUtils

QgsMessageLog should only be used for errors or warnings, not 'everything is working ok' type debug messages

…erver on Travis

Also add missing docstrings

… key parsing

can be shown simultaneously

Avoids a lot of duplicate code. Note that while this refactoring was done to allow WFS and WMS settings to be simultaneously visible, I've removed the settings from the GeoNode connection for now. Looking into this they were being stored, but not used at all when loading the layers from the GeoNode instance.

…n settings

…rom source select dialog

Instead of including these classes in app, we instead make a shell data provider which implements only the dataItemProviders() and sourceSelectProviders() methods. Helps keep GeoNode GUI related code partitioned

nyalldawson · 2017-09-12T21:42:38Z

Thanks @elpaso for the comment. In this PR, we support both QGIS Server backend and Geoserver. We are using the Geonode api to retrieve URLs, so it might be fine because the geoserver URL will be given by Geonode. I can't speak about the OAuth2 authentication, I don't know.

For QGIS Server, we don't expose it to the public. All requests go into Geonode application as a proxy for QGIS Server.

Lets keep an eye on the bug tracker, and if this proves to be a limitation "in the wild" we can address at that stage.

nyalldawson requested a review from elpaso September 8, 2017 02:11

elpaso reviewed Sep 9, 2017

View reviewed changes

nyalldawson force-pushed the geonode branch 2 times, most recently from e45b42d to 5041c00 Compare September 11, 2017 04:06

nyalldawson mentioned this pull request Sep 11, 2017

[FEATURE] QGIS - GeoNode Integration: Integrate with QGIS Browser Pan… #4995

Closed

8 tasks

nyalldawson force-pushed the geonode branch from 5041c00 to 5b87988 Compare September 12, 2017 05:33

myarjunar and others added 17 commits September 13, 2017 05:47

fix qgis crash on startup (invalid pointer)

92a1550

Fix QGIS Crash on loading WFS/WMS/XYZ layer from GeoNode.

3d3ff70

add copy and paste style actions to the layer node

bdc3d36

Simplify getLayers method for geonode client.

500c5c1

Add style retriever for a geonode layer.

3016158

Fix can not delete geonode instance from browser.

cad8fa8

Static members should use a 's' prefix, not 'm'

a46cc54

Pass strings by const ref

b1745b0

Performance optimizations

5b092ae

- avoid container detachments - use QStringLiteral

QgsGeonodeConnection is not a Q_OBJECT

b7683f2

Split QgsGeoNodeConnection into two classes

378a9e9

Move utility functions to QgsGeoNodeConnectionUtils

Use QgsDebugMsg instead of QgsMessageLog for non-error feedback

80148c9

QgsMessageLog should only be used for errors or warnings, not 'everything is working ok' type debug messages

Fix ui file build warning

92a2499

QgsGeoNodeSourceSelect is a QgsAbstractDataSourceWidget

003bf3a

Remove unused code

a5957af

Minor code cleanup

e51a0d4

nyalldawson added 23 commits September 13, 2017 05:47

Add missing sip file

e803545

Run all GeoNode tests locally, but skip ones which rely on a remote s…

e6a8980

…erver on Travis

Class shuffle

1a19283

[geonode] Don't block data source manager while connecting to a server

e1562df

Also add missing docstrings

Cleanup and add docs for QgsGeoNodeConnection

e254368

Hide non-functional style related browser actions

388b37a

Fix spelling

9a8adfc

Revert unused changes to QgsDataItem

944cb8c

Remove Q_FOREACH, fix inefficient loops

af03d2f

Clean comments

2c00199

Fix ordering of Arcgis map server item

88b8741

Use explicit flags to set behavior of QgsNewHttpConnection instead of…

6134950

… key parsing

Rework QgsNewHttpConnection layout so that both WFS and WMS settings

35eea22

can be shown simultaneously

Cleanup some strings

90237fa

Fix build

42869bb

Remove duplicate code

51922fc

Cleanup QgsOwsConnection

4c87338

Generalize some WFS/WMS connection setting handling

05e047d

Add support for updating a uri with GeoNode WFS/WMS related connectio…

ac15df9

…n settings

Make sure WFS/WMS settings are respected when adding GeoNode layers f…

687fc52

…rom source select dialog

Fix geonode connection correctly storing service specific settings

54df0a2

Move GeoNode browser/source select from app -> new 'geonode' provider

220c5e8

Instead of including these classes in app, we instead make a shell data provider which implements only the dataItemProviders() and sourceSelectProviders() methods. Helps keep GeoNode GUI related code partitioned

nyalldawson force-pushed the geonode branch from 5b0938b to 220c5e8 Compare September 12, 2017 19:48

nyalldawson merged commit 5796783 into qgis:master Sep 12, 2017

nyalldawson deleted the geonode branch September 12, 2017 21:42

myarjunar mentioned this pull request Sep 15, 2017

geonode integration - copy style action #5207

Merged

8 tasks

This was referenced May 25, 2019

Expression editor in Layer Properties does not support division of integer data #19495

Closed

Dividing integer with integer gives wrong result (if operation is supposed to return a decimal) #19757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] QGIS - GeoNode Integration #5153

[FEATURE] QGIS - GeoNode Integration #5153

nyalldawson commented Sep 8, 2017

timlinux commented Sep 8, 2017

timlinux commented Sep 8, 2017

elpaso left a comment

elpaso Sep 9, 2017

nyalldawson Sep 10, 2017

elpaso Sep 9, 2017

nyalldawson Sep 10, 2017

elpaso Sep 9, 2017

timlinux Sep 10, 2017

nyalldawson Sep 11, 2017

elpaso Sep 11, 2017

nyalldawson Sep 12, 2017

elpaso Sep 12, 2017 •

edited

Loading

nyalldawson Sep 12, 2017

timlinux Sep 12, 2017

elpaso Sep 9, 2017

timlinux Sep 10, 2017

timlinux Sep 10, 2017

elpaso commented Sep 12, 2017

nyalldawson commented Sep 12, 2017

elpaso commented Sep 12, 2017 •

edited

Loading

Gustry commented Sep 12, 2017

timlinux commented Sep 12, 2017

nyalldawson commented Sep 12, 2017

@@ @@ -192,6 +195,84 @@ void QgsWfsRootItem::newConnection() @@
               }
               #endif
+              //////

		@@ -0,0 +1,272 @@
		/***************************************************************************

[FEATURE] QGIS - GeoNode Integration #5153

[FEATURE] QGIS - GeoNode Integration #5153

Conversation

nyalldawson commented Sep 8, 2017

timlinux commented Sep 8, 2017

timlinux commented Sep 8, 2017

elpaso 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elpaso Sep 12, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elpaso commented Sep 12, 2017

nyalldawson commented Sep 12, 2017

elpaso commented Sep 12, 2017 • edited Loading

Gustry commented Sep 12, 2017

timlinux commented Sep 12, 2017

nyalldawson commented Sep 12, 2017

elpaso Sep 12, 2017 •

edited

Loading

elpaso commented Sep 12, 2017 •

edited

Loading