Implement tool installation request form and notification/email system - #22259
Implement tool installation request form and notification/email system#22259arash77 wants to merge 70 commits into
Conversation
c0b39cc to
db2f14a
Compare
davelopez
left a comment
There was a problem hiding this comment.
Looking pretty solid already!
Some comments for potential improvements below.
mvdbeek
left a comment
There was a problem hiding this comment.
I like most of it, but it'd be awesome if you can use the notifications API instead of a bespoke API for this.
| */ | ||
| ToolRequestFormData: { | ||
| /** | ||
| * Conda available |
There was a problem hiding this comment.
That doesn't seem like something a user would know, and even if they did we can't trust that so it's just more work, and even if there is a package it doesn't help if it's in some random channel.
There was a problem hiding this comment.
Removed in 3f22971f9. No conda_available field anywhere in the current schema; test asserts absent (ToolRequestForm.test.ts:117).
| * Requester affiliation | ||
| * @description The affiliation/lab of the requester. | ||
| */ | ||
| requester_affiliation?: string | null; |
There was a problem hiding this comment.
I'd just add an additional remarks field here. If they use OIDC or institutional email we can get that from their user profile.
There was a problem hiding this comment.
Replaced with additional_remarks (notifications.py:147, notifications.ts:41,67). requester_email is derived server-side from the authenticated user's profile.
| * Test data available | ||
| * @description Whether test data for this tool is available. | ||
| */ | ||
| test_data_available?: boolean | null; |
There was a problem hiding this comment.
Let's not do that, same reason as the conda check
There was a problem hiding this comment.
Removed in 3f22971f9. No test_data_available field; test asserts absent (ToolRequestForm.test.ts:118).
| * Tool name | ||
| * @description The name of the requested tool. | ||
| */ | ||
| tool_name: string; |
There was a problem hiding this comment.
This is a string, but tool ids is an array, so that doesn't line up
There was a problem hiding this comment.
Now tool_names: list[str] with min_length=1 (notifications.py:122-124); form sends array [toolName.value.trim()] (ToolRequestForm.vue:86); TypeScript type tool_names: string[] (notifications.ts:34,61).
| * Workflow name | ||
| * @description Name of the workflow requiring these tools, if applicable. | ||
| */ | ||
| workflow_name?: string | null; |
There was a problem hiding this comment.
A workflow id that we should have in the context is probably more useful ?
There was a problem hiding this comment.
Done in 252a8d2. WorkflowMissingToolsRequest.vue:13-16 takes workflowId as a prop and submits it as workflow_id at line 48 (schema field notifications.py:144).
| </BAlert> | ||
|
|
||
| <BAlert v-if="submitted || alreadyRequested" variant="success" show> | ||
| Installation request sent — admins have been notified and will review it shortly. |
There was a problem hiding this comment.
Don't the notifications have an ID ? Seems like we could link to the actual notification message
There was a problem hiding this comment.
submitUserNotification now returns the encoded notification id (notifications.ts:73-93); the component captures it (WorkflowMissingToolsRequest.vue:46,52) and renders a link: <BLink :href="/user/notifications#notification-card-${submittedNotificationId}">view your request</BLink> (line 70). The anchor target is the id on each card (NotificationCard.vue:157).
| <BAlert v-if="workflowError" variant="danger" show> | ||
| <h2 class="h-text">Workflow cannot be executed. Please resolve the following issue:</h2> | ||
| {{ workflowError }} | ||
| <WorkflowMissingToolsRequest :missing-tool-ids="missingToolIds" :workflow-name="workflowName" /> |
There was a problem hiding this comment.
Doesn't that need to be guarded by whether the feature is enabled ?
There was a problem hiding this comment.
Yes — <WorkflowMissingToolsRequest v-if="config?.enable_tool_request_form" ...> (WorkflowRun.vue:242-243). The component itself double-guards (WorkflowMissingToolsRequest.vue:26-32): isConfigLoaded && config.enable_tool_request_form && !userStore.isAnonymous.
3b5d3da to
21aad9f
Compare
|
I will rename this feature to P.S. Done! |
b0942f1 to
d18b8c7
Compare
c975acc to
8d27d61
Compare
Changes submitToolInstallationRequest to return void instead of notification ID. Replaces 'view your request' links with generic success messages to align with async Celery behavior.
The mock function already returns undefined by default when created with vi.fn(), so explicitly calling mockResolvedValueOnce(undefined) is unnecessary. This cleanup removes all such redundant calls from both ToolInstallationRequestForm and WorkflowMissingToolsRequest test suites.
When enable_celery_tasks=True, send_notification now uses force_sync=True to create notifications synchronously (returning NotificationCreatedResponse with the notification ID) while email delivery remains async via the Celery beat task. This fixes the CI test failure in test_submission_creates_notification_without_sending_admin_email_synchronously. The change only affects user-initiated submissions through the send_notification service method (tool installation requests with small recipient sets). Bulk broadcasts and internal callers remain unchanged.
… integration tests to support async creation
templates.render now accepts an autoescape flag (default False) so the historical raw-rendering behavior of existing callers (e.g. the activation email) is preserved. Callers that render user-supplied content in HTML templates opt in by passing autoescape=True; plain-text templates are never escaped. Adds unit tests covering opt-in escaping, the default raw behavior, and the | safe passthrough filter.
Add a get_template_path hook on the base email builder and a confirmation template so requesters receive a confirmation email while admins keep the existing request template. The confirmation routing compares the recipient email to the recorded requester_email and excludes admins case-insensitively, fixing an asymmetry where the email match was case-insensitive but the admin check used the case-sensitive config.is_admin_user. Also include the tool name(s) in the email subject, cache the StoredWorkflow name lookup once per builder (avoiding a duplicate DB hit across TXT and HTML renders), and opt the tool-request HTML body into autoescaping so user-supplied fields are escaped.
Add a cancelDisabled prop to GModal that blocks all dismiss paths (Cancel button, close button, backdrop click, and the native dialog Escape/cancel event). The tool installation request form passes cancel-disabled=submitting so the modal cannot be closed while the request is in flight, ensuring the success or error feedback is not lost; the cancel button also relabels to Cancel while submitting.
Upstream dropped Python 3.8 support and removed Optional/Union from the typing imports in the notification schema, manager, and service modules in favor of PEP 604 X | None syntax. The tool installation request code added by this branch still used Optional[...]; after rebasing onto upstream/dev those names were no longer imported, raising NameError at import/collection time. Convert the branch's Optional[X] annotations to X | None to match upstream's style and restore imports.
Type the _UNSET workflow_name cache sentinel as Any (matching the existing sentinel pattern in tool_util_models/parameters.py) instead of an ad-hoc _Unset = type(_UNSET) alias, which mypy rejected as 'Variable not valid as a type'. This also removes the now-unneeded 'type: ignore[return-value]' comments mypy flagged as unused. Apply black formatting to the autoescape render signature, the AnyUserNotificationContent union, and the custom template tests.
Flip render() autoescape default to True for HTML templates (False for .txt, which raises on an explicit True instead of silently overriding), and adopt the secure default across email builders. Security fixes: - Activation HTML email now renders with autoescape=True, escaping the Host-derived hostname and config URLs; pre-escaped Markup values (name/user_email) pass through unchanged, and the admin custom_message is marked | safe. - NewSharedItem builder opts into autoescape so user-controlled item_name/owner_name are escaped in the HTML email. - Message builder opts into autoescape and marks the to_html()-sanitized content['message'] as | safe, so the user-controlled subject (e.g. a malicious workflow name from the completion hook) and name are escaped instead of enabling a cross-user stored-XSS. Cleanup: - Replace the _AUTOESCAPE_UNSET sentinel + bool | object (which collapsed to object and required a type: ignore) with bool | None = None, which is type-safe and needs no ignore. - Document the autoescape_html opt-out rationale at each builder. - Wrap over-long activation render calls to satisfy black (line-length 120). Tests updated to cover the new default, the opt-out path, and the .txt explicit-True ValueError.
UserNotificationPreferences.get() used dict subscript access (`self.preferences[category]`), so a user who saved preferences before a category was introduced (e.g. tool_installation_request) had no key for it. The resulting KeyError was swallowed by the manager's broad except, dropping the notification at association-creation time -- no inbox entry, no email, no push -- for exactly the admins who should receive tool requests. Fall back to NotificationCategorySettings() (the same default used by get_default_personal_notification_preferences) when the category is missing from the stored blob. No migration needed; the fallback handles legacy blobs lazily. Includes a unit test that builds preferences from a stale blob missing tool_installation_request and asserts get() returns defaults rather than raising.
Add a server-side-only boolean that marks a tool installation request notification as the requester's confirmation copy (vs. the admin-facing request). Defaults to False; the notification service stamps it when building the requester copy, and the email template builder reads it to select the confirmation template and subject. This replaces email-string matching as the discrimination signal (done in a follow-up commit), so the field needs no client input -- but it serializes into the API response, so regenerate the TypeScript schema via make update-client-api-schema.
The service already knows which recipient is the submitter when it builds the notification, but the email builder was re-inferring "is this the requester / is this an admin" from email-string matching at render time -- re-running model_construct and an admin-email rescan per email, and silently flipping the template on email case, whitespace, or None. Instead, build two distinct notifications server-side: an admin-facing request (recipients = all admins, is_confirmation=False) and a requester confirmation copy (recipients = [sender], is_confirmation=True). The sender gets a confirmation only when they are not already an admin (an admin submitter already receives the admin-facing request). The API returns the admin-facing response so it describes the request the admin will act on. The email template builder now reads content.is_confirmation (a real field) to pick the template and subject, deleting the _is_confirmation_to_requester email-matching path entirely. Integration tests assert the admin copy is_confirmation=False, the sender's copy is_confirmation=True, and an admin submitter receives only the admin copy.
templates.render built its Environment with no loader, so {% include %}
could not resolve partials. The tool-request admin and confirmation
templates (HTML and TXT) therefore duplicated ~90 lines of identical
tool-detail markup.
Give the Environment a ChoiceLoader over [FileSystemLoader(custom_dir),
PackageLoader("galaxy.config", "templates")] so templates can include
partials with the same custom-first, package-default precedence as the
main template. The main template still uses the doc-header body split;
only {% include %} lookups use the loader. Autoescape inherits the
parent environment, so partial output is escaped too.
Extract the shared tool-fields block into
_tool_installation_request_fields.{html,txt} (pure body, no doc header);
the admin template keeps its workflow + requester rows after the
include, the confirmation template its footer. Single-sources the
field set and deletes the duplication.
Tests cover: a template including a packaged partial renders it, a
custom partial overrides the packaged one, partial output is
autoescaped, and the bundled admin template renders end-to-end via
its partial.
The regenerated TypeScript schema emits is_confirmation as a required field (openapi-typescript renders fields with a non-null default as required, the same convention used for other defaulted bools like is_dynamic and purge_history). Once the api-client package is rebuilt (pnpm postinstall rebuilds dist on every install), vue-tsc failed on the two form components and the Notifications test util with TS2741: "Property 'is_confirmation' is missing". Add is_confirmation: false to the three payload objects, matching how the rest of the codebase sends defaulted-required bools. The value is server-stamped and ignored on input; it is sent only to satisfy the generated schema.
…test
The shared _tool_installation_request_fields.txt partial opened with a
Jinja comment whose trailing newline, combined with the parent
template's blank line before the {% include %}, produced a visible
double blank line between the intro and the first field in the
plain-text email. Use {#- ... -#} / {%- whitespace control so the
comment and its trailing newline are trimmed.
Also extend test_requester_email_overridden_server_side to send
is_confirmation: True and assert the admin-facing copy is stamped
is_confirmation=False with the server-stamped requester_email -- locking
in the security property that a client-supplied is_confirmation can
never reach admins and render the confirmation template/subject.
The config_schema.yml entry added for the tool installation request feature was not reflected in the generated _galaxy_config_schema_attributes.py stub. GalaxyAppConfiguration sets schema keys via setattr at runtime, so this caused no runtime failure (the live server and integration tests passed), but the stub is what mypy/IDEs read: without it, config.enable_tool_installation_request_form in the notification service is an unknown attribute and fails the project's mypy gate. Regenerate via `python lib/galaxy/config/config_manage.py build_config_types galaxy`; the diff is a single line adding the typed bool attribute next to its sibling enable_help_forum_tool_panel_integration.
…elds The function accepted the full ToolInstallationRequestNotificationContent, exposing server-stamped fields (category discriminator, requester_email, is_confirmation) that callers must never set. Every caller had to pass is_confirmation: false with an apologetic 'sent only to satisfy the generated schema' comment, and nothing type-stopped a caller from spoofing requester_email. Introduce ToolInstallationRequestInput = Omit<..., 'category' | 'requester_email' | 'is_confirmation'> and have submitToolInstallationRequest fill the discriminator + is_confirmation internally. Callers now pass only the fields they actually own.
_build_user_sender_requests built the admin and confirmation notifications with two near-identical NotificationCreateData.model_construct blocks differing only in content.is_confirmation and recipients -- six lines of envelope construction duplicated. The confirmation is just the admin request with is_confirmation flipped and the recipient set narrowed to the submitter, so derive it with model_copy on the admin request instead of reconstructing the envelope. Deletes the second model_construct pair and makes the invariant read explicitly.
Replace the flat tool_names/tool_url content with a RequestedTool model (name, tool_shed_id, tool_url, description, scientific_domain, requested_version - each its own field, at least one identifier required) and an outer content model carrying the request-level metadata. Name and tool shed id are no longer mixed in one array. Migrate the email templates, the email builder (validate the stored content so nested tool dicts become models, cache it per builder, and handle multi-tool subjects), the notification card (per-tool details nested under each tool), the request form, and the workflow missing-tools flow. Workflow tool ids are split into shed id, name, and version by a parser shared with tool-version.ts. Addresses review feedback on PR galaxyproject#22259: mixed request formats, a model for a single requested thing, and name vs tool shed id.
Split the content model: clients submit ToolInstallationRequestCreateContent, which does not have the requester_email and is_confirmation fields at all, so they cannot be set and no longer show up in the POST schema docs. The persisted ToolInstallationRequestNotificationContent extends it with the two stamped fields, and the service promotes the validated create content when stamping. Addresses review feedback on PR galaxyproject#22259: is_confirmation and requester_email should not be settable or public facing.
Introduce a NotificationRequestHandler protocol and a handler registry. Each user-submittable request category owns its feature gate, content stamping, recipient resolution, and confirmation copy in one handler; _build_user_sender_requests has no category-specific branching left. The user allow-list is derived from the registry keys, so the two cannot drift apart. Groundwork for reusing the same path for other request types, e.g. a request to join a group (see galaxyproject#23248).
The Celery serializer dumps a NotificationCreateRequest by its declared schema. With the create-only content union on NotificationCreateData, the server-stamped requester_email and is_confirmation were silently stripped on any Celery-enabled deployment: admins got requests without a requester email and the submitter received the admin-facing template. Add InternalNotificationCreateData, whose content is the full union, use it for the internal NotificationCreateRequest, and switch all internal senders to it. An integration test now renders and delivers the admin email through the real dispatch path and asserts the stamped Requested by line survives.
Enforce the agreement as a model validator on NotificationCreateData, so every validated construction is covered, including the admin passthrough path. The handler's stamp_content also raises instead of silently passing through foreign content, and the previously unreachable guard in build_confirmation is now an assert documenting the invariant. Addresses review feedback on PR galaxyproject#22259 about the unreachable branch.
Stamp source and variant and ignore client-supplied timing fields for user submissions. A client must not be able to escalate the variant to urgent (which bypasses notification channel opt-outs) or set a short expiration that hides the request from admins; publication is immediate and expiration falls back to the default retention period applied by the notification model.
Free-text fields end up in emails (including the subject header) and notification cards, so normalize them on validation: collapse C0/C1 control characters and Unicode line separators, strip zero-width, bidi override, and tag characters, turn whitespace-only values into None, and run the sanitizers before the new max_length bounds so normalized text is measured. Cap requests at 50 tools, require an http(s) tool_url (scheme checked case-insensitively), truncate the email subject label to stay under the RFC 5322 header limit, and indent multiline values in the plain-text email so user text cannot fake a field row like 'Requested by:' at column 0. The clients follow the same bounds: the form validates field lengths with a clear message, and the workflow missing-tools request caps at 50 tools, tells the submitter about the truncation, and no longer repeats the tool ids in the remarks that the structured entries already carry.
Show the tool shed id as its own row next to the name, skip the URL row when the URL is already used as the tool's label, use the neutral 'Tool:' label in the text email (matching the HTML email) through a shared tool_label macro with an '(unspecified)' fallback, and preserve the line structure of multiline descriptions and remarks in the HTML email and the card (white-space: pre-line). The card builds its per-tool detail rows from one toolDetailRows() source for both the single-tool and multi-tool branches. Addresses review feedback on PR galaxyproject#22259: name and tool shed id are different things and should be shown as such.
cafc7d8 to
10ae14e
Compare
Closes #21758
Adds a Tool Installation Request Form that allows logged-in users to request new tools to be installed on a Galaxy instance. Submitted requests are delivered to all admin users via Galaxy's notification system and by email (emails are sent by the periodic notification dispatcher, so delivery is asynchronous). The submitter also receives a confirmation copy of the request.
This PR also extends the feature to handle workflow runs with missing tools: when a workflow cannot be run because required Tool Shed tools are not installed, a one-click "Request Installation" button appears inline on the workflow run page so users can notify admins without filling out the full form.
(Some screenshots below show an earlier version of the form.)
Tool Installation Request Form (ToolBox panel)
enable_tool_installation_request_form: trueis set, shown when a tool search returns no results.Workflow Missing-Tools Install Request
POST /api/notificationsis called withcategory: "tool_installation_request". Each missing tool id is split into its tool shed repository id, tool name, and version, and sent as a structuredRequestedToolentry together with the workflow id.Admin Side
Admins receive an in-app notification and an email showing each requested tool (name, tool shed id, URL, description, domain, version), the workflow (with its resolved name in the email), any remarks, and the requester email. The submitter receives a confirmation copy with its own email template.
Data model
Requests use a per-item model: each requested tool is a
RequestedTool(name, tool_shed_id, tool_url, description, scientific_domain, requested_version; at least one identifier required), wrapped by a content model with the request-level metadata (workflow_id, additional_remarks). Clients submitToolInstallationRequestCreateContent; the persistedToolInstallationRequestNotificationContentadds the two server-stamped fields (requester_email,is_confirmation).The service dispatches through a per-category
NotificationRequestHandlerregistry, so adding another user-submittable request type (for example a request to join a group, #23248) is a new content model plus one handler, with no service changes.Security & Hardening
requester_emailandis_confirmationdo not exist in the request schema; the service stamps them. The notification envelope (source, variant, publication and expiration time) is also server-controlled, so a client cannot escalate tourgentor hide the request with a short expiration.Requested by:; the subject label is truncated to stay inside the RFC 5322 header limit;tool_urlmust be http(s) and is always rendered as plain text, never as a link.Note for branch followers
Tool-request notifications stored by older versions of this branch use the old content shape and will break the notification listing. Please delete those rows; nothing released is affected.
How to test the changes?
(Select all options that apply)
I've included appropriate automated tests.
This is a refactoring of components with existing test coverage.
Instructions for manual testing are as follows:
Tool Installation Request Form (ToolBox):
config/galaxy.yml:Workflow Missing-Tools Button:
License