Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() #65

JivanAmara · 2016-11-09T00:22:08Z

Update GeoNodePublishHandler to create/update new GeoNode Layer instances without reliance on geoserver-dependent gs_slurp().

…config.

coveralls · 2016-11-09T00:37:18Z

Coverage decreased (-1.6%) to 83.356% when pulling 374364f on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

…mpatibility. Added test for shim.

…ject.

coveralls · 2016-11-10T01:28:10Z

Coverage decreased (-0.5%) to 84.424% when pulling 579a63c on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

…dler. Added handle() test.

coveralls · 2016-11-15T20:03:46Z

Coverage increased (+0.2%) to 85.18% when pulling 18749e4 on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

…ppending to it.

coveralls · 2016-11-15T22:40:56Z

Coverage increased (+0.2%) to 85.18% when pulling 4c55185 on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

ghost · 2016-11-16T20:35:02Z

osgeo_importer/handlers/geonode/backward_compatibility.py

@@ -0,0 +1,105 @@
+'''
+Created on Nov 9, 2016


This information is in the git history and doesn't need to be in source files.

ghost · 2016-11-16T20:42:20Z

osgeo_importer/handlers/geonode/publish_handler.py

+'''
+Created on Nov 14, 2016
+
+@author: jivan


I strongly prefer that people don't add these attributions to source code...

It doesn't help anything, since that information is already available in git blame.

It adds clutter; if everyone did this for each change, then for example I would have my name in hundreds of places in the tests.

It raises issues about attribution that are better left to git blame. For example, you just wrote at the top of this file "author: jivan". But you did not actually write the original GeoNodePublishHandler. So do we separately attribute your changes vs the original class? It should not even matter anyway because anyone who cares knows what work you did anyway, so why even make the issue salient?

ghost · 2016-11-16T20:44:28Z

osgeo_importer/handlers/geonode/__init__.py

-logger = logging.getLogger(__name__)
-
-
-class GeoNodePublishHandler(ImportHandlerMixin):


For the benefit of other readers, this mostly moved to osgeo_importer/handlers/geonode/publish_handler.py where it does have a few changes.

This kind of move does make it harder to do diffs and thus to review things. It would help a little to separate these kinds of opinions about where things should be from the actual implementation. Maybe we should just have some separate PRs for this kind of reformatting

ghost · 2016-11-16T20:47:51Z

requirements.txt

@@ -2,3 +2,6 @@
 python-dateutil
 psycopg2
 numpy
+
+# Needed for testing
+mock


I'm OK with including this here (rather than e.g. in a separate dev-requirements.txt) because in principle it's nice to be able to run unit tests on deployed instances.

ghost · 2016-11-16T20:49:36Z

osgeo_importer_prj/settings.py

@@ -62,9 +62,12 @@

 # Note that Django automatically includes the "templates" dir in all the
 # INSTALLED_APPS, se there is no need to add maps/templates or admin/templates
+try: TEMPLATE_DIRS


Putting this on the same line isn't PEP 8, e.g.

Definitely not: ... try: something() finally: cleanup()

ghost · 2016-11-16T21:29:41Z

osgeo_importer/tests/handlers/geonode/test_backward_compatibility.py

+        # Disconnect the geoserver-specific post_save signal attached to Layer creation.
+        # The geoserver signal handler assumes things about the store where the Layer is placed.
+        # this is a workaround.
+        disconnected_post_save = signals.post_save.disconnect(geoserver_post_save, sender=Layer)


comment helps

ghost · 2016-11-16T21:30:27Z

osgeo_importer/tests/handlers/geonode/test_backward_compatibility.py

+        ]
+
+        # attribute_map gets modified as a side-effect of the call to set_attributes()
+        expected_results = copy.deepcopy(attribute_map)


Out of scope, but it would be nice if set_attributes made its own copy to avoid that

Trying to stage changes; first change to set_attributes was just to refactor into two pieces for use here, rather than introducing changes in functionality.

ghost · 2016-11-16T21:31:50Z

osgeo_importer/handlers/geonode/publish_handler.py

+            'workspace': self.workspace,
+            'store': store_name,
+            'storeType': store_type,
+            'typename': '{}:{}'.format(workspace_name.encode('utf-8'), layer_name.encode('utf-8')),


mildly prefer this to be done prior in its own line but ok

ghost · 2016-11-16T21:33:05Z

osgeo_importer/handlers/geonode/__init__.py

-            upload_layer.save()
-
-        return results
+from publish_handler import GeoNodePublishHandler  # NOQA - Moved this code but want it still available here.


thanks, it helps to look after these little public API things

ghost · 2016-11-16T21:35:27Z

osgeo_importer/tests/tests_original.py

@@ -27,15 +27,15 @@
 from osgeo_importer.handlers.geoserver import GeoWebCacheHandler


For reader context, there are almost no changes between tests.py and tests/tests_original.py.

This is good, because it means I don't have to go hunting for missing things, so never mind about that.

JivanAmara · 2016-11-17T02:12:39Z

@harts-boundless Thanks for your comments. Which do you consider blockers to a merge?

…ings. Set typename prior to use.

coveralls · 2016-11-18T18:19:02Z

Coverage increased (+0.2%) to 85.187% when pulling dd05400 on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

bitner · 2016-11-18T19:35:09Z

Assuming all Travis checks pass here, it looks like all stopper issues have been addressed. Thanks Sasha for taking the time for such a thorough review. From what I can see this PR looks ready to be merged.

coveralls · 2016-11-18T19:49:07Z

Coverage increased (+0.2%) to 85.187% when pulling b643b4d on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

coveralls · 2016-11-18T20:34:35Z

Coverage increased (+0.2%) to 85.187% when pulling 6433c84 on JivanAmara:nogeoserver-geonodemaster into f75b853 on GeoNode:master.

sarasafavi · 2016-11-20T03:17:12Z

Tested against Exchange, nothing appears to be broken - also tested with geoserver handlers removed from settings; confirmed that this fixes issue raised in #58

ghost · 2016-11-20T03:19:46Z

Great, thanks!

JivanAmara added 2 commits November 7, 2016 18:02

GeonodePulishHandler: Use AnonymousUser if there's no owner in layer_…

a9cad63

…config.

Save a Genode Layer in GeoNodePulishHandler w/o geoserver assistance.

374364f

JivanAmara changed the title ~~Nogeoserver geonodemaster~~ Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() Nov 9, 2016

JivanAmara added 4 commits November 9, 2016 13:29

Moved tests into 'tests' package.

a01b34f

tests_original flake8 fixes.

a7112be

Moved compatibility shim from geonode.__init__ to geonode.backward_co…

c40c970

…mpatibility. Added test for shim.

Updated tests_original._TEST_FILES_DIR to reflect new location in pro…

579a63c

…ject.

Moved GeoNodePublishHandler: handlers.__init__ -> handlers.import_han…

4d535a5

…dler. Added handle() test.

JivanAmara force-pushed the nogeoserver-geonodemaster branch from 17726d0 to 4d535a5 Compare November 15, 2016 02:56

Added test for new models.UploadLayer.status.

8c721e3

JivanAmara force-pushed the nogeoserver-geonodemaster branch 2 times, most recently from 3a960f9 to 18749e4 Compare November 15, 2016 19:45

Set settings.TEMPLATE_DIR to empty tuple if it isn't defined before a…

4c55185

…ppending to it.

JivanAmara force-pushed the nogeoserver-geonodemaster branch from 18749e4 to 4c55185 Compare November 15, 2016 22:24

ghost reviewed Nov 16, 2016

View reviewed changes

Removed module headers. Fixed formatting of try/except clause in sett…

dd05400

…ings. Set typename prior to use.

JivanAmara mentioned this pull request Nov 18, 2016

Fix some assertion mistakes & add a layer count assertion to UploaderTests.test_multi_upload(). #66

Merged

Removed module header from tests/test_models.py.

b643b4d

Added period to requirements comment to kick build.

6433c84

jj0hns0n mentioned this pull request Nov 19, 2016

Added utils.import_all_layers & endpoint for UploadedData to use it. #67

Closed

ghost merged commit 77f14f7 into GeoNode:master Nov 20, 2016

ghost mentioned this pull request Nov 28, 2016

GeoServer-independent use of Importer #58

Closed

JivanAmara deleted the nogeoserver-geonodemaster branch November 30, 2016 18:51

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() #65

Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() #65

JivanAmara commented Nov 9, 2016

coveralls commented Nov 9, 2016 •

edited

Loading

coveralls commented Nov 10, 2016 •

edited

Loading

coveralls commented Nov 15, 2016 •

edited

Loading

coveralls commented Nov 15, 2016 •

edited

Loading

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

JivanAmara Nov 17, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

ghost Nov 16, 2016

JivanAmara commented Nov 17, 2016

coveralls commented Nov 18, 2016 •

edited

Loading

bitner commented Nov 18, 2016

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

sarasafavi commented Nov 20, 2016 •

edited

Loading

ghost commented Nov 20, 2016

		logger = logging.getLogger(__name__)


		class GeoNodePublishHandler(ImportHandlerMixin):

		@@ -27,15 +27,15 @@
		from osgeo_importer.handlers.geoserver import GeoWebCacheHandler

Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() #65

Change GeoNodePublishHandler to create / update Layer instances without using geoserver-specific gs_slurp() #65

Conversation

JivanAmara commented Nov 9, 2016

coveralls commented Nov 9, 2016 • edited Loading

coveralls commented Nov 10, 2016 • edited Loading

coveralls commented Nov 15, 2016 • edited Loading

coveralls commented Nov 15, 2016 • 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

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

JivanAmara commented Nov 17, 2016

coveralls commented Nov 18, 2016 • edited Loading

bitner commented Nov 18, 2016

coveralls commented Nov 18, 2016 • edited Loading

coveralls commented Nov 18, 2016 • edited Loading

sarasafavi commented Nov 20, 2016 • edited Loading

ghost commented Nov 20, 2016

coveralls commented Nov 9, 2016 •

edited

Loading

coveralls commented Nov 10, 2016 •

edited

Loading

coveralls commented Nov 15, 2016 •

edited

Loading

coveralls commented Nov 15, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

sarasafavi commented Nov 20, 2016 •

edited

Loading