Fix maps test for qgis_server #235

lucernae · 2017-06-01T10:17:41Z

Refactor thumbnail url generator for QGIS Server. Now giving direct url to
QGIS Server, instead of proxying to Django.
Remove unnecessary thumbnail view in qgis_server, because geonode already
have internal cache for thumbnail. Thumbnail should just be accessed
directly to QGIS Server without proxy.
Fix map creation test.
Refactor wrong/inconsistent bbox convention for maps.

codecov · 2017-06-01T12:37:37Z

Codecov Report

Merging #235 into master-qgis_server will increase coverage by 0.53%.
The diff coverage is 74.43%.

@@                  Coverage Diff                   @@
##           master-qgis_server     #235      +/-   ##
======================================================
+ Coverage               44.28%   44.81%   +0.53%     
======================================================
  Files                     254      254              
  Lines                   18292    18318      +26     
  Branches                 2409     2410       +1     
======================================================
+ Hits                     8100     8209     +109     
+ Misses                   9719     9633      -86     
- Partials                  473      476       +3

Impacted Files	Coverage Δ
geonode/layers/models.py	`77.95% <ø> (ø)`	⬆️
geonode/qgis_server/models.py	`80.55% <ø> (ø)`	⬆️
geonode/qgis_server/urls.py	`100% <ø> (ø)`	⬆️
geonode/qgis_server/admin.py	`100% <ø> (ø)`	⬆️
geonode/base/models.py	`76.94% <100%> (ø)`	⬆️
geonode/qgis_server/views.py	`16.25% <100%> (+3.47%)`	⬆️
geonode/qgis_server/tests/test_qgis_settings.py	`100% <100%> (ø)`	⬆️
geonode/qgis_server/tests/test_views.py	`100% <100%> (ø)`	⬆️
geonode/maps/models.py	`67.08% <25%> (-0.15%)`	⬇️
geonode/geoserver/helpers.py	`20.95% <50%> (+0.08%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2c8171...a802f13. Read the comment docs.

lucernae · 2017-06-02T06:28:41Z

geonode/qgis_server/helpers.py

@@ -82,3 +89,138 @@ def validate_django_settings():
            "{package}.".format(package=qgis_server.BACKEND_PACKAGE))

    return True
+
+
+def map_thumbnail_url(instance):


Check this out @Gustry , what I did is to put this as a helper function and not a django view. Then I deleted map_thumbnail django view. Is it ok to do this?

Ok, I guess it's much better as it will be integrated with Geonode resources (set thumbnail manually etc, list of resources ...)

Gustry

Nice !

Gustry · 2017-06-02T06:30:59Z

geonode/base/models.py

-        self.bbox_x1 = box[1]
-        self.bbox_y0 = box[2]
+        self.bbox_y0 = box[1]
+        self.bbox_x1 = box[2]


Was-it a bug in geonode upstream ? This makes me very worried !

This is like a very consistent inconsistency 😟 and it's been around since 2012.
The problem is as follow:

If you see bbox property from above (in the same file)
the format is: [x0,y0,x1,y1] which is probably the standard format, CMIIW.

However, set_latlon_bounds assumes the format: [x0,x1,y0,y1]

Some bbox calculation in maps creation seems wrong because it takes layer.bbox (which inherits from base), but comparing it as if [x0,x1,y0,y1] to get the largest extent of multiple layers. Then it uses this method to set the bbox. Which is swapped.

I believe this has to be double/triple checked everywhere they use this property. Because this is hectic !

I even tried to screen it again the third time for separate PR in upstream. For us, we only use leaflet map view, so we know where to look.

Gustry · 2017-06-02T06:37:47Z

geonode/qgis_server/helpers.py

@@ -82,3 +89,138 @@ def validate_django_settings():
            "{package}.".format(package=qgis_server.BACKEND_PACKAGE))

    return True
+
+
+def map_thumbnail_url(instance):


Ok, I guess it's much better as it will be integrated with Geonode resources (set thumbnail manually etc, list of resources ...)

lucernae · 2017-06-02T06:42:27Z

geonode/base/models.py

@@ -559,6 +559,7 @@ def __unicode__(self):

    @property
    def bbox(self):
+        """BBOX is in the format: [x0,y0,x1,y1]."""


add this docstring to avoid people having the same mistake.

lucernae · 2017-06-02T06:56:22Z

geonode/maps/views.py

-                    bbox[1] = max(bbox[1], layer_bbox[1])
-                    bbox[2] = min(bbox[2], layer_bbox[2])
+                    bbox[1] = min(bbox[1], layer_bbox[1])
+                    bbox[2] = max(bbox[2], layer_bbox[2])


CC @Gustry
This is the part which gets funny because it compares layer.bbox which is in [x0,y0,x1,y1] format for the first layer bbox, but comparing it with the others as if the format is [x0,x1,y0,y1]. Then when the user saves the map, it will be saved and swapped.

- Refactor thumbnail url generator for QGIS Server. Now giving direct url to QGIS Server, instead of proxying to Django. - Remove unnecessary thumbnail view in qgis_server, because geonode already have internal cache for thumbnail. Thumbnail should just be accessed directly to QGIS Server without proxy. - Fix map creation test. - Refactor wrong/inconsistent bbox convention for maps.

lucernae · 2017-06-02T11:54:58Z

Merge this before anymore changes :))

lucernae added the in progress label Jun 1, 2017

lucernae force-pushed the fix_map_test branch from f638298 to ab90563 Compare June 1, 2017 12:37

lucernae force-pushed the fix_map_test branch 11 times, most recently from 9ce2647 to e42d3ad Compare June 2, 2017 05:04

lucernae mentioned this pull request Jun 2, 2017

refactor qgis views #236

Merged

lucernae requested review from ismailsunni and Gustry June 2, 2017 06:26

lucernae commented Jun 2, 2017

View reviewed changes

Gustry approved these changes Jun 2, 2017

View reviewed changes

lucernae commented Jun 2, 2017

View reviewed changes

lucernae force-pushed the fix_map_test branch 6 times, most recently from 6cc324c to 52dacb1 Compare June 2, 2017 10:53

lucernae force-pushed the fix_map_test branch from 52dacb1 to a802f13 Compare June 2, 2017 11:03

lucernae merged commit 76a6407 into kartoza:master-qgis_server Jun 2, 2017

lucernae removed the in progress label Jun 2, 2017

lucernae deleted the fix_map_test branch June 2, 2017 11:55

This was referenced Jun 4, 2017

Image thumbnails are not generated when uploading a layer with management commands #195

Closed

List of current failed Geonode's Integration tests #172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix maps test for qgis_server #235

Fix maps test for qgis_server #235

lucernae commented Jun 1, 2017 •

edited

Loading

codecov bot commented Jun 1, 2017 •

edited

Loading

lucernae Jun 2, 2017

Gustry Jun 2, 2017

Gustry left a comment

Gustry Jun 2, 2017 •

edited

Loading

lucernae Jun 2, 2017

Gustry Jun 2, 2017

lucernae Jun 6, 2017

Gustry Jun 2, 2017

lucernae Jun 2, 2017

lucernae Jun 2, 2017

lucernae commented Jun 2, 2017

Fix maps test for qgis_server #235

Fix maps test for qgis_server #235

Conversation

lucernae commented Jun 1, 2017 • edited Loading

codecov bot commented Jun 1, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gustry left a comment

Choose a reason for hiding this comment

Gustry Jun 2, 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

Choose a reason for hiding this comment

lucernae commented Jun 2, 2017

lucernae commented Jun 1, 2017 •

edited

Loading

codecov bot commented Jun 1, 2017 •

edited

Loading

Gustry Jun 2, 2017 •

edited

Loading