archesproject / arches-her

6 stars 12 forks source link

Replace geojson app_area map_source with mvt #1318

Closed chiatt closed 2 months ago

chiatt commented 3 months ago

Adds an endpoint to serve app-areas as vector tiles for use in related resources map card.

aj-he commented 3 months ago

@chiatt - out of interest why were you not able to reuse the resource layer MVT /mvt/1909956f-3a3b-11eb-ae99-f875a44e0e11/{z}/{x}/{y}.pbf. Other code shows that you just need to use the nodeid as the source-layer??

Also, Application Area and Consultations are protected resources in our implementation so we need to make sure a new MVT endpoint using these respects those permissions. Perhaps use the MVT view as a base class

chiatt commented 3 months ago

@chiatt - out of interest why were you not able to reuse the resource layer MVT /mvt/1909956f-3a3b-11eb-ae99-f875a44e0e11/{z}/{x}/{y}.pbf. Other code shows that you just need to use the nodeid as the source-layer??

Also, Application Area and Consultations are protected resources in our implementation so we need to make sure a new MVT endpoint using these respects those permissions. Perhaps use the MVT view as a base class

Unfortunately the related resources map card builds the style off the nodeid pulled out of the data property of the source here: https://github.com/archesproject/arches/blob/d76c0576f78d3bd822737538328a35fa79e4ae79/arches/app/media/js/views/components/cards/related-resources-map.js#L64 That's why I had to add this in the source ("data": "/geojson?nodeid=1909956f-3a3b-11eb-ae99-f875a44e0e11"})).

That would be missing from the mvt. Things would still function, but you wouldn't see the application area layer. That's a good suggestion about filtering out restricted resources, but another concern about the mvt endpoint is that it can return clusters which you would not want in this card because the style isn't supported. It would be safer just to filter out restricted instances in the custom view.

aj-he commented 3 months ago

Unfortunately the related resources map card builds the style off the nodeid pulled out of the data property of the source here: https://github.com/archesproject/arches/blob/d76c0576f78d3bd822737538328a35fa79e4ae79/arches/app/media/js/views/components/cards/related-resources-map.js#L64 That's why I had to add this in the source ("data": "/geojson?nodeid=1909956f-3a3b-11eb-ae99-f875a44e0e11"})).

That would be missing from the mvt. Things would still function, but you wouldn't see the application area layer. That's a good suggestion about filtering out restricted resources, but another concern about the mvt endpoint is that it can return clusters which you would not want in this card because the style isn't supported. It would be safer just to filter out restricted instances in the custom view.

@chiatt - that's useful info. One thing I have also realised is that the migration makes changes to DB rows that aren't present until added in the prelim sql part of the package. My understanding is that manage.py migrate is run before the package is loaded.

You could probably just change that SQL script instead? https://github.com/archesproject/arches-her/blob/dev/1.0.x/arches_her/pkg/preliminary_sql/select_sources.sql

chiatt commented 2 months ago

Unfortunately the related resources map card builds the style off the nodeid pulled out of the data property of the source here: https://github.com/archesproject/arches/blob/d76c0576f78d3bd822737538328a35fa79e4ae79/arches/app/media/js/views/components/cards/related-resources-map.js#L64 That's why I had to add this in the source ("data": "/geojson?nodeid=1909956f-3a3b-11eb-ae99-f875a44e0e11"})). That would be missing from the mvt. Things would still function, but you wouldn't see the application area layer. That's a good suggestion about filtering out restricted resources, but another concern about the mvt endpoint is that it can return clusters which you would not want in this card because the style isn't supported. It would be safer just to filter out restricted instances in the custom view.

@chiatt - that's useful info. One thing I have also realised is that the migration makes changes to DB rows that aren't present until added in the prelim sql part of the package. My understanding is that manage.py migrate is run before the package is loaded.

You could probably just change that SQL script instead? https://github.com/archesproject/arches-her/blob/dev/1.0.x/arches_her/pkg/preliminary_sql/select_sources.sql

Ah - thanks @aj-he, I actually meant to remove those insert statements. We can't replace the migration with the sql script because we need Python to set the domain based on the project settings (settings.PUBLIC_SERVER_ADDRESS). As we move toward Arches Applications, we are moving away from running raw sql in a packages, so the migration is a step in that direction.

chiatt commented 2 months ago

I didn't realize that @csmith-he was working on the same solution, but I've updated this PR filtering out instance permissions.

csmith-he commented 2 months ago

I didn't realize that @csmith-he was working on the same solution, but I've updated this PR filtering out instance permissions.

@chiatt It was more a case of needing to put in the permissions filtering and to make sure it works with @SDScandrettKint's instance. I didn't want to mess up anything that would be applicable to the Arches Application method going forward. Obviously this has just caused an over complication of things.

Should we go with this pull request (as you're the one who has done most of the work) and @SDScandrettKint can manually register the application area source in the database using the SQL in the other PR?

chiatt commented 2 months ago

@csmith-he I don't mind closing this PR. You've got other changes in yours that look good, so let's just go with that one.