ETT-686: refactor download; add matomo tags and error handling#184
ETT-686: refactor download; add matomo tags and error handling#184carylwyatt wants to merge 10 commits intomainfrom
Conversation
carylwyatt
force-pushed
the
download-with-fetch
branch
2 times, most recently
from
31d0210 to
74717c5
Compare
| const HT = getContext('HT'); | ||
|
|
||
| const formatTitle = {}; | ||
| const formatTitle = $state({}); |
There was a problem hiding this comment.
I updated this component to svelte 5 syntax, so any variable that needs to be reactive is declared with the $state() rune.
| let flattenedSelection = $state([]); | ||
| let clearSelectionLabel = $state('Clear selection'); | ||
|
|
||
| let emptySelection = $derived(range == 'selected-pages' && selection.pages.length == 0); |
There was a problem hiding this comment.
I extracted the error messages and their conditionals out of the submission logic so they could be reactive and update the UI as soon as a condition is met (instead of waiting for the "Download" button to be clicked).
|
|
||
| let allowDownload = manifest.allowSinglePageDownload || manifest.allowFullDownload; | ||
|
|
||
| const _mtm = (window._mtm = window._mtm || []); |
There was a problem hiding this comment.
Added matomo to this component.
|
|
||
| const _mtm = (window._mtm = window._mtm || []); | ||
|
|
||
| let simpleUrl = $derived.by(() => { |
There was a problem hiding this comment.
This was once a tangled mess of conditionals inside submitDownload() and now it's a $derived variable that updates whenever any of the reactive/$state() variables change. The conditionals were extracted to their own function, all this does is construct a URL and return it.
| status = status; | ||
| } | ||
|
|
||
| function trackInterval() { |
There was a problem hiding this comment.
Since we don't really care about the tracker cookie, I removed this tracker/interval altogether.
| } | ||
| } | ||
|
|
||
| function finalizeDownload() { |
There was a problem hiding this comment.
This function was never called. 🤷♀️
|
|
||
| let scriptEl = tunnelWindow.document.createElement('script'); | ||
| scriptEl.type = 'module'; | ||
| let scriptEl = document.createElement('script'); |
There was a problem hiding this comment.
For the downloads that use the JSONP callback from imgsrv, we don't use an iframe for that request anymore, but we do use a script tag that gets appended to the <body>. It's doing essentially the same thing as before, but without with the outdated and sort-of-shady hidden iframe.
|
|
||
| if (selection.pages.length > 0) { | ||
| selection.seq = selection.pages; | ||
| function isSimpleDownload() { |
There was a problem hiding this comment.
Here are the submission conditionals that I extracted from submitDownload().
| // start the download in the iframe | ||
| let scriptEl = tunnelWindow.document.createElement('script'); | ||
| scriptEl.type = 'module'; | ||
| function buildCallbackDownloadUrl() { |
There was a problem hiding this comment.
This is the other half of the logic that was originally in submitDownload().
| action += 'image'; | ||
| sizeAttr = 'size'; | ||
| sizeValue = targetPPI == '0' ? 'full' : `ppi:${targetPPI}`; | ||
| function calculateSelection() { |
There was a problem hiding this comment.
More conditionals that were extracted from submitDownload() and refactored a bit with reactivity in mind.
|
|
||
| function isPartialDownload() { | ||
| return range == 'selected-pages' || range.startsWith('current-page'); | ||
| function submitDownload(e) { |
There was a problem hiding this comment.
Now this function that was over 100 lines is just 24 lines long 🤩 (and most of that is making sure the spinner on the download button stops spinning after the Save As window disappears/download is finished)
| range = 'selected-pages'; | ||
| } | ||
| $: meta = manifest.meta($currentSeq); | ||
| $effect(() => { |
There was a problem hiding this comment.
There are a lot of new $effect()s in this component. In Svelte 5, the $effect rune is reserved for state mutations made to reactive variables. If any reactive $state() variables referenced inside the $effect change, then the whole effect block runs. This can be handy for this kind of tricky UI/state management that we do in the download form. We have so many variations of which form inputs need to be enabled/disabled, appear/disappear, change their wording/value based on the volume itself, the user's role or permissions, and what the user clicks on in the form itself, which view the user selects... it's diabolical.
Some of these effects were updated from the svelte 4 magic $ syntax (which you can see in the red side of the diff), but I added 4 new ones to handle all the state/UI updates necessary for a reactive form.
Generally, svelte suggests not mutating state inside an $effect as it can lead to complicated state management (yes, indeed), but one of the things the docs suggests to use it for is DOM manipulation, which is what we're doing here.
| <label class="form-check-label" for="format-pdf"> Ebook (PDF) </label> | ||
| </div> | ||
| {#if manifest.allowFullDownload} | ||
| {#snippet icon()} |
There was a problem hiding this comment.
Svelte 5 swaps slots for snippets.
| </p> | ||
| {/if} | ||
| </form> | ||
| <form class="d-none" bind:this={tunnelForm} method="GET" {action} target="download-module-xxx"> |
| <!-- do something else --> | ||
| {/if} | ||
| </form> | ||
| <iframe |
There was a problem hiding this comment.
Goodbye hidden iframe!
| Building your {formatTitle[format]} | ||
| {#if $selected.size > 0} | ||
| ({$selected.size} page{$selected.size > 1 ? 's' : ''}) | ||
| {#if simpleDownload} |
There was a problem hiding this comment.
Added some template conditionals for different wording depending on simple/callback download.
| await page.getByLabel('Current page scan (#1)').check(); | ||
| await downloadButton.click(); | ||
|
|
||
| const modalHeading = page.getByRole('heading', { name: 'Download your Image (JPEG)' }); |
There was a problem hiding this comment.
Now that all downloads see the modal, I added the modal portion of the download to all existing playwright tests.
| expect(downloadHeaders['content-type']).toEqual('image/jpeg'); | ||
| expect(downloadBody.length).toBeGreaterThan(1); | ||
| }); | ||
| test('download single selected page tiff, high resolution', async ({ request, page }) => { |
There was a problem hiding this comment.
I added some new imgsrv tests after getting that 403 error for single selection, full-resolution TIFF. I'm still baffled by why this is failing.
carylwyatt
changed the title
carylwyatt
force-pushed
the
download-with-fetch
branch
from
d2a698c to
eba91d5
Compare
carylwyatt
force-pushed
the
download-with-fetch
branch
from
bf664e1 to
cb2d416
Compare
carylwyatt
force-pushed
the
download-with-fetch
branch
from
cb2d416 to
2ae94f8
Compare
|
@carylwyatt I'm not sure if the "tests/sidebar.spec.js:105:5 › sidebar actions › download scans › download selected scans as tiff" failures are a work in progress but they were the low hanging fruit here. The "TIFF downloads are limited to 10 page scans" notice appears to be removed, hence the failure. Commenting out On to the trickier one... |
|
@carylwyatt The "Restricted" message is originating with Does this represent a way forward? Ummm, not sure. I am tremendously ignorant of session management under Playwright. babel/imgsrv/lib/SRV/Volume/Image/Bundle.pm Lines 89 to 105 in 97accac |
@moseshll What's so weird about this test failure is that it starts exactly like all the other I opened the playwright error trace to double check it is indeed going to pageturner and accepting the cookies, so why does As for the other test failure, I cleaned up the other tests that referenced that TIFF error but must've forgotten one! Thanks! |
|
@moseshll I used claude to help me understand the perl from that routine (thanks for the breadcrumb!), and I see now that it's expecting to not have a new/not-empty session. What fixed the tests was to change how the request was made. Playwright has a browser context and an HTTP context. The cookie banner click that accepts the cookies/creates new cookies is in the browser context, but the |
|
@carylwyatt I was about to mention a discovery to you, and then I got caught up in the whole Jira API token discussion with Ryan... yes I had discovered that if you commented out the |
2 participants
Ironically, this
download-with-fetchbranch doesn't actually use the fetch API anywhere after we made the decision to "just use a link" for simple downloads.The tests that are failing are
imgsrvrelated. Some of the requests I wrote return a 403/Restricted, and I can't figure out why.It occasionally happens in the browser when I try to download that scan as a full-resolution TIFF:
And the related request in my docker apache:
After I added the
downloadattribute to the download link, it stopped redirecting when I got the restricted error, and started downloading... nothing. In back-to-back attempts at an identical download of that scan, one was successful and one was not 🤷♀️I haven't run in to this on any other volume, so I'm wondering if this is a problem with the version of imgsrv in docker? It can't just be my machine if these tests are also failing in github actions.
Following up on "simple" downloads that don't actually download
While I was testing the matomo tag for download errors on dev-3, I realized that there is no way to catch errors from broken links. The user makes a selection, clicks "Download," the download modal appears with the second "Download" button. They click the button, it looks like something downloaded... but it's just errors. Do we want to try to mitigate this by reverting back to fetch? It doesn't feel right to assume that as long as the link is correct that
imgsrvwill respond correctly 100% of the time.Testing
This is staged on dev-3. Gayathri already approved the UI/a11y changes, but there are multiple versions of downloads you could test:
All the "simple download" options that should give you a download modal without a progress bar:
everything else should trigger the "build" version of the download modal with the progress ba
Conclusion
bin/run_playwright_tests.sh all) to see if this is just a me problem