Disable delete for in-use licenses, add tooltip#137
Merged
kosarko merged 5 commits intoJun 5, 2026
Merged
Conversation
Prevent deleting licenses that are attached to bitstreams by disabling the Delete button and showing a tooltip explaining why. Adds UI markup (ngbTooltip wrapper, aria-label, dsBtnDisabled and guarded click), a new isSelectedLicenseInUse() helper and a check in deleteLicense(). Adds unit tests covering disabled/enabled states and click behavior, and provides i18n strings for English and Czech for the disabled-tooltip.
There was a problem hiding this comment.
Pull request overview
Prevents administrators from deleting CLARIN licenses that are currently referenced by bitstreams by disabling the delete action and explaining the reason via a tooltip, while also adding unit coverage and i18n strings.
Changes:
- Disable the “Delete License” button when the selected license has
bitstreams > 0, and show a tooltip explaining why. - Add
isSelectedLicenseInUse()helper and a guard indeleteLicense()to block deletion for in-use licenses. - Add unit tests for disabled/enabled states and click behavior; add EN/CZ translations for the tooltip.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/assets/i18n/en.json5 | Adds English tooltip text for disabled delete button |
| src/assets/i18n/cs.json5 | Adds Czech tooltip text for disabled delete button |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts | Adds “in use” helper + deletion guard |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.html | Disables delete via dsBtnDisabled and adds tooltip wrapper |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts | Adds tests for delete button state and click behavior |
Replace isNull with hasNoValue when validating selectedLicense.id in deleteLicense to correctly handle undefined/null values. Also add hasNoValue to the imports from shared/empty.util.
Update clarin-license-table component spec to import NgbTooltip and retrieve the tooltip instance from the delete wrapper's injector. Replace the previous assertion that inspected the DOM's ng-reflect-ngb-tooltip attribute with an assertion on deleteTooltip.ngbTooltip to verify the expected translation key. This makes the test more robust by avoiding reliance on Angular's rendered reflection attribute.
…ministration-when-a-bitstream-is-connected
Member
|
It works ✅ Verified manually:
(demo video + screenshot attached below) pr137-demo.webm
|
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch downstream dtq-dev
git worktree add -d .worktree/backport-137-to-dtq-dev downstream/dtq-dev
cd .worktree/backport-137-to-dtq-dev
git switch --create backport-137-to-dtq-dev
git cherry-pick -x 42c4fccba0bb87d9f2f05dd245745b194d599dff |
kosarko
pushed a commit
that referenced
this pull request
Jun 5, 2026
* Disable delete for in-use licenses, add tooltip Prevent deleting licenses that are attached to bitstreams by disabling the Delete button and showing a tooltip explaining why. Adds UI markup (ngbTooltip wrapper, aria-label, dsBtnDisabled and guarded click), a new isSelectedLicenseInUse() helper and a check in deleteLicense(). Adds unit tests covering disabled/enabled states and click behavior, and provides i18n strings for English and Czech for the disabled-tooltip. * Use hasNoValue for license id check Replace isNull with hasNoValue when validating selectedLicense.id in deleteLicense to correctly handle undefined/null values. Also add hasNoValue to the imports from shared/empty.util. * Use NgbTooltip instance in delete button spec Update clarin-license-table component spec to import NgbTooltip and retrieve the tooltip instance from the delete wrapper's injector. Replace the previous assertion that inspected the DOM's ng-reflect-ngb-tooltip attribute with an assertion on deleteTooltip.ngbTooltip to verify the expected translation key. This makes the test more robust by avoiding reliance on Angular's rendered reflection attribute. (cherry picked from commit 42c4fcc)
Member
now part of dataquest-dev#1299 |
milanmajchrak
pushed a commit
to dataquest-dev/dspace-angular
that referenced
this pull request
Jun 29, 2026
* 84 license labels management (ufal#134) * Add PUT and DELETE to ClarinLicenseLabel service Replace BaseDataService with IdentifiableDataService and add PutData/DeleteData support for ClarinLicenseLabel. Wire PutDataImpl and DeleteDataImpl in the constructor, expose put(), delete() and deleteByHref() methods, and update imports (remove BaseDataService, add PutData, DeleteData, IdentifiableDataService and NoContent). Enables updating and deleting Clarin License Labels via the data service. * Add license labels table and placeholders Introduce a new License Labels section in the Clarin license table UI with a table, empty-state row, aria labels, and a ds-loading indicator (src/...component.html). Add CSS rule to size the actions column (src/...component.scss). In the component (src/...component.ts) add placeholder Observables (labels$, loading$) and stub methods editLabel and confirmDeleteLabel that log actions; data loading and wiring will be implemented in a follow-up task. * Add label selection and refresh logic Add selectable labels UI and wiring for label management: - Template: add a radio column with aria labels, mark selected row, disable Edit/Delete unless a label is selected, and adjust colspan for empty state. Edit/Delete buttons now call no-arg handlers. - Component: introduce selectedLabel, labels$ and loading$ BehaviorSubjects, refreshLabels() to fetch all labels via clarinLicenseLabelService.findAll (with loading/error handling), selectLabel() and isSelected() helpers, and call refreshLabels() on init and after label creation. Add ngUnsubscribe and ngOnDestroy to clean up subscriptions (takeUntil). Update scan initial state to use a defaultListState and import SortOptions. - Tests: update spec to provide mockLicenseLabelListRD$ and spy findAll. These changes enable selecting a license label for future edit/delete actions and keep the label list in sync with the backend. * Add license labels management and i18n Replace the static license-labels table with a paginated, RemoteData-driven table and translated headers; add loading state and accessible SR-only labels. Wire up edit and delete flows: open DefineLicenseLabelFormComponent for edits (convert file input, call PUT, notify and refresh) and ConfirmationModalComponent for deletes (call DELETE, notify and refresh). Introduce labelsRD$ stream, pagination options, labelsRefresh$ trigger and initializeLabelsPaginationStream to reactively fetch pages. Enhance DefineLicenseLabelFormComponent to support edit mode (prefill form, boolean extended options, aria/id fixes) and update serializer to accept booleans and legacy string values. Update and extend unit tests to cover edit/delete flows and template changes. Add new English and Czech i18n keys for the labels UI and actions. * Use dedicated i18n key for label load errors Introduce a specific i18n key for license label load failures and use it in the labels pagination stream. Added a labelsLoadErrorKey constant in ClarinLicenseTableComponent and replaced usages of the create-error key when handling load failures. Also added the new "clarin.license.label.load.error" translations to en.json5 and cs.json5. * Refresh licenses after label update Call loadAllLicenses() when a label PUT succeeds so dependent license lists are refreshed. The component now only calls refreshLabels() and loadAllLicenses() if the RemoteData indicates success. Updated unit test to spy on loadAllLicenses and assert it is called on modal submit; adjusted method comment accordingly. * Reload licenses after label deletion When a license label is successfully deleted, also reload the full license list to ensure UI reflects the change. Call loadAllLicenses() alongside refreshLabels() in the component, and update the spec to assert loadAllLicenses() is invoked (adding a spy and expectation). * Hide empty table row while loading Prevent the "no licenses" message from appearing while data is being fetched by adding a loading$ | async check to the empty-row *ngIf in clarin-license-table.component.html. The empty message now only shows when not loading and cLicenseLabels.page is empty, avoiding a flash of the empty state during load. * Disable delete for labels in use with tooltip Prevent deleting license labels that are referenced by any license: template changes wrap the delete button in a span that conditionally shows an ngbTooltip and uses dsBtnDisabled with a conditional click handler. Component logic now loads all licenses (paged) on init, builds a Set of in-use label IDs (inUseLabelIds) and exposes isLabelInUse to drive the UI; helper methods fetchAllLicensePages, rebuildLabelUsageSet and loadAllLicensesForUsage were added along with an allLicensesRD$ stream. Unit tests were extended to cover linked vs unlinked label behavior (disabled state, tooltip, and preventing the confirmation modal), and i18n entries for the disabled-tooltip were added for English and Czech. * Show labels empty row only on successful load Update clarin-license-table template to only render the labels empty-state row when the labels request has succeeded ((labelsRD$ | async)?.hasSucceeded). This prevents the empty message from appearing during failed or pending label requests. Add a unit test that simulates a failed labels request to verify the empty-state row is not shown, and import the needed createFailedRemoteDataObject helper for the test. * Optimize license usage loading and tests Add caching and locking around the expensive full license-usage crawl to avoid redundant requests. Introduce licenseUsageLoaded and licenseUsageLoading flags, a new ensureLicenseUsageLoaded(forceReload) helper, and a forceUsageReload parameter to loadAllLicenses so callers can explicitly invalidate the cache. Update create/edit/delete flows to force a usage reload, and set/clear the loading flags in the usage fetch success/error handlers. Add unit tests to verify that the full usage dataset is loaded only once across repeated reloads and that forcing a reload triggers a new fetch. * F1: disable label Delete until usage crawl completes The in-use set (inUseLabelIds) is empty until the async license usage crawl finishes, so during that window an in-use label's Delete button was briefly enabled. Add a usageReady$ stream that only emits true once the crawl succeeds and gate every row's Delete button on it: all Delete buttons stay disabled (with a "checking usage" tooltip) until usage resolves, after which in-use labels stay disabled and unused ones enable. On failure the buttons stay disabled so there is no unsafe window. Frontend-only mitigation; the usage crawl itself is unchanged. * F2: de-wrap double-nested modal in both license forms Both define-license-form and define-license-label-form nested their own .modal > .modal-dialog > .modal-content inside ng-bootstrap's NgbModalWindow, which already provides those wrappers. The duplicate chrome collapsed ngb's .modal-content and pinned the inner position:fixed .modal to the top of the viewport instead of centering. Remove the redundant inner wrappers so ngb owns the modal chrome, delete the now-dead `.modal { display: ... }` scss workarounds in both forms, fix the `modal-boy` -> `modal-body` typo in define-license-form, and open all four form modals with { centered: true } so the dialog is centered. * F3: surface newly created label without manual paging The labels endpoint lists labels in ascending insertion order and ignores the sort param, so a freshly created label received the highest id and landed on the last pagination page, invisible to the admin after Save. After a successful create, jump the labels table to the last page (derived from the current total plus the one just added) so the new row is shown immediately. * F4: allow clearing a label icon on edit and preview the current icon The label edit form could keep or replace an icon but never remove one, and gave no indication of the current icon. Add a "Remove current icon" checkbox and a small preview (reusing secureImageData) shown in edit mode when the label has an icon. When clear is requested and no new file is chosen, the table sends an empty icon array through the existing PUT path to remove it; selecting a new file still takes precedence. * F5: center the delete-confirmation modal and complete cs.json5 label parity Pass { centered: true } when opening ConfirmationModalComponent from confirmDeleteLabel() so the delete-confirmation dialog is vertically centered like the four license/label form dialogs, removing the top-aligned visual inconsistency. Also add the three Czech translations that were missing from cs.json5 (table.delete.checking-tooltip, edit.icon.preview, edit.icon.clear) so the en/cs label key sets are back in parity. * test: expect centered option in delete-confirmation modal spec The F5 change passes { centered: true } to modalService.open; update the assertion to match so the label delete flow spec passes. --------- Co-authored-by: kosarko <ko_ok@centrum.cz> (cherry picked from commit 87cb7a7) * Disable delete for in-use licenses, add tooltip (ufal#137) * Disable delete for in-use licenses, add tooltip Prevent deleting licenses that are attached to bitstreams by disabling the Delete button and showing a tooltip explaining why. Adds UI markup (ngbTooltip wrapper, aria-label, dsBtnDisabled and guarded click), a new isSelectedLicenseInUse() helper and a check in deleteLicense(). Adds unit tests covering disabled/enabled states and click behavior, and provides i18n strings for English and Czech for the disabled-tooltip. * Use hasNoValue for license id check Replace isNull with hasNoValue when validating selectedLicense.id in deleteLicense to correctly handle undefined/null values. Also add hasNoValue to the imports from shared/empty.util. * Use NgbTooltip instance in delete button spec Update clarin-license-table component spec to import NgbTooltip and retrieve the tooltip instance from the delete wrapper's injector. Replace the previous assertion that inspected the DOM's ng-reflect-ngb-tooltip attribute with an assertion on deleteTooltip.ngbTooltip to verify the expected translation key. This makes the test more robust by avoiding reliance on Angular's rendered reflection attribute. (cherry picked from commit 42c4fcc) * Fix license label issues flagged in code review - Change label icon fallback from selectedLabel.icon to undefined to avoid sending a base64 string when the icon is unchanged - Correct 0‑based page parameter to 1‑based in fetchAllLicensePages - Replace hard‑coded aria-label on modal close button with a translatable key for accessibility --------- Co-authored-by: Amad Ul Hassan <hassan@ufal.mff.cuni.cz>
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
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.
Prevent deleting licenses that are attached to bitstreams
Disable the Delete button for licenses with attached bitstreams and display a tooltip explaining why deletion is prevented. Adds UI markup (ngbTooltip wrapper, aria-label, dsBtnDisabled and guarded click), a new isSelectedLicenseInUse() helper and a check in deleteLicense(). Adds unit tests covering disabled/enabled states and click behavior, and provides i18n strings for English and Czech for the disabled-tooltip.
Problem description
Administrators need protection against accidentally deleting licenses that are currently attached to one or more bitstreams. Without this safeguard, deleting an in-use license could leave orphaned bitstream-to-license mappings and create data inconsistency.
Analysis
The frontend license model already includes a
bitstreams: numberproperty (from the backend REST serialization). This field reflects the count of non-deleted mapped bitstreams and is available on every license in the paginated table. No backend change and no extra frontend API call is required.The existing license-label delete flow in the same component already demonstrates the correct accessibility pattern: a wrapper
spanwithngbTooltip, conditionaltabindex="0", and a button using[dsBtnDisabled]. This pattern is reused verbatim for the license delete button.The current license delete button is a bottom action (not per-row), so the disable rule is applied to
selectedLicense?.bitstreams > 0. The button remains disabled when no license is selected.Changes made
clarin-license-table.component.html — Replaced native
[disabled]with the label-delete wrapper + tooltip +dsBtnDisabledpattern. AddedngbTooltipbound to the disabled-tooltip i18n key (only shown whenbitstreams > 0). Added wrappertabindex="0"for accessibility when disabled. Added icon (fas fa-trash) and aria-label to the button. Guarded click handler withselectedLicense != null && !isSelectedLicenseInUse().clarin-license-table.component.ts — Added
isSelectedLicenseInUse(): booleanhelper method returningthis.selectedLicense?.bitstreams > 0. Added optional guard at the start ofdeleteLicense()to return ifthis.isSelectedLicenseInUse()is true.en.json5 — Added i18n key:
"clarin-license.button.delete-license.disabled-tooltip": "License \"{{name}}\" cannot be deleted because it is attached to one or more bitstreams."cs.json5 — Added i18n key:
"clarin-license.button.delete-license.disabled-tooltip": "Licenci \"{{name}}\" nelze smazat, protože jsou k ní připojeny jeden nebo více bitstreamů."clarin-license-table.component.spec.ts — Added test group "license delete button" covering:
bitstreams > 0bitstreams === 0Problems
None encountered. All changes compile, lint, and pass unit tests (5369 tests completed, 0 failed).
Sync verification
Copilot review