PB-111: Fix print spec send to the print service #727

ismailsunni · 2024-03-21T08:05:11Z

Exclude print rectangle layer
Update mapfishprint from this PR Fix encodeWmsLayerState geoblocks/mapfishprint#28
- Remove npm warning
- Fix WMS encoding

cypress · 2024-03-21T08:15:01Z

Passing run #1432 ↗︎

0	170	22	0	0

Details:

PB:111: Update geoblocks/mapfishprint for a WMTS matrix size fix.
Project: web-mapviewer	Commit: `c8560ea895`
Status: Passed	Duration: 04:57 💡
Started: Apr 3, 2024 3:24 AM	Ended: Apr 3, 2024 3:29 AM

Review all test suite changes for PR #727 ↗︎

tests/cypress/tests-e2e/print.cy.js

src/api/print.api.js

pakb · 2024-03-28T06:54:48Z

src/api/print.api.js

+ * @param {String[]} [config.excludedLayers=[]] List of layer names to exclude from the print.
+ *   Default is `[]`


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)

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?

pakb · 2024-03-28T06:56:35Z

src/modules/menu/components/print/MenuPrintSection.vue

@@ -84,6 +84,7 @@ async function printMap() {
    try {
        const documentUrl = await print(printGrid.value, printLegend.value)
        if (documentUrl) {
+            console.log('documentUrl: ', documentUrl)


little console log left 😉

tests/cypress/tests-e2e/print.cy.js

ltshb · 2024-03-28T10:15:01Z

@ismailsunni the legend seems not to be printed anymore, at least with the public transport layer and hiking closure layer.

pakb · 2024-03-28T10:26:41Z

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 🙃

ismailsunni · 2024-03-28T10:31:55Z

@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.

pakb · 2024-03-28T10:36:11Z

@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 🙈

ltshb · 2024-03-28T10:37:31Z

@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 ?

ltshb · 2024-03-28T10:43:06Z

@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.

ismailsunni · 2024-03-28T10:45:48Z

@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.

pakb · 2024-03-28T11:02:24Z

@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 🙃 )

hansmannj · 2024-03-28T14:55:05Z

I'll check on the train in about half an hour.
If I remember correctly, we did not really touch any of the service configurations itself. The only difference should be, that the image is based on a different image tag of c2c.
Our new service-print3 release should be based on an c2c image, which includes some bug fixes which where discovered due to our load testing.
The service config files should not have changed.

Please correct me, if I have it wrong in my mind @LukasJoss

hansmannj · 2024-03-28T14:57:29Z

And yes, probably we never had a test case so far, with more than one layer plus legend, might well be.

hansmannj · 2024-03-28T15:02:02Z

Probably it is in the print payload.
I guess, this is how it should look like, when there's more than one legend entry: https://github.com/geoadmin/service-print3/blob/c69347869eb7f284886f5352a64aa8042e5f6979/print-apps/mapviewer/requestData_WMTS_legend.json#L226-L237

hansmannj · 2024-03-28T15:59:25Z

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?

ismailsunni · 2024-03-29T07:19:19Z

@hansmannj @pakb @ltshb
I found the issue. It's legend vs legends. The correct one should be legend. It's indeed very tricky to find, I need to compare with the one that has legend on the older version. I have added the test also for the legend spec. Below is a sample report with the test link above.
mapfish-print-report (49).pdf

ltshb

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 ?

ltshb · 2024-04-02T06:37:12Z

@ltshb I guess that one is related to the service-print. There is this file that contains the disclaimer: https://github.com/geoadmin/service-print3/blob/c69347869eb7f284886f5352a64aa8042e5f6979/print-apps/mapviewer/Report.properties

@hansmannj how is it to translate the scale and disclaimer on the print ?

hansmannj · 2024-04-02T13:54:50Z

@ltshb I guess that one is related to the service-print. There is this file that contains the disclaimer: https://github.com/geoadmin/service-print3/blob/c69347869eb7f284886f5352a64aa8042e5f6979/print-apps/mapviewer/Report.properties

@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.
Probably @marionb or @sbrunner could help out here?

ltshb

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.

ltshb · 2024-04-02T14:10:57Z

@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.

pakb

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.

github-actions bot added the feature label Mar 21, 2024

ismailsunni force-pushed the feat-PB-111-fix-print-spec branch 2 times, most recently from dcd4ce4 to c76f625 Compare March 26, 2024 03:01

ismailsunni marked this pull request as ready for review March 26, 2024 06:28

ismailsunni requested review from ltshb and pakb March 26, 2024 06:28

ltshb reviewed Mar 26, 2024

View reviewed changes

tests/cypress/tests-e2e/print.cy.js Outdated Show resolved Hide resolved

tests/cypress/tests-e2e/print.cy.js Show resolved Hide resolved

ltshb mentioned this pull request Mar 26, 2024

Move "print" job logic and management out of area composable #737

Merged

ltshb changed the title ~~Feat PB 111: Fix print spec send to the print service~~ PB-111: Fix print spec send to the print service Mar 28, 2024

ismailsunni force-pushed the feat-PB-111-fix-print-spec branch from 39ff8cc to 234ae53 Compare March 28, 2024 06:47

pakb reviewed Mar 28, 2024

View reviewed changes

ismailsunni force-pushed the feat-PB-111-fix-print-spec branch 2 times, most recently from f70e028 to 06511db Compare March 28, 2024 09:36

ismailsunni requested review from pakb and ltshb March 28, 2024 09:42

ltshb reviewed Apr 2, 2024

View reviewed changes

ltshb approved these changes Apr 2, 2024

View reviewed changes

ltshb force-pushed the feat-PB-111-fix-print-spec branch from 2293d86 to a59e753 Compare April 2, 2024 14:11

pakb approved these changes Apr 2, 2024

View reviewed changes

ismailsunni added 20 commits April 3, 2024 10:21

PB-111: Exclude world layer/print area from the map spec.

bca7abb

PB-111: Add GeoAdminCustomizer to exclude some layers from the spec.

59bdbd6

PB-111: Use my fix in geoblocks/mapfishprint from github until they r…

e1a1600

…elease a new version.

PB-111: Add e2e test for print.

1ba8624

PB-111: Update geoblocks/mapfishprint to 0.2.9

de3f04a

PB-111: Add case with different projection.

44b46ad

PB-111: Add e2e test for print with different layers.

8b7b139

PB-111: Reduce delay to fix the failed test

06fb083

PB-111: Increase pageLoadTimeout

dddbf78

PB-111: Separate test case to prevent overtime.

e193c86

PB-111: Add GeoAdminCustomizer to exclude print area rectangle layer.

a2ad316

PB-111: Fix unit test in print.

ef32adc

PB-111: Fix failed to print with grid.

09327cd

PB-111: Use file from fixtures on cypress intercept.

d13d55b

PB-239: Refactor area layer name.

178cc28

PB-111: Add JSDoc use OpenLayers ID not name.

73a65ac

PB-111: Fix unit test after removal of number in Layout.

8a54190

PB-111: Fix missing legend.

4a64c73

PB-111: Add test for legend spec.

c7a470c

PB:111: Update geoblocks/mapfishprint for a WMTS matrix size fix.

c8560ea

ismailsunni force-pushed the feat-PB-111-fix-print-spec branch from a59e753 to c8560ea Compare April 3, 2024 03:21

ismailsunni merged commit 466cbf4 into develop Apr 3, 2024
6 checks passed

ismailsunni deleted the feat-PB-111-fix-print-spec branch April 3, 2024 03:29

cypress bot mentioned this pull request Apr 3, 2024

New Release v1.13.0 - #minor #756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PB-111: Fix print spec send to the print service #727

PB-111: Fix print spec send to the print service #727

ismailsunni commented Mar 21, 2024 •

edited by github-actions bot

Loading

cypress bot commented Mar 21, 2024 •

edited

Loading

pakb Mar 28, 2024

ismailsunni Mar 28, 2024

pakb Mar 28, 2024

ltshb commented Mar 28, 2024

pakb commented Mar 28, 2024 •

edited

Loading

ismailsunni commented Mar 28, 2024

pakb commented Mar 28, 2024

ltshb commented Mar 28, 2024

ltshb commented Mar 28, 2024

ismailsunni commented Mar 28, 2024

pakb commented Mar 28, 2024

hansmannj commented Mar 28, 2024 •

edited

Loading

hansmannj commented Mar 28, 2024

hansmannj commented Mar 28, 2024

hansmannj commented Mar 28, 2024

ismailsunni commented Mar 29, 2024

ltshb left a comment

ltshb commented Apr 2, 2024

hansmannj commented Apr 2, 2024

ltshb left a comment

ltshb commented Apr 2, 2024

pakb left a comment

		* @param {String[]} [config.excludedLayers=[]] List of layer names to exclude from the print.
		* Default is `[]`

PB-111: Fix print spec send to the print service #727

PB-111: Fix print spec send to the print service #727

Conversation

ismailsunni commented Mar 21, 2024 • edited by github-actions bot Loading

cypress bot commented Mar 21, 2024 • edited Loading

Passing run #1432 ↗︎

Review all test suite changes for PR #727 ↗︎

pakb Mar 28, 2024

Choose a reason for hiding this comment

ismailsunni Mar 28, 2024

Choose a reason for hiding this comment

pakb Mar 28, 2024

Choose a reason for hiding this comment

ltshb commented Mar 28, 2024

pakb commented Mar 28, 2024 • edited Loading

ismailsunni commented Mar 28, 2024

pakb commented Mar 28, 2024

ltshb commented Mar 28, 2024

ltshb commented Mar 28, 2024

ismailsunni commented Mar 28, 2024

pakb commented Mar 28, 2024

hansmannj commented Mar 28, 2024 • edited Loading

hansmannj commented Mar 28, 2024

hansmannj commented Mar 28, 2024

hansmannj commented Mar 28, 2024

ismailsunni commented Mar 29, 2024

ltshb left a comment

Choose a reason for hiding this comment

ltshb commented Apr 2, 2024

hansmannj commented Apr 2, 2024

ltshb left a comment

Choose a reason for hiding this comment

ltshb commented Apr 2, 2024

pakb left a comment

Choose a reason for hiding this comment

ismailsunni commented Mar 21, 2024 •

edited by github-actions bot

Loading

cypress bot commented Mar 21, 2024 •

edited

Loading

pakb commented Mar 28, 2024 •

edited

Loading

hansmannj commented Mar 28, 2024 •

edited

Loading