Messagebox feature + letter transfer - #56
Merged
Merged
Conversation
- Rename task name from `process_messagebox_client_response` to `process-messagebox-client-response` - Remove unused `get_base_folder_name` function and simplify file location structure - Update antivirus scanning to use new task name and pass notification_id instead of filename - Make organisation_id optional in response and fix response structure - Remove redundant filename processing loop in favor of direct task call
- Add new debug configurations for non-local API, worker, and beat - Change log level from INFO to DEBUG for celery worker and beat - Add celery flags --without-gossip, --without-mingle, --without-heartbeat - Update VSCode settings to use env file for Python terminal - Add new pre-launch tasks for non-local debugging - Add compound configuration for non-local Notify API debugging - Add tasks for functional test fixtures and platform admin user creation
messagebox_deliver had no guard against re-dispatch, unlike send_sms_to_provider/send_email_to_provider's `if notification.status == "created":` idempotency check. A duplicate or stale trigger would blindly resend to ebms-core, which rejects it as a duplicate BerichtID (BerichtBestaatAl) and can race the real async result.
…real status detail check_if_messagebox_still_pending treated a notification stuck in `sending` the same as one stuck in `created`, blindly resending both. `sending` means ebms-core already accepted the message and we're only waiting on its async result, which has no fixed SLA -- a real pre-prod capture showed this bug in action: the same BerichtID rejected 7 times as a duplicate (BerichtBestaatAl). Split the DAO query so `sending` is never touched for resend, and add a separate 24h Zendesk alert (never resend) so a silently stuck unprocessed-message poller gets noticed early instead of piling up for months. Separately, messagebox_response_map had two real bugs found against captured production responses: three Stadium values were listed as if they were VerwerkingsCode values (they're a different field entirely, so those entries were dead code), and the delivered-status entry expected detailed_status_code "0" when real success responses always carry "Verwerkt". Thread Stadium through as its own parameter instead of folding it into detailed_status_code, and persist both detailed_status_code and the new messagebox_stadium on the notification (new NL migration) plus into notification_history, so this detail survives retention archival instead of being discarded.
Messagebox was entirely absent from delete_notifications_older_than_retention
(only email/sms/letter were wired up), even though ServiceDataRetention and
notification_type already supported it at the DB layer. Add the missing
per-service purge task, reusing _delete_notifications_older_than_retention_by_type
unmodified, and widen the data-retention API schema so a messagebox row can
actually be created.
The compliance-critical part: messagebox purging must never run while a
notification is still in flight, since we need a definitive success or
failure before removing the citizen's BSN, respecting messagebox_deliver's
retry logic. Reuses the exact status-exclusion pattern letters already use
(NOT IN ('pending-virus-check', 'created', 'sending')) rather than the
age-only query email/sms use, so this is gated the same way, not something
bespoke.
Adds the -e ../ebms-adapter-client editable dependency the messagebox channel is built on, plus devcontainer tooling to support it: VS Code tasks for creating/compiling NL migrations and requirements, a run-on-save compile step for requirements_nl*.in, and the recommended extension it needs. Also drops the "Deactivate Spryng provider" task (superseded now that the SMS provider stub round-trips Spryng correctly) and tidies the celerybeat-schedule gitignore pattern to match its -wal/-shm variants.
The Service.oin model field and its use in the messagebox send path (BerichtLeverancierID) were already committed, but the migration that actually creates the column, and schema-level validation (must be exactly 20 digits), were never committed alongside it.
…ring Adds app.clients.messagebox.EbmsAdapterClient (wraps ebms-adapter-client for the send path, decoding attachments and BSN before dispatch) and the supporting config: QueueNamesNL/TaskNamesNL entries, the messagebox Celery beat schedule (poll-unprocessed every 5 min, check-still-pending hourly), EBMS_ADAPTER_URL and the GLOBE-R-BV contract constants, and a dedicated api-worker-messagebox entrypoint. This is the foundation the messagebox Celery tasks (messagebox_tasks.py, messagebox_scheduled_tasks.py, process_messagebox_client_response_tasks.py) already committed on this branch depend on.
Wires MESSAGEBOX_TYPE into the v2 notification type schema and completes the /v2/notifications/messagebox request handling and notification persistence path.
Extends fact_billing_dao's per-service usage queries with a messagebox variant (free/cost-0, matching email's treatment) so messagebox notifications flow into ft_billing instead of being silently excluded from annual/monthly usage reporting.
…essagebox delivery - Allow up to 3 precompiled letter PDFs to be uploaded and merged into a single canonical letter downstream (app/letters/utils.py, app/celery/letters_pdf_tasks.py). - Track detailed Logius failure reasons/codes and stuck-notification retries for messagebox sends (messagebox_tasks, messagebox_scheduled_tasks, process_messagebox_client_response_tasks, ebms_adapter). - Make notifications.to nullable so a messagebox notification's BSN can be wiped once it reaches a terminal state (migrations_nl/0020). - Bump notifynl-utils to 101.2.0.
Bump sentry-sdk to 2.x to pick up enable_logs (Sentry Logs ingestion) and max_request_body_size, and add Boto3Integration for visibility into S3/SQS calls (including kombu's own SQS broker polling). Tag notification_id on Sentry events and attach it as structured `extra` log context across the messagebox send/callback path (messagebox_tasks, client record_outcome, process_messagebox_client_response_tasks, post_notifications_messagebox), so individual notifications become traceable end-to-end. Also re-pins notifications-utils to 101.1.15 and regenerates the NL lockfiles, which pick up defusedxml (transitive via ebms-adapter-client).
Allow up to 2 ad-hoc PDF attachments (base64, 2MB each pre-encoding) on the templated-letter send API, merged into the generated letter after the template's own fixed letter_attachment (if any): - notification_schemas/validators: accept and size-check the new `attachments` field on POST /v2/notifications/letter. - letters/utils: upload attachments to the scan bucket keyed by notification id (upload_letter_attachments/get_letter_attachment_keys), since a templated letter has no canonical filename yet at send time. - post_notifications: decode/upload attachments inside the notification create transaction, then route real-key sends through antivirus (SCAN_LETTER_ATTACHMENTS) while test-key/no-attachment sends go straight to PDF generation as before. - letters_pdf_tasks: three-way virus-scan callback (success/failed/error), mirroring the existing messagebox virus-scan task shape, and include the attachment keys in the payload sent to notifynl-template-preview. - scheduled_tasks: recognise letters stuck pending virus-check by ad-hoc attachments (no single scan-bucket file to check) and auto-retry the scan instead of only falling through to the Zendesk alert.
New `flask command create-letter-template` CLI command for spinning up a basic letter template locally, matching the existing create-sms-template/ create-email-template commands. Wire all three into .vscode/tasks.json (the latter two were previously only reachable via the CLI, not tasks.json).
The rebase onto develop surfaced dead references to transaction(), which develop dropped as part of its SQLAlchemy 2.0 migration. Ports the same call sites over to the _autocommit=False + explicit commit/rollback pattern already used elsewhere in these files.
…st-dotenv pytest-env>=1.2 registers its own --envfile CLI option, which collides with pytest-dotenv's option of the same name and breaks pytest collection entirely when both are installed.
- _messagebox_query referenced start_date/end_date, but _query_for_billing_data's params are named start_dt/end_dt -- NameError on any billing query touching the messagebox notification type. - test_fetch_usage_for_service_annual_includes_messagebox accessed sample_service.id inside the QueryRecorder block, triggering a stray default-session query; hoisted to match the sibling test's pattern.
Marshmallow's @validates callback signature now always passes data_key; every other validator in this file already accepts it, this one was missed when the OIN field was added.
_dispatch_templated_letter_pdf (added for ad-hoc attachment support) omitted MessageGroupId on its celery calls, breaking SQS FIFO ordering per service -- every other dispatch call in this file passes it. Updates the attachment tests that had asserted the missing-MessageGroupId behavior to match. Also suppresses a pre-existing ruff B017 (blind exception assert) that started failing the pre-commit hook after this rebase's ruff version bump, in an unrelated test in the same file -- the blind catch there is intentional (verifying rollback-and-reraise), not a bug.
@pytest.mark.parametrize("reference_paceholder,", ...) was parsed as a
2-name spec because of the trailing comma, which broke collection of the
entire module under the newer, stricter pytest bundled by this rebase.
…nder Org invitation emails hardcoded 'The GOV.UK Notify team' as the inviter name, leaking upstream branding into an otherwise fully NL-translated email.
This error is flashed directly to the end user in notifynl-admin (InviteTokenError -> flash(str(error))), so it should match the rest of the localized admin UI instead of staying in English.
…r message Sibling of the same fix already applied to organisation invitations.
Replaces the UK GOV.UK Notify docs URL with docs.notifynl.nl.
Points to NotifyNL's own trial-mode guidance page (built off ADMIN_BASE_URL) instead of the UK site.
This comment was marked as outdated.
This comment was marked as outdated.
…branding 11 upstream-seeded system templates (MOU receipts, go-live requests/ approvals/rejections, service-join requests, team-member-edit notifications) were never touched by any NL translation migration and were sending verbatim GOV.UK-branded English content in production — same bug class as the already-fixed org/service invitation emails. Also fixes the 'you're now live' email: ConfigNL already points at a dedicated NL template (ec92ba79) but it only had a bare stub, while a fully detailed Dutch translation already existed unused on the old upstream template row (618185c6, migrations_nl/0008). Reuses that content on the correct, live template id instead of re-authoring it. Excludes the DVLA letter-volume template pending a separate decision on its postage-class labels and underlying counting logic.
This comment was marked as outdated.
This comment was marked as outdated.
_get_callback_url built the URL dvla-interceptor POSTs letter-status callbacks back to using API_HOST_NAME, which the notifynl-full chart is about to flip from the internal Service DNS to the public ingress hostname for its genuinely-public consumers (e.g. the unsubscribe link). This callback is pod-to-pod within the same namespace and needs to stay off the public ingress, so it now reads the new API_HOST_NAME_INTERNAL instead -- added to ConfigNL (falls back to API_HOST_NAME) so upstream Config/Test/Development are untouched.
…te to Dutch Personalisation keys were built from item.postage (netherlands/europe/ rest-of-world) but the template still used UK-era placeholders (first_class_volume etc.), so the real report email always showed all-zero counts. Fixes the key mismatch and translates the seeded template to Dutch/NotifyNL branding with the correct NL postage zones.
Stop sending the daily volume-report email to DVLA -- letter delivery via send_dvla_letters_via_api is unaffected. Local dev's LETTER_COLLATION_FREQUENCY schedule (every 5 min) also currently applies to Test/Acc/Prod since no environment overrides it, so this was firing far more often than the intended twice-daily cadence.
Deletes -pr tagged images beyond the newest 10 via snok/container-retention-policy, and publishes a daily tag inventory to the job summary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.