From 10f8a3538c99d870aa1e497619f7abfc062dab23 Mon Sep 17 00:00:00 2001 From: ambermartin681 Date: Mon, 31 Aug 2026 13:03:48 +0100 Subject: [PATCH] Move effective_status to service layer, document env vars and test setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #978, #979, #980. - #980: Move effective_status() out of src/api/payment_requests.rs and into src/services/payment_requests.rs as a public function, so any future caller (a webhook handler, a scheduled job) can determine expiry without importing from the API layer. - #979: .env.example now documents TEST_DATABASE_URL (previously undocumented despite being required for real integration test runs) and warns against committing real secrets. .env is already gitignored. - #978: Verified GET /me already returns `email` in MeView and in the Me schema in openapi.yaml — no code change needed, issue was already resolved by prior work. - #977: Added a doc comment block to tests/common/mod.rs explaining the migration-once model, the lack of per-test isolation/teardown, and why parallel test execution is currently safe. The per-test DB isolation implementation (sqlx::test macro, schema-per-test, etc.) is test-authoring work and intentionally left out of this pass. Also fixes the same pre-existing extractor.rs compile break described in PR #1004 (ApiError gained a `field` member; two call sites weren't updated). --- .env.example | 7 ++++++ Cargo.lock | 1 + src/api/payment_requests.rs | 14 ++---------- src/auth/extractor.rs | 4 ++-- src/services/payment_requests.rs | 14 ++++++++++++ tests/common/mod.rs | 39 ++++++++++++++++++++++++++++++++ 6 files changed, 65 insertions(+), 14 deletions(-) diff --git a/.env.example b/.env.example index 74ebc26..72dc048 100644 --- a/.env.example +++ b/.env.example @@ -1,3 +1,5 @@ +# Do not commit this file with real values — copy it to `.env` first +# (`cp .env.example .env`), which is already gitignored. DATABASE_URL=postgres://postgres:postgres@localhost/aframp # Use 0.0.0.0:3000 in a container deployment. APP_BIND_ADDR=127.0.0.1:3000 @@ -19,3 +21,8 @@ CORS_ALLOWED_ORIGINS=http://localhost:3001 # same-origin behind one reverse proxy. See README "Deploying behind TLS". COOKIE_SECURE=true COOKIE_SAME_SITE=lax +# Only read by `cargo test` (see tests/common/mod.rs), never by the running +# server. Point it at a separate database — the integration tests run real +# migrations against it. Unset, the integration tests silently no-op with a +# green "ok" instead of failing; always set this before trusting a test run. +# TEST_DATABASE_URL=postgres://postgres:postgres@localhost:5432/aframp_test diff --git a/Cargo.lock b/Cargo.lock index b8242e2..8293d00 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -52,6 +52,7 @@ dependencies = [ "hmac", "http", "jsonwebtoken", + "mime", "rand 0.8.7", "reqwest", "serde", diff --git a/src/api/payment_requests.rs b/src/api/payment_requests.rs index d8b8bea..e363d5b 100644 --- a/src/api/payment_requests.rs +++ b/src/api/payment_requests.rs @@ -99,16 +99,6 @@ pub struct ListParams { pub limit: Option, } -/// A `pending` row whose expiry has passed is reported as `expired` at read -/// time, so a request going stale needs no background job to flip it. -fn effective_status(status: &str, expires_at: DateTime) -> String { - if status == "pending" && expires_at < Utc::now() { - "expired".to_string() - } else { - status.to_string() - } -} - fn to_view(pr: &PaymentRequest, address: &str, network: &str) -> PaymentRequestView { PaymentRequestView { id: pr.id, @@ -118,7 +108,7 @@ fn to_view(pr: &PaymentRequest, address: &str, network: &str) -> PaymentRequestV amount_stroops: pr.amount_stroops, asset: pr.asset.clone(), memo: pr.memo.clone(), - status: effective_status(&pr.status, pr.expires_at), + status: payment_requests::effective_status(&pr.status, pr.expires_at), expires_at: pr.expires_at, created_at: pr.created_at, sep7_uri: build_sep7_uri(address, pr.amount_stroops, &pr.asset, &pr.memo), @@ -134,7 +124,7 @@ fn row_to_view(row: &payment_requests::PaymentRequestWithWallet) -> PaymentReque amount_stroops: row.amount_stroops, asset: row.asset.clone(), memo: row.memo.clone(), - status: effective_status(&row.status, row.expires_at), + status: payment_requests::effective_status(&row.status, row.expires_at), expires_at: row.expires_at, created_at: row.created_at, sep7_uri: build_sep7_uri(&row.address, row.amount_stroops, &row.asset, &row.memo), diff --git a/src/auth/extractor.rs b/src/auth/extractor.rs index 7913134..4ac2168 100644 --- a/src/auth/extractor.rs +++ b/src/auth/extractor.rs @@ -28,9 +28,9 @@ impl FromRequestParts for AuthUser { .and_then(|v| v.to_str().ok()) .and_then(|v| v.strip_prefix("Bearer ")) .or_else(|| cookie::from_headers(&parts.headers)) - .ok_or_else(|| (StatusCode::UNAUTHORIZED, Json(ApiError { error: "missing session cookie or bearer token".into() })))?; + .ok_or_else(|| (StatusCode::UNAUTHORIZED, Json(ApiError { error: "missing session cookie or bearer token".into(), field: None })))?; let claims = jwt::verify(&state.jwt_secret, token) - .map_err(|_| (StatusCode::UNAUTHORIZED, Json(ApiError { error: "invalid or expired token".into() })))?; + .map_err(|_| (StatusCode::UNAUTHORIZED, Json(ApiError { error: "invalid or expired token".into(), field: None })))?; Ok(AuthUser { user_id: claims.sub, merchant_id: claims.merchant_id, diff --git a/src/services/payment_requests.rs b/src/services/payment_requests.rs index 9b08976..0a242f2 100644 --- a/src/services/payment_requests.rs +++ b/src/services/payment_requests.rs @@ -147,3 +147,17 @@ pub async fn mark_partial(db: &PgPool, id: Uuid, payment_id: Uuid) -> Result<(), .await .map(|_| ()) } + +/// A `pending` row whose expiry has passed is reported as `expired` at read +/// time, so a request going stale needs no background job to flip it. Lives +/// here rather than in the API layer so any caller that needs to know whether +/// a request is effectively expired — a webhook handler, a scheduled job, a +/// new endpoint — can call it without importing from `api::payment_requests`, +/// an inversion of the normal API → service dependency direction. +pub fn effective_status(status: &str, expires_at: chrono::DateTime) -> String { + if status == "pending" && expires_at < Utc::now() { + "expired".to_string() + } else { + status.to_string() + } +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index e217ec8..8a3ca7a 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,3 +1,42 @@ +//! Shared setup for the end-to-end flow tests in this directory +//! (`auth_flow.rs`, `wallet_flow.rs`, `payment_request_flow.rs`, +//! `withdrawal_flow.rs`). +//! +//! **Migrations, once per process.** [`state()`] runs `sqlx::migrate!()` +//! exactly once (guarded by [`MIGRATED`]/[`MIGRATION_LOCK`]) the first time +//! any test calls it, then hands out a pool to a database that already has +//! the full schema. Every test file in this crate shares one `cargo test` +//! process, so this only runs once per `cargo test` invocation, not once per +//! test. +//! +//! **No per-test isolation.** There is currently no schema-per-test, +//! transactional rollback, or truncation between tests: every test that +//! calls [`state()`] shares one physical database and its rows persist +//! across tests. That is why every helper that creates a merchant +//! ([`ensure_merchant`]) generates a fresh random email — tests avoid +//! collisions by never reusing identity, not by the database resetting +//! itself. A test that lists or counts rows scoped to something other than +//! its own freshly created merchant/wallet/etc. will observe leftovers from +//! every other test that has run against the same database. +//! +//! **Parallelism.** Rust runs test functions concurrently by default +//! (`cargo test -- --test-threads=N`). That is safe here specifically +//! *because* of the point above — tests only assert against data scoped to +//! identifiers they just created — but it means adding a test that queries +//! unscoped state (e.g. "assert exactly one payment exists") would be a race +//! against every other test in the suite, not a bug in the runner. +//! +//! **No teardown.** Nothing here deletes rows after a test runs. The target +//! database (`TEST_DATABASE_URL`) is treated as disposable and expected to +//! accumulate rows across runs; drop and recreate it if that accumulation +//! ever matters (e.g. before a run that asserts on total row counts). +//! +//! **Silent skip.** [`state()`] returns `None` — not a panic — when +//! `TEST_DATABASE_URL` is unset or unreachable, and every test built on it is +//! written to pass trivially in that case. See the README's "Running tests" +//! section: a fully green `cargo test` can mean "everything passed" or +//! "nothing ran," and only setting `TEST_DATABASE_URL` distinguishes them. + use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex};