[stevenwinship]

What this PR does / why we need it: External tools can't download files that require/have a guestbook response

Which issue(s) this PR closes:#12760

• Closes Previewers no longer working on files with required guestbooks, due to the API changes (since 6.10) #12760

Special notes for your reviewer:

Suggestions on how to test this: Create a dataset with multiple files to preview. Add a guestbook to the dataset and publish. Test previewing the files and download from the external tool. Test files with restricted access.
Download timeout comes from the external tool params. Try creating an external tool with a 1 minute timeout and make sure the signed url expires.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?: included

Additional documentation:

[coveralls]

Coverage is 25.664% — 12760-previewers-not-working-with-guestbook-responses into develop. No base build found for develop.

[github-actions]

Test Results

417 tests  ±0   403 ✅ ±0   31m 36s ⏱️ -18s
 59 suites ±0    14 💤 ±0 
 59 files   ±0     0 ❌ ±0 

Results for commit 5f7fe78. ± Comparison against base commit 3cbdeab.

♻️ This comment has been updated with latest results.

[landreev]

Could you please sync the branch up with develop.

[landreev]

@stevenwinship
It may be something I'm doing, but it's still not working for me.
I have an audio file with a guestbook and the audio previewer installed locally. Looking at the javascript console, the callback url doesn't appear to have the new guestbookresponse id parameter:

http://localhost:8080/previewers/v1.5/AudioPreview.html?callback=aHR0cDovLzEyNy4wLjAuMTo4MDgwL2FwaS92MS9maWxlcy8xNi9tZXRhZGF0YS8yL3Rvb2xwYXJhbXMvNA==&locale=en&preview=true

decoded:

echo "aHR0cDovLzEyNy4wLjAuMTo4MDgwL2FwaS92MS9maWxlcy8xNi9tZXRhZGF0YS8yL3Rvb2xwYXJhbXMvNA==" | base64 -d 
http://127.0.0.1:8080/api/v1/files/16/metadata/2/toolparams/4

This in turn produces an /access url without the gbrid, which bombs with a 400.

Please advise.

[stevenwinship]

Did you install your own external tool? If so did you include "gbrecs=false" or "gbrecs=true" in the allowedapicalls?
I don't see it in your urls above.

This is what it looks for to add the gb response id. I'll look into changing it so it gets added either way

[{"name":"retrieveFileContents","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true","timeOut":3600},{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=false","timeOut":3600},{"name":"getDatasetVersionMetadata","httpMethod":"GET","urlTemplate":"/api/v1/datasets/{datasetId}/versions/{datasetVersion}","timeOut":3600}]

[landreev]

I installed the tool using the standard command as published. Which in my case was:

curl -X POST -H 'Content-type: application/json' http://localhost:8080/api/admin/externalTools -d \
'{                                            
  "displayName":"Play Audio",    
  "description":"Listen to an audio file.",
  "toolName":"audioPreviewer",
  "scope":"file",                    
  "types":["preview"],    
  "toolUrl":"http://localhost:8080/previewers/v1.5/AudioPreview.html",
  "toolParameters": {                
      "queryParameters":[ 
        {"fileid":"{fileId}"},                                      
        {"siteUrl":"{siteUrl}"},
        {"datasetid":"{datasetId}"},
        {"datasetversion":"{datasetVersion}"},                       
        {"locale":"{localeCode}"}
      ]                   
    },                                                               
  "contentType":"audio/mpeg",             
  "allowedApiCalls": [    
    {                                                                         
      "name": "retrieveFileContents",     
      "httpMethod": "GET",
      "urlTemplate": "/api/v1/access/datafile/{fileId}?gbrecs=true",          
      "timeOut": 3600 
    },                    
    {                                                               
      "name": "downloadFile",   
      "httpMethod": "GET",          
      "urlTemplate": "/api/v1/access/datafile/{fileId}?gbrecs=false",
      "timeOut": 3600            
    },                    
    {                                                                
      "name": "getDatasetVersionMetadata",
      "httpMethod": "GET",
      "urlTemplate": "/api/v1/datasets/{datasetId}/versions/{datasetVersion}",
      "timeOut": 3600                     
    }                     
  ]                                                                           
}'

seems to be the same - i.e., gbrecs=true for the retrieveFileContents (the main access url that the player uses), and gbrecs=false for the download one, just like in your command below. (but please double-check)
If the installation commands need to be changed/tools need to be reinstalled, that will need to be documented in the release note.

Did you install your own external tool? If so did you include "gbrecs=false" or "gbrecs=true" in the allowedapicalls? This is what it looks for to add the gb id. I'll look into changed it so it gets added either way

[{"name":"retrieveFileContents","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true","timeOut":3600},{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=false","timeOut":3600},{"name":"getDatasetVersionMetadata","httpMethod":"GET","urlTemplate":"/api/v1/datasets/{datasetId}/versions/{datasetVersion}","timeOut":3600}]

[landreev]

Do I need to re-test, with your latest commits from 1 hour ago? Or are these the same initial commits from force-pushing the branch?

I'm getting errors/warning from git pull origin 12760-previewers-not-working-with-guestbook-responses as well:

hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
...

[landreev]

... otherwise I can report another issue that I may be seeing.

[stevenwinship]

My latest commit was just a merge from develop

[landreev]

I figured; but they are showing as new commits this am, above. I'm assuming because of a force-push?

Since we do want/need to have the gbrids= in the main access url, it should probably have gbrecs=false, not true - (false means "do not write a new gbr", right?)

[qqmyers]

FWIW: ignoring when a gb is assigned, gbrecs determines whether an entry is made that signifies a download, so the main preview needs/needed true to avoid the API writing a download count. At some point, we decided that a preview is close enough to a download that we want to ask the guestbook questions, but I don't think we ever decided that, in the no guestbook case, previews should count as downloads.

[landreev]

In other words, I was wrong above - in that gbrecs=true actually means "do NOT write a new gbr"?

On a side note, I'm not seeing either gbrecs or gbrids mentioned anywhere in the guides. (am I missing something?)

FWIW: ignoring when a gb is assigned, gbrecs determines whether an entry is made that signifies a download, so the main preview needs/needed true to avoid the API writing a download count. At some point, we decided that a preview is close enough to a download that we want to ask the guestbook questions, but I don't think we ever decided that, in the no guestbook case, previews should count as downloads.

[landreev]

In other words, I was wrong above - in that gbrecs=true actually means "do NOT write a new gbr"?

Of course; I misread the code in Access.java earlier.

[stevenwinship]

I've been trying to reproduce your (@landreev) error using both .wav and .mpeg files. I am getting the dialog to enter the gb info and then I get the download to happen correctly with the id

{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true&gbrids=4","timeOut":3600},

Note: I can preview the wav file but I don't see the "eye" with the mpeg

[stevenwinship]

Made the change to pass the guestbook response instead of the id. The guestbook response will be written when the toolUrl is generated.

[landreev]

Made the change to pass the guestbook response instead of the id. The guestbook response will be written when the toolUrl is generated.

Looks good, retesting now.
One quick question: if /toolUrl is called on a file with an explicit guestbook defined, but without supplying the response, should it fail with a 400, like the /api/access/datafile api?

[landreev]

And hopefully the last thing - when I use a previewer in JSF, the gbr recored has an eventtype = 'Explore'. As currently implemented, the gbr produced by /toolUrl is of type 'Download'. One of the advantages of switching to passing response values to /toolUrl was that we'd be able to keep the API behavior identical to JSF.

Functionally everything is working as far as I can tell.

Made the change to pass the guestbook response instead of the id. The guestbook response will be written when the toolUrl is generated.

Looks good, retesting now. One quick question: if /toolUrl is called on a file with an explicit guestbook defined, but without supplying the response, should it fail with a 400, like the /api/access/datafile api?

[landreev]

Hi, I'm not seeing any new commits since my last comments - were there supposed to be new changes?

[landreev]

Note that AbstractApiBean already had guestbookResponseService included, under a different name.
Also, there's a SonarCloud warning further down about Files.java duplicating guestbookResponseService under the same name.

[stevenwinship]

fixed

[landreev]

There is a whole bunch of SonarCloud warnings like this one about "useless assignments" in the test classes.
I'm not sure whether all of them are in fact useless; this variable doesn't seem to be used anywhere, but I'm guessing it's here to test for an exception (?).
Can these SonarCloud warnings be marked resolved or dismissed??

[stevenwinship]

removed

[stevenwinship]

And hopefully the last thing - when I use a previewer in JSF, the gbr recored has an eventtype = 'Explore'. As currently implemented, the gbr produced by /toolUrl is of type 'Download'. One of the advantages of switching to passing response values to /toolUrl was that we'd be able to keep the API behavior identical to JSF.

If we write a guestbook response with 'Explore' and the tool does a download, we will never get a 'Download' event. I'm not sure how to handle that

[landreev]

And hopefully the last thing - when I use a previewer in JSF, the gbr recored has an eventtype = 'Explore'. As currently implemented, the gbr produced by /toolUrl is of type 'Download'. One of the advantages of switching to passing response values to /toolUrl was that we'd be able to keep the API behavior identical to JSF.

If we write a guestbook response with 'Explore' and the tool does a download, we will never get a 'Download' event. I'm not sure how to handle that

It didn't sound like there was anything we could do to record the extra download (if the user decides to download a file that has an explicit guestbook defined from within the viewer). There was a discussion about that somewhere above.

But all I'm requesting is to make the /toolUrl API behave identically to what the internal logic in jsf is doing: when a user clicks on the previewer there, it creates a GBR of type "Explore". It doesn't seem controversial to have the exact same behavior in /toolUrl. I.e., have the same end result in jsf and /modern.

[landreev]

Hey, it sounds like you have a lot going on (I hope the kitten's ok too!).
But you keep un-assigning yourself from this PR, even though there are still 2 unaddressed comments (on top of the one I just replied about). At least one of which I've tried to bring up repeatedly now.
Sorry if I am missing something on my end.

[landreev]

#12801 (comment)

#12801 (comment)

[sonarqubecloud]

Quality Gate failed

Failed conditions
10.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[github-actions]

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:12760-previewers-not-working-with-guestbook-responses

ghcr.io/gdcc/configbaker:12760-previewers-not-working-with-guestbook-responses

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.