Conversation
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
Deployment URLs ready for review. |
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
|
@PatStLouis this has become stale - do you mind refreshing it so we can review/merge please? |
There was a problem hiding this comment.
Pull request overview
Updates the tenant UI’s credential revocation flow (and related display formatting) to better track exchanges and refresh state, while also aligning AnonCreds schema storage behavior and bumping the underlying ACA-Py agent version.
Changes:
- Prefer
cred_ex_idfor revocation requests and includecredential_exchange_idin issued-credential table rows. - Improve connection display name resolution by falling back to
their_labelwhenaliasis missing. - Update AnonCreds schema creation/storage flow and upgrade
acapy-agent+ Docker base image from1.5.0rc0to1.5.0.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| services/tenant-ui/frontend/src/store/issuerStore.ts | Simplifies revocation endpoint selection and refreshes credential list after revocation. |
| services/tenant-ui/frontend/src/store/governanceStore.ts | Removes manual schema-storage fallback and relies on backend event handling + delayed refresh. |
| services/tenant-ui/frontend/src/store/connectionStore.ts | Updates connection name lookup to fall back to their_label. |
| services/tenant-ui/frontend/src/helpers/tableFormatters.ts | Adds credential_exchange_id and improves connection name formatting for issued credentials. |
| services/tenant-ui/frontend/src/components/issuance/credentials/RevokeCredentialForm.vue | Builds revocation payload using cred_ex_id when available; removes format-detection logic. |
| services/tenant-ui/frontend/src/components/connections/Connections.vue | Uses `alias |
| plugins/traction_innkeeper/traction_innkeeper/v1_0/schema_storage/schema_storage_service.py | Makes AnonCreds schema-finished event subscription unconditional and normalizes handling. |
| plugins/traction_innkeeper/pyproject.toml | Bumps acapy-agent dependency to 1.5.0. |
| plugins/traction_innkeeper/poetry.lock | Locks updated acapy-agent version and transitive dependency ranges. |
| plugins/docker/Dockerfile | Updates ACA-Py agent container tag to py3.13-1.5.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| error.value = err; | ||
| throw err; | ||
| } | ||
|
|
||
| console.log('< issuerStore.revokeCredential'); | ||
|
|
||
| if (error.value != null) { | ||
| // throw error so $onAction.onError listeners can add their own handler | ||
| const errToThrow = error.value; | ||
| throw errToThrow; | ||
| } | ||
| // return data so $onAction.after listeners can add their own handler | ||
| return result; |
There was a problem hiding this comment.
The if (error.value != null) block is redundant here: the only assignment to error.value inside this action is in the inner catch, and that branch immediately re-throws, so execution won’t reach this check with error.value set. Consider simplifying the control flow by removing the extra error.value check (and potentially the nested try/catch) to reduce dead/duplicated error-handling paths.
| // Wait a moment for the backend event handler to process and store the schema | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| // Refresh the schema list to show the newly created schema | ||
| await listStoredSchemas(); |
There was a problem hiding this comment.
createAnoncredsSchema now relies on a fixed 500ms delay followed by a single listStoredSchemas() refresh. Since schema storage is handled asynchronously by a backend event handler, this can still race (event processing >500ms) and the newly created schema may not appear until a later manual refresh. Consider polling listStoredSchemas() until the new schema is present (with a timeout), or having the backend return/emit a definitive completion signal to avoid timing-based UI flakiness.
| // Wait a moment for the backend event handler to process and store the schema | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| // Refresh the schema list to show the newly created schema | |
| await listStoredSchemas(); | |
| // Poll for a short period to give the backend event handler time | |
| // to process and store the schema before refreshing the list. | |
| const maxAttempts = 10; | |
| const delayMs = 500; | |
| for (let attempt = 0; attempt < maxAttempts; attempt++) { | |
| // Refresh the schema list to show the newly created schema | |
| await listStoredSchemas(); | |
| // If more attempts remain, wait before the next poll | |
| if (attempt < maxAttempts - 1) { | |
| await new Promise((resolve) => setTimeout(resolve, delayMs)); | |
| } | |
| } |
| python = "^3.13" | ||
| acapy-agent = { version = "1.5.0rc0" } | ||
| acapy-agent = { version = "1.5.0" } |
There was a problem hiding this comment.
The PR title is focused on a UI revocation badge fix, but this change also upgrades acapy-agent from an RC to 1.5.0 (plus corresponding Docker image/lockfile changes) and adjusts schema event subscription behavior. If these are required for the badge fix, it would help to mention that in the PR description; otherwise consider splitting the dependency/runtime upgrade into a separate PR to keep scope and rollback surface smaller.
| def subscribe(bus: EventBus): | ||
| # Subscribe to Indy schema events | ||
| bus.subscribe(INDY_SCHEMA_EVENT_PATTERN, schemas_event_handler) | ||
| # Subscribe to AnonCreds schema events if available | ||
| # Explicitly compile as literal pattern to ensure it's a Pattern object, not a string | ||
| if ANONCREDS_SCHEMA_FINISHED_EVENT: | ||
| bus.subscribe( | ||
| re.compile(re.escape(ANONCREDS_SCHEMA_FINISHED_EVENT)), | ||
| schemas_event_handler, | ||
| ) | ||
| # Subscribe to AnonCreds schema finished events | ||
| # Use exact match pattern - escape special chars and anchor to start/end | ||
| bus.subscribe( | ||
| re.compile(f"^{re.escape(ANONCREDS_SCHEMA_FINISHED_EVENT)}$"), | ||
| schemas_event_handler, | ||
| ) |
There was a problem hiding this comment.
subscribe() now unconditionally adds a second subscription for ANONCREDS_SCHEMA_FINISHED_EVENT with a newly constructed anchored regex. There are existing unit tests for subscribe(), but they only assert the Indy subscription and would not catch regressions in the AnonCreds subscription (e.g., wrong pattern/handler). Consider extending test_subscribe to assert that the event bus is also subscribed for the AnonCreds schema-finished topic with the expected handler.
| async function revokeCredential(payload?: RevokeRequest) { | ||
| error.value = null; | ||
| loading.value = true; | ||
|
|
There was a problem hiding this comment.
revokeCredential accepts an optional payload, but revocation requests are not meaningful without at least one identifier (e.g., cred_ex_id or rev_reg_id + cred_rev_id). Making this parameter optional makes it easy to accidentally call this action with no data (which would post {} due to the API wrapper’s default), leading to a confusing backend 4xx. Consider making payload required and/or validating required fields up-front and throwing a clear error before calling the API.
| // Validate payload before making the API call to avoid sending an empty or meaningless request. | |
| if (!payload) { | |
| loading.value = false; | |
| throw new Error( | |
| 'Revoke request payload is required and must include either "cred_ex_id" or both "rev_reg_id" and "cred_rev_id".' | |
| ); | |
| } | |
| const hasCredExId = | |
| typeof (payload as any).cred_ex_id === 'string' && | |
| (payload as any).cred_ex_id.trim().length > 0; | |
| const hasRevRegAndCredRevId = | |
| typeof (payload as any).rev_reg_id === 'string' && | |
| (payload as any).rev_reg_id.trim().length > 0 && | |
| typeof (payload as any).cred_rev_id === 'string' && | |
| (payload as any).cred_rev_id.trim().length > 0; | |
| if (!hasCredExId && !hasRevRegAndCredRevId) { | |
| loading.value = false; | |
| throw new Error( | |
| 'Invalid revoke request: provide either "cred_ex_id" or both "rev_reg_id" and "cred_rev_id".' | |
| ); | |
| } |
No description provided.