PB-111: Fix print spec send to the print service#727
Conversation
Passing run #1432 ↗︎
Details:
Review all test suite changes for PR #727 ↗︎ |
|||||||||||||||
dcd4ce4 to
c76f625
Compare
39ff8cc to
234ae53
Compare
src/api/print.api.js
Outdated
| * @param {String[]} [config.excludedLayers=[]] List of layer names to exclude from the print. | ||
| * Default is `[]` |
There was a problem hiding this comment.
I would either pass AbstractLayer instances, or rename this param excludedLayerOlNames to make it clear what it contains
If I understand correctly you will filter layers based on the name we have given them in the OpenLayers context, right?
Might be better to find a way to filter things out based on the layer ID we have in the store (we give each OL layer the same ID as the layer ID in the store, most of the time)
There was a problem hiding this comment.
I change it to filter by ID, since I guess it's more common to have ID than name. So, I change the param with excludedLayerIDs to make it more clear. Is that ok?
| try { | ||
| const documentUrl = await print(printGrid.value, printLegend.value) | ||
| if (documentUrl) { | ||
| console.log('documentUrl: ', documentUrl) |
f70e028 to
06511db
Compare
|
@ismailsunni the legend seems not to be printed anymore, at least with the public transport layer and hiking closure layer. |
|
Maybe there's something wrong with how we build the legend attribute, if you have a look here https://github.com/geoadmin/service-print3/blob/develop/print-apps/mapviewer/requestData_legend.json it looks like it should be only one entry with all icons in an array. We do that instead : {
"legends": {
"classes": [
{
"icons": [
"https://sys-api3.dev.bgdi.ch/static/images/legends/ch.bav.haltestellen-oev_en.png"
],
"name": "Public transport stops"
},
{
"icons": [
"https://sys-api3.dev.bgdi.ch/static/images/legends/ch.swisstopo.swisstlm3d-wanderwege_en.png"
],
"name": "Hiking trails"
}
],
"name": "Legend"
}
}TL;DR; it works fine with only one layer with legend, but with two it breaks 🙃 |
|
@pakb that's strange. I will check it why and add a unit test for it also. But I am pretty sure it was working fine with more than one legend. |
Maybe my fault when I moved and reworked the code 🙈 |
|
@ismailsunni @pakb @hansmannj the legend issue with the print might be also due to the service-print3 deployment of this morning, @hansmannj what is the difference of you local service-print3 image and the master and develop latest state ? |
I just tried to revert on dev the deployment of service-print3 but the issue with the legend is still present. |
|
@ltshb just curious, is it possible to revert the service-print deployment to some time last week? Just to make sure. Because the payloads sent are the same as before, even after refactoring. |
As said, the issue arise when there's more than one layer with legend. If you, by any chance, didn't test that, but only added one layer extra (with legend) before printing, it might have gone under your radar (as mine 🙃 ) |
|
I'll check on the train in about half an hour. Please correct me, if I have it wrong in my mind @LukasJoss |
|
And yes, probably we never had a test case so far, with more than one layer plus legend, might well be. |
|
Probably it is in the print payload. |
|
Testing locally with the payload form the link in my last comment works and results in a PDF with two legends. So maybe there's something wrong with the payload sent by the viewer? |
|
@hansmannj @pakb @ltshb |
ltshb
left a comment
There was a problem hiding this comment.
One last issue that I've noticed, the scale and the disclaimer is always in german in the PDF, I could not find anything related to this in the print payload, @hansmannj is it something that has been hardcoded on the server side ?
@hansmannj how is it to translate the scale and disclaimer on the print ? |
Dear all Thanks for working on the fixes. Regarding the translations, I created this ticket: https://jira.swisstopo.ch/browse/PB-383. |
ltshb
left a comment
There was a problem hiding this comment.
Ok from my side this is good to go. The issue with the disclaimer and scale translation can be done later in a separate PR as it might require some backend work.
|
@ismailsunni I allow myself to rebase to develop your PR, @pakb could you eventually have a quick look and if everything is ok @ismailsunni can you merge it tomorrow ? I'd like to deploy on test.map.geo.admin.ch tomorrow and it would be great to have this PR merged before. |
2293d86 to
a59e753
Compare
pakb
left a comment
There was a problem hiding this comment.
LGTM as we will be dealing with the translation of the footer separately.
Also let's keep in mind we need to revert back to a more "stable" versioning of the geoblocks lib ASAP (as soon as it is published)
…elease a new version.
a59e753 to
c8560ea
Compare
Test link