[Port to dtq-dev] license management#1299
Open
kosarko wants to merge 3 commits into
Open
Conversation
* 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 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)
📝 WalkthroughWalkthroughExtends ChangesLicense Label Management with In-Use Guards
Sequence Diagram(s)sequenceDiagram
participant User
participant ClarinLicenseTableComponent
participant NgbModal
participant DefineLicenseLabelFormComponent
participant ClarinLicenseLabelDataService
participant fetchAllLicensePages
User->>ClarinLicenseTableComponent: click Edit Label
ClarinLicenseTableComponent->>NgbModal: open(DefineLicenseLabelFormComponent, {centered:true})
NgbModal-->>DefineLicenseLabelFormComponent: componentInstance.clarinLicenseLabel = label
User->>DefineLicenseLabelFormComponent: submit form
DefineLicenseLabelFormComponent-->>ClarinLicenseTableComponent: modalRef result (updated label data)
ClarinLicenseTableComponent->>ClarinLicenseLabelDataService: put(updatedLabel)
ClarinLicenseLabelDataService-->>ClarinLicenseTableComponent: RemoteData<ClarinLicenseLabel>
ClarinLicenseTableComponent->>fetchAllLicensePages: ensureLicenseUsageLoaded(forceReload=true)
fetchAllLicensePages-->>ClarinLicenseTableComponent: inUseLabelIds rebuilt
ClarinLicenseTableComponent->>ClarinLicenseTableComponent: refreshLabels()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kosarko
changed the title
2 tasks
There was a problem hiding this comment.
Pull request overview
Ports the “license management” UI features to the dtq-dev branch, extending the Clarin Licenses admin screen with license-label CRUD capabilities and safer delete behavior (including tooltips/confirmation), aligned with the backend changes required by dataquest-dev/DSpace#1325.
Changes:
- Added a License Labels section with pagination, edit/delete actions, and “in-use” delete disabling + tooltips.
- Extended the Clarin license-label data service to support PUT and DELETE operations.
- Updated modals/forms (create vs edit mode, icon preview/clear) and added/updated i18n strings + unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/assets/i18n/en.json5 | Adds new EN strings for license delete tooltip and the new license-label UI (table, modals, toasts). |
| src/assets/i18n/cs.json5 | Adds corresponding CS strings for the new license-label UI and delete tooltip. |
| src/app/core/shared/clarin/clarin-license-label-extended-serializer.ts | Makes “extended” serialization accept boolean values while retaining legacy string handling. |
| src/app/core/data/clarin/clarin-license-label-data.service.ts | Adds PUT/DELETE support for Clarin license labels by switching to IdentifiableDataService and delegating to PutDataImpl/DeleteDataImpl. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.ts | Adds edit mode support, icon preview helpers, and boolean-based extended options. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.spec.ts | Adds test coverage for edit-mode behavior and template rendering. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.scss | Removes modal display override styling. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.html | Refactors modal markup, adds icon preview/clear UI, and distinct create vs edit labels. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.scss | Removes modal display override styling. |
| src/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.html | Refactors modal markup layout (header/body/footer). |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts | Adds labels table streams, label CRUD flows, confirmation modal delete, and frontend-only label usage derivation. |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts | Adds/updates tests for label edit/delete flows, tooltips, and usage-crawl behavior. |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.scss | Adds column sizing for label action buttons. |
| src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.html | Adds labels section UI and improves license delete button behavior with tooltip/disabled handling. |
milanmajchrak
requested changes
Jun 26, 2026
- 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
|
@milanmajchrak I have pushed the changes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts (1)
491-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting tooltip text via
ng-reflect-*attributes.
ng-reflect-ngb-tooltipis a debug-only Angular implementation detail, so this check is brittle. Please read theNgbTooltipdirective instance here as well, like the license-delete test above, and assert onngbTooltipdirectly.Suggested refactor
const labelRows = fixture.debugElement.queryAll(By.css('.labels-section tbody tr')); const linkedRowDeleteWrapper = labelRows[0].query(By.css('td:last-child span')); const linkedRowButtons = labelRows[0].queryAll(By.css('button')); const linkedDeleteButton = linkedRowButtons[1]; + const linkedDeleteTooltip = linkedRowDeleteWrapper.injector.get(NgbTooltip); expect(linkedRowButtons.length).toBe(2); expect(linkedDeleteButton.attributes['aria-disabled']).toBe('true'); expect(linkedDeleteButton.nativeElement.classList.contains('disabled')).toBeTrue(); expect((linkedRowDeleteWrapper.nativeElement as HTMLElement).getAttribute('tabindex')).toBe('0'); - expect((linkedRowDeleteWrapper.nativeElement as HTMLElement).getAttribute('ng-reflect-ngb-tooltip')).toContain('clarin.license.label.table.del'); + expect(linkedDeleteTooltip.ngbTooltip as string).toContain('clarin.license.label.table.del');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts` around lines 491 - 503, The linked-label delete tooltip test is asserting against the debug-only ng-reflect-ngb-tooltip attribute, which is brittle. Update the spec in clarin-license-table.component.spec.ts to follow the existing delete test pattern by retrieving the NgbTooltip directive instance from the wrapper element and asserting its ngbTooltip value directly, while keeping the current checks on disabled state and tabindex.src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd TypeDoc for the modified exported component class.
ClarinLicenseTableComponentis modified but has no class-level TypeDoc/JSDoc. Add a short summary above the class. As per coding guidelines,**/*.ts: All new/modified public methods and classes require TypeDoc/JSDoc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts` at line 36, `ClarinLicenseTableComponent` is a modified exported class and needs class-level TypeDoc/JSDoc. Add a short summary comment immediately above the `ClarinLicenseTableComponent` declaration, following the project’s documentation convention for public classes.Source: Coding guidelines
src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.spec.ts (1)
63-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a spec for the icon preview/remove path.
The new edit-mode suite only uses
icon: [], sohasIcon,currentIconUrl, and theclearIconcheckbox can still regress without a failing test. As per coding guidelines, "New features must include new unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.spec.ts` around lines 63 - 127, Add an edit-mode unit test in DefineLicenseLabelFormComponent that covers the icon preview/remove path, since the current edit-mode setup only uses an empty icon array and does not exercise hasIcon, currentIconUrl, or the clearIcon control. Update the edit-mode spec around DefineLicenseLabelFormComponent and clarinLicenseLabelForm to initialize a label with an actual icon value, assert the preview state is populated, toggle the clearIcon checkbox, and verify the form/component state changes accordingly so regressions in icon handling are caught.Source: Coding guidelines
src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.html (1)
155-155: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate async subscription to
labelsRD$.
labelsRD$is already subscribed via*ngVarat Line 102 (providingcLicenseLabels). Line 155 adds a second independent async subscription to the same observable. Similarly,loading$is subscribed independently on Lines 162 and 163. Each| asyncpipe creates its own subscription; prefer usingcLicenseLabels(already in scope from the outer*ngVar) and the singleloading$binding rather than re-subscribing.♻️ Suggested fix
- <tr *ngIf="!(loading$ | async) && (labelsRD$ | async)?.hasSucceeded && !(cLicenseLabels?.page?.length > 0)"> + <tr *ngIf="!(loading$ | async) && labelsRD.hasSucceeded && !(cLicenseLabels?.page?.length > 0)">Or restructure the outer
*ngVarto also capture the RD itself:- <div *ngVar="(labelsRD$ | async)?.payload as cLicenseLabels"> + <div *ngVar="(labelsRD$ | async) as labelsRD"> + <ng-container *ngVar="labelsRD?.payload as cLicenseLabels">Then replace
(labelsRD$ | async)?.hasSucceededwithlabelsRD?.hasSucceededand avoid the duplicate pipe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.html` at line 155, The template has duplicate async subscriptions for labelsRD$ and loading$, which creates extra independent subscriptions and unnecessary work. Update clarin-license-table.component.html to reuse the existing cLicenseLabels value already introduced by the outer *ngVar instead of calling (labelsRD$ | async) again, and similarly consolidate the loading$ usage so the template binds once and reuses that value. If needed, adjust the *ngVar setup around cLicenseLabels so the underlying RD object is available for the conditional checks without adding another async pipe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts`:
- Around line 418-425: The delete-failure spec is mocking the service with a
thrown stream error instead of the same RemoteData contract used elsewhere in
this test file. Update the `ClarinLicenseTableComponent` delete test to have
`clarinLicenseLabelDataService.delete` emit a failed `RemoteData` value, and
keep the existing `confirmDeleteLabel`/`refreshLabels` assertion path so the
test verifies handling of `hasFailed` delete responses rather than `throwError`.
In
`@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts`:
- Around line 600-607: The ensureLicenseUsageLoaded method is ignoring
forceReload whenever licenseUsageLoading is already true, which lets an older
crawl finish and mark stale inUseLabelIds as ready. Update
ensureLicenseUsageLoaded in clarin-license-table.component.ts so a forced reload
is not dropped during an in-flight crawl, either by queuing a follow-up reload
or restarting the crawl after the current one completes. Make sure the reload
path that feeds the post-create/edit/delete guards always reflects the latest
license-label links, including the logic around the existing licenseUsageLoading
and labelUsageReady$ checks.
- Line 416: The edit flow is assigning `updatedLabel.extended` with a boolean
coercion, which turns string inputs like `'false'` into `true`. Update the PUT
path in `ClarinLicenseTableComponent` to serialize `formValues.extended` the
same way the create path does via
`ClarinLicenseLabelExtendedSerializer.Serialize(...)`, so the persisted label
type matches the user's edited value.
- Around line 672-682: The page traversal logic in fetchAllLicensePages is
mixing zero-based and one-based page indexes, causing the last page to be
skipped when recursing from response.payload.currentPage. Update the next-page
calculation in ClarinLicenseTableComponent so it consistently converts the API’s
currentPage into the index used by fetchAllLicensePages before incrementing, and
ensure the hasNextPage check compares against the correct page number so every
page is fetched.
- Around line 462-487: `confirmDeleteLabel` only checks `id` before opening the
delete flow, so an in-use `ClarinLicenseLabel` can still reach
`clarinLicenseLabelService.delete` if the handler is called without the current
template guard; add the same in-use guard inside this method before creating the
modal or subscribing, and reject missing ids with `hasNoValue` instead of only
`isNull`. Keep the check near the start of `confirmDeleteLabel` so the deletion
path exits early for undefined or in-use labels.
In
`@src/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.html`:
- Around line 11-46: Associate each label with an actual control in
define-license-form.component.html by giving the name, definition, confirmation,
clarinLicenseLabel, extendedClarinLicenseLabels, and requiredInfo controls
matching id attributes and wiring the label for values to those ids. For the
radio and checkbox groups in the *ngFor blocks, wrap each input and its text in
a label (or use a nested label) so the option text becomes clickable and screen
readers get the proper accessible name. Keep the existing
formControlName/formArrayName bindings and update the template structure only
where needed to preserve behavior.
In
`@src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.ts`:
- Around line 87-95: Add TypeDoc/JSDoc for the public ngOnInit method in
DefineLicenseLabelFormComponent. Document that it initializes the form via
createForm() and, when isEditMode is true, patches the form with the existing
clarinLicenseLabel values; keep the comment aligned with the method’s current
initialization responsibilities.
- Around line 89-94: Normalize the legacy extended value before calling
defineLicenseLabelFormComponent’s patchValue in the edit-mode branch so the
boolean-backed select always receives a real boolean instead of a persisted
string. Update the isEditMode flow in define-license-label-form.component.ts to
map clarinLicenseLabel.extended from legacy 'Yes'/'No' values into true/false
before patching clarinLicenseLabelForm, using the existing clarinLicenseLabel
and clarinLicenseLabelForm symbols to keep the save path consistent.
In `@src/app/core/shared/clarin/clarin-license-label-extended-serializer.ts`:
- Around line 5-12: Add TypeDoc/JSDoc to the exported
ClarinLicenseLabelExtendedSerializer.Serialize method to document its boolean
and legacy-string input contract. Update the Serialize(extended: any) method
comment so it clearly states that boolean values are returned as-is and that the
legacy string value 'Yes' maps to true while other values map to false.
---
Nitpick comments:
In
`@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.html`:
- Line 155: The template has duplicate async subscriptions for labelsRD$ and
loading$, which creates extra independent subscriptions and unnecessary work.
Update clarin-license-table.component.html to reuse the existing cLicenseLabels
value already introduced by the outer *ngVar instead of calling (labelsRD$ |
async) again, and similarly consolidate the loading$ usage so the template binds
once and reuses that value. If needed, adjust the *ngVar setup around
cLicenseLabels so the underlying RD object is available for the conditional
checks without adding another async pipe.
In
`@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.ts`:
- Around line 491-503: The linked-label delete tooltip test is asserting against
the debug-only ng-reflect-ngb-tooltip attribute, which is brittle. Update the
spec in clarin-license-table.component.spec.ts to follow the existing delete
test pattern by retrieving the NgbTooltip directive instance from the wrapper
element and asserting its ngbTooltip value directly, while keeping the current
checks on disabled state and tabindex.
In
`@src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.ts`:
- Line 36: `ClarinLicenseTableComponent` is a modified exported class and needs
class-level TypeDoc/JSDoc. Add a short summary comment immediately above the
`ClarinLicenseTableComponent` declaration, following the project’s documentation
convention for public classes.
In
`@src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.spec.ts`:
- Around line 63-127: Add an edit-mode unit test in
DefineLicenseLabelFormComponent that covers the icon preview/remove path, since
the current edit-mode setup only uses an empty icon array and does not exercise
hasIcon, currentIconUrl, or the clearIcon control. Update the edit-mode spec
around DefineLicenseLabelFormComponent and clarinLicenseLabelForm to initialize
a label with an actual icon value, assert the preview state is populated, toggle
the clearIcon checkbox, and verify the form/component state changes accordingly
so regressions in icon handling are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b720b1c3-8fc7-4928-9d8e-5f8768c79043
📒 Files selected for processing (14)
src/app/clarin-licenses/clarin-license-table/clarin-license-table.component.htmlsrc/app/clarin-licenses/clarin-license-table/clarin-license-table.component.scsssrc/app/clarin-licenses/clarin-license-table/clarin-license-table.component.spec.tssrc/app/clarin-licenses/clarin-license-table/clarin-license-table.component.tssrc/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.htmlsrc/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.scsssrc/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.htmlsrc/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.scsssrc/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.spec.tssrc/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.tssrc/app/core/data/clarin/clarin-license-label-data.service.tssrc/app/core/shared/clarin/clarin-license-label-extended-serializer.tssrc/assets/i18n/cs.json5src/assets/i18n/en.json5
💤 Files with no reviewable changes (2)
- src/app/clarin-licenses/clarin-license-table/modal/define-license-label-form/define-license-label-form.component.scss
- src/app/clarin-licenses/clarin-license-table/modal/define-license-form/define-license-form.component.scss
milanmajchrak
approved these changes
Jun 29, 2026
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.
Port of ufal#134 and ufal#137 by @amadulhaxxani to
dtq-dev.Note that this needs dataquest-dev/DSpace#1325 on the backend
Summary by CodeRabbit
New Features
Bug Fixes