ETT-1241: fix section 108 access #177

aelkiss · 2026-01-14T19:32:39Z

This PR:

fixes section 108 access (from a bug introduced here: https://github.com/hathitrust/babel/pull/116/changes#diff-508a0f9c6e9ef9e4fbc2b3ff4aa062c8221b537df29287228f5cc8620a196c3cR285)
adds playwright tests for ETAS and section 108 & tries to make authenticated tests less flaky
removes the apache vs. apache-test distinction
fixes an issue in the setup script
fixes an issue regarding the catalog health check for php 8 (although with removing the apache vs. apache-test distinction I'm not sure it's used)

ln shouldn't be in environment

aelkiss · 2026-01-14T19:36:57Z

@carylwyatt @Ronster2018 See 39b2c1d.

I think the main blocker to just using the regular apache service (rather than apache-test) is that we have a health check configured for the catalog; previously, the catalog repo was private, so we couldn't easily check it out here.

I think we could maybe think about removing some of the health checks for optional things (i.e. the things we don't have automated playwright tests for anyway), controlling bringing the rest of it up via profile instead (e.g. a 'test' profile vs a 'everything' profile), and get rid of apache vs. apache-test.

I may see what I can do here with that, because I think it will simplify trying to develop these ETAS & section 108 tests.

carylwyatt · 2026-01-14T19:44:27Z

Yes, please. The apache-test service has led to headaches for me, and I would love to get it out of here if possible. I think you're right that we only added it for playwright testing but I don't remember specifically why. If it was because of the catalog, then I agree, now that it's public and can be checked out, let's do that if we can!

aelkiss · 2026-01-15T19:29:35Z

The playwright tests that were failing were an experiment that accidentally got committed. I think they should be fine.

This should eliminate the issue of apache vs. apache-test and fighting over port numbers. One potential drawback is that the auth state won't be retained after running the tests, but that was the case currently too for apache-test.

aelkiss · 2026-01-15T21:31:14Z

@Ronster2018 I'm hoping you can take a look at the docker compose changes.
@carylwyatt I'm hoping you can look at the docker compose changes and make sure it works for your workflows as well as the playwright tests
@moseshll I'm hoping you can look at the (very small) change to fix section 108 access, and also at the playwright tests

aelkiss · 2026-01-15T21:32:13Z

bin/run_playwright_tests.sh

-  echo -e "Resetting ht_sessions database table "
-  docker compose exec mysql-sdr mariadb -u mdp-lib -pmdp-lib -h localhost ht -e "DELETE FROM ht_sessions;"
+  echo -e "Resetting ht_sessions & pt_exclusivity_ng database tables"
+  docker compose exec mysql-sdr mariadb -u mdp-lib -pmdp-lib -h localhost ht -e "DELETE FROM ht_sessions; DELETE FROM pt_exclusivity_ng;"


Need to clear pt_exclusivity_ng to start with a blank slate for ETAS and section 108 testing. Ideally, could clear it before every test; not sure we have a good way to do that; should work as-is though.

aelkiss · 2026-01-16T13:45:19Z

Playwright tests for section 108 access are failing. I'm not exactly sure what's missing, but I'll look more.

* Sample item op_brlm with requisite holdings * Limit authed tests to one worker - parallel workers will run into race conditions with the item already being checked out, etc. * Run authed tests only with chromium -- we're mainly testing application logic, not browser display * Restart apache with HUP rather than USR1 & add delay - may help with more reliably reloading config completely before running tests

aelkiss · 2026-01-16T17:40:26Z

Was missing the holdings API response for the brittle item. Hopefully this should be fixed now & playwright tests will pass here.

carylwyatt · 2026-01-16T19:49:02Z

When I first pulled this down to test out docker, catalog wasn't working correctly. There was an error that referenced php 8, so I just ran ./setup.sh and built it again. The catalog started working, but now I'm getting a familiar GeoIP error in the cgi/babel apps: Error opening database file "/htapps/babel/geoip/GeoIP2-Country.mmdb": Error opening the specified MaxMind DB file at /htapps/babel/mdp-lib/Utils/GeoIP.pm line 27

I did a docker system prune and setup/built again, but it didn't fix the GeoIP error. What am I missing?

aelkiss · 2026-01-16T21:03:57Z

I did a docker system prune and setup/built again, but it didn't fix the GeoIP error. What am I missing?

setup.sh should be putting this in place, but there was an issue until this PR: https://github.com/hathitrust/babel/pull/177/changes#diff-4209d788ad32c40cbda3c66b3de47eefb929308ca703bb77a6382625986add17R44 (note the misplaced EOT for the end of the heredoc).

Regardless I might have to see what's going on. Maybe ln behaves differently on Mac or something such that ln -rsv geoip/always_us.mmdb geoip/GeoIP2-Country.mmdb doesn't do what it does on Linux?

moseshll · 2026-01-19T17:29:14Z

setup.sh

 BABEL_HOME="$(dirname $(realpath $0))"
+EOT

 ln -rsv geoip/always_us.mmdb geoip/GeoIP2-Country.mmdb


My ln does not like the -r option. I had to remove it.

Does the symlink work correctly without the -r option?

Apache local didn't work for me until I changed the command to ln -svf always_us.mmdb geoip/GeoIP2-Country.mmdb (using the -f flag from geoip/README.md)

Thanks, updated in this branch. @carylwyatt see if that works for you

I've tested this branch, and it works well in my environment.

moseshll · 2026-01-19T19:43:37Z

I too had issues, even with the updated ln command, but was able to run all of the playwright tests only after running Martin's "nuke Docker" script.

It also helped that I somewhat belatedly remembered to update imgsrv sample data :-)

carylwyatt · 2026-01-20T19:16:20Z

It took me many attempts to get all the right pieces lined up in the correct order, but this is working! Catalog, pt, ls, mb, all loaded. I had no problem running my usual firebird and pt commands, and I ran playwright without issue.

Probably unrelated to this PR, but the playwright tests failed on some of the full resolution TIFF downloads, and honestly it's the perfect timing for adding that try/catch block there, so I'm kind of glad those tests failed.

I have never seen this response for a download before!

Request URL: http://localhost:8080/cgi/imgsrv/download/image?id=test.pd_open&attachment=1&tracker=D1&format=image%2Ftiff&target_ppi=0&seq=2

Response:

<html>
    <body>
        <p>Your download is in progress. Please try again later.</p>
        <p>Packing images...</p>
        <p>Status: 2 / 1</p>
        <p>
            <a href="http://localhost:8080/cgi/imgsrv/download/volume/image?attachment=1&tracker=D1&id=test.pd_open&seq=2&format=image%2Ftiff&target_ppi=0">Download</a>
        </p>
    </body>
</html>

Which seems to be related to this _in_progress_alert from imgsrv: https://github.com/hathitrust/babel/blob/85a632055b26e4b5b9ab8d45654d66f74e87d469/imgsrv/lib/SRV/Volume/Base.pm#L564C1-L590C2

It's a 200 response, but for whatever reason, this "in progress"/"packing images..." message is sent to pt, but ~~pt doesn't do anything with that information. I cmd+F in the inspector tools to see if this message is hidden in an iframe or modal or something, but it's not present in the markup.~~ EDIT: Yes, this HTML response is injected into the download iframe. This isn't really an error since it's a 200? 🥴 But it's blocking the download (infinite spinning download button), and because the download button is disabled, I can't click it again.

Of course when I try to get it to happen again, the tests pass and everything is normal in the browser. 😡

moseshll

I verified that a unit test would have caught that $access_type vs $user_access_type error I introduced. Stay tuned for a proof of concept branch. This all looks good.

EDIT: I did get some PIFiller tests on the go, but I'll be darned if I can get Global symbol "$user_access_type" requires explicit package name (did you forget to declare "my $user_access_type"?) to show up now, short of slapping a use strict in the source file itself, which is not exactly what I intended (but which we should probably do for the whole of mdp-lib at least)!

aelkiss added 2 commits January 14, 2026 13:37

Fix typo in setup script for geoip

a7c08a4

ln shouldn't be in environment

ETT-636: update catalog-in-babel health check for php 8

2a57dd2

aelkiss added 2 commits January 15, 2026 14:34

Fix typo for querying for BRLM access (from ETT-320)

a0902b7

Get rid of apache vs. apache-test distinction

e14da11

This should eliminate the issue of apache vs. apache-test and fighting over port numbers. One potential drawback is that the auth state won't be retained after running the tests, but that was the case currently too for apache-test.

aelkiss force-pushed the ETT-1241-section108 branch 2 times, most recently from 3c711c2 to f4bfef2 Compare January 15, 2026 21:25

aelkiss requested review from Ronster2018, carylwyatt and moseshll and removed request for carylwyatt January 15, 2026 21:25

aelkiss force-pushed the ETT-1241-section108 branch from f4bfef2 to 6ed770c Compare January 15, 2026 21:30

aelkiss commented Jan 15, 2026

View reviewed changes

aelkiss force-pushed the ETT-1241-section108 branch from 6ed770c to bf44eac Compare January 15, 2026 21:34

aelkiss marked this pull request as ready for review January 15, 2026 21:35

aelkiss force-pushed the ETT-1241-section108 branch from bf44eac to 71b169f Compare January 16, 2026 17:40

moseshll reviewed Jan 19, 2026

View reviewed changes

fix geoip link in setup.sh for mac os x ln (no -r option)

a9154fd

moseshll self-requested a review January 21, 2026 16:10

moseshll approved these changes Jan 21, 2026

View reviewed changes

ETT-1241: fix section 108 access #177

Are you sure you want to change the base?

ETT-1241: fix section 108 access #177

Uh oh!

Conversation

aelkiss commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelkiss commented Jan 14, 2026

Uh oh!

carylwyatt commented Jan 14, 2026

Uh oh!

aelkiss commented Jan 15, 2026

Uh oh!

aelkiss commented Jan 15, 2026

Uh oh!

aelkiss Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

aelkiss commented Jan 16, 2026

Uh oh!

aelkiss commented Jan 16, 2026

Uh oh!

carylwyatt commented Jan 16, 2026

Uh oh!

aelkiss commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moseshll Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

aelkiss Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

moseshll Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

aelkiss Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

liseli Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

moseshll commented Jan 19, 2026

Uh oh!

carylwyatt commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moseshll left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aelkiss commented Jan 14, 2026 •

edited

Loading

aelkiss commented Jan 16, 2026 •

edited

Loading

carylwyatt commented Jan 20, 2026 •

edited

Loading

moseshll left a comment •

edited

Loading