Fix restoration emails

[mjiori]

This PR enables an Alert to be sent when a Restoration is complete (Successful status).

The Alert will appear in the Notifications section of Registry for Institutional Users and Admins. Additionally, an email will be sent to the same, following the restoration_completed.txt template which was pre-existing.

I repurposed code for alerting on completion of a spot test restore to also alert when any regular restoration is completed. Tests were also modified to ensure that the template reflects the correct data - the name of the user who requested the restore, the name, and the location.

All Registry tests are passing.

[mjiori]

Claude Opus 4.7 Code Review (high effort):

Here's my code review of commits 354ae9b and 5b9fba0 (both touching pgmodels/work_item.go and pgmodels/work_item_test.go).

Summary

The change extends AlertOnSuccessfulSpotTest to also alert for all successful object restorations (not just spot tests) and renames it to
AlertOnSuccessfulRestore. A new code path was inlined into the existing function, and a new test helper testAlertOnSuccessfulRestore was added.

Bugs

1. Nil-pointer crash on the new path (work_item.go:338)

The "regular restoration" branch is gated by inst == nil || inst.ID == 0 || obj == nil || obj.ID == 0. Inside that branch, if UserSelect returns an
error, the log line does:

ctx.Log.Error().Msgf("AlertOnSuccessfulRestore: Error getting users for institution %s: %v", inst.Identifier, err)

inst is nil here in the common case — this will panic. Same issue if inst.ID == 0 but inst is non-nil and inst.Identifier is set, that's fine; but
the inst == nil case is reachable (it's literally the gate). Use item.InstitutionID instead.

2. GetSpotTestDetails swallows IsNoRowError and returns nil error, but the rename's docstring no longer makes sense. Minor — not a behavior bug, but
   the comment "GetSpotTestDetails returns true if…" was already stale (it doesn't return a bool) and is more confusing now that callers use it for
   non-spot-test paths.

3. Content: "Your fries are ready." in the new Alert struct (work_item.go:378). This looks like placeholder/joke text. CreateAlert likely overwrites
   Content from the template, but if it doesn't and that string ends up in any DB row or notification, it would be visible to customers. Confirm
   CreateAlert always replaces it; either way, change the literal to "" or remove the field. The pre-existing spot-test branch has the same string —
   that one was presumably safe because templates overwrite it, but it's worth verifying once and removing the joke.

Design / structure

4. Massive code duplication between the two branches. The new "regular restoration" block (work_item.go:335–390) is ~95% identical to the spot-test
   block (work_item.go:392–433): user query, note parsing, alert struct, CreateAlert call, logging. The only real differences are:

• Template name (restoration_completed.txt vs restoration_spot_test_completed.txt)
• Subject string
• alertData contents (RequesterName vs SpotTestDays/RegistryURL)
• InstitutionID source (obj.InstitutionID vs inst.ID) — these should be equal anyway

This belongs as a single block with a small if isSpotTest { … } else { … } for the differences, or two thin helpers. As written, any future fix has
to be made in two places.

5. Control flow is inverted. The function reads as: "if NOT a spot test, do the entire alert flow and return; otherwise fall through to the
   spot-test flow." Reversing it — handle spot test inside the gate, then fall through to the regular path — would let the function share the user
   query, note parsing, and CreateAlert call across both cases.

6. obj is shadowed at work_item.go:352. inst, obj, err := item.GetSpotTestDetails() returns obj already; the new branch re-fetches it with obj, err
   := IntellectualObjectByID(item.IntellectualObjectID) using :=, creating a new local. Since the gate guarantees the outer obj is nil here, the
   re-fetch is needed — but the shadowing is confusing. Either rename or restructure so there's a single fetch.

7. URL parsing is fragile. strings.Split(item.Note, " restored to ") plus urlWithTrailingPeriod[0 : len(urlWithTrailingPeriod)-1] assumes the Note
   ends with exactly one period. If the format ever changes (no trailing period, or trailing whitespace), you slice off a real character or panic on
   empty string. This is duplicated in both branches. A strings.TrimSuffix(parts[1], ".") is safer. (Pre-existing in the spot-test path; the new copy
   doubles the exposure.)

8. requester is dereferenced without a nil check (work_item.go:369). UserByEmail could plausibly return (nil, nil) if the user was deleted.
   requester.Name would panic. Add a nil guard alongside the error check.

Tests

9. The test comment at work_item_test.go:516 is wrong. "Should not alert, because this is not a successful restore" — actually the item is an ingest
   (not a restore at all), and AlertOnSuccessfulRestore returns nil because item.Action != ActionRestoreObject. The old comment ("not a spot test")
   was clearer about why.

10. Test ordering hazard. TestIsRestorationSpotTest now calls testAlertOnSuccessfulRestore before linking the institution, then mutates the
    institution to make it a spot test. If CreateAlert persists the alert to the test DB, the regular-restore alert is now in DB state when subsequent
    assertions run. Verify the test fixtures get reset, or this could flake when run with -count=2 or alongside other tests.

11. Coverage gap. No test exercises the new error paths (UserSelect failure, IntellectualObjectByID nil/error, UserByEmail failure, malformed Note).
    The whole point of commit 5b9fba0 is "adding error handling" — but the handling itself isn't tested.

Naming / docs

12. GetSpotTestDetails should be renamed or split. It's now load-bearing for the non-spot-test path too: callers use (inst == nil || obj == nil) as
    the signal for "regular restoration." That's an awkward overload of a function still named for spot tests.

13. Docstring on AlertOnSuccessfulRestore says "sends an email to institutional users and admins" — but admins aren't in the user query
    (institution_id == item.InstitutionID only). Either the docstring is wrong or admin notification is missing.

Recommendation

Fix #1 (the nil-pointer log) and #3 (the "fries" string) before merging — those are user-visible. The rest is cleanup that would be much easier to
land before the duplication ossifies: I'd refactor the function into one shared flow with a spot-test branch rather than two parallel copies.

+check for object vs file restorations