Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/api/src/models/license/license.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export class License extends Model<License> {
readonly licenseCapabilities?: CapabilityList;
readonly version?: string;
readonly installationId?: string;
readonly valid?: boolean;
readonly valid: boolean;
readonly message?: string;
readonly present?: boolean;
readonly usageRestriction?: string;
Expand All @@ -42,7 +42,7 @@ export class License extends Model<License> {
this.licenseCapabilities = data?.licenseCapabilities;
this.version = data?.version || '';
this.installationId = data?.installationId || '';
this.valid = data?.valid;
this.valid = data?.valid ?? false;
this.typeOfUse = data?.typeOfUse || '';
this.message = data?.message || '';
this.present = data?.present || false;
Expand Down
4 changes: 4 additions & 0 deletions packages/api/src/models/repositories/repository-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ export class RepositoryList extends ModelList<Repository> {
super(repositories);
}

filterWithCallback(filterCallback: (repository: Repository) => boolean): RepositoryList {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called filterCallback? Isn't filter informative enough?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a collision with the super's names?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so .. what it does different than the already inherited filter method is that it returns a new List with the values. I think filterWithCallback should be renamed to something like filteredList.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. The argument can be renamed to filter I guess. My idea was somewhat I've seen in lodash for example where they have functions like unionWith, uniqueWith, etc. where those take a callback/comparator function but made it a bit more verbose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the lodash functions, but here you already have filter which does the same thing, only difference is that here you return a new reference for a list.

If you need both: to get a list and to get an array, perhaps this shoud be a .toFilteredList or something like this.

return new RepositoryList(super.filter(filterCallback));
}

/**
* Finds a repository in the list by its ID and location.
*
Expand Down
2 changes: 2 additions & 0 deletions packages/api/src/models/repositories/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export class Repository extends Model<Repository> implements RepositoryReference
externalUrl: string;
location: string;
state: RepositoryState | undefined;
stateLowercase: string | undefined;
local: boolean | undefined;
readable: boolean | undefined;
writable: boolean | undefined;
Expand All @@ -34,6 +35,7 @@ export class Repository extends Model<Repository> implements RepositoryReference
this.externalUrl = data.externalUrl || '';
this.location = data.location || '';
this.state = data.state;
this.stateLowercase = data.stateLowercase;
this.local = data.local;
this.readable = data.readable;
this.writable = data.writable;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const mapRepositoryListResponseToModel: MapperFn<RepositoryListResponse,
externalUrl: repositoryData.externalUrl,
location: repositoryData.location,
state: toEnum(RepositoryState, repositoryData.state),
stateLowercase: repositoryData.state ? repositoryData.state.toLowerCase() : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set this here? Isn't it more apropriate to set it in the constructor? It is a helper prop.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this should be in the constructor as it is a derived property. I used to change it, but I guess I've dropped it somewhere down the road

local: repositoryData.local,
readable: repositoryData.readable,
writable: repositoryData.writable,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import {AuthenticatedUser, Authority, Rights, SecurityConfig} from '../../../models/security';
import {service} from '../../../providers';
import {SecurityContextService} from './security-context.service';
import {Repository} from '../../../models/repositories';
import {Repository, RepositoryList} from '../../../models/repositories';
import {
RepositoryAuthorityService,
RepositoryContextService,
Expand Down Expand Up @@ -77,6 +77,9 @@
* @returns {boolean} True if the user has the repository manager role, false otherwise.
*/
isRepoManager(): boolean {
// TODO the legacy is doing the following

Check warning on line 80 in packages/api/src/services/domain/security/authorization.service.ts

View check run for this annotation

SonarQubeCloud / [Shared-components] SonarCloud Code Analysis

Complete the task associated to this "TODO" comment.

See more on https://sonarcloud.io/project/issues?id=Ontotext-AD_WB_migration_workbench&issues=AZ68oVGBKloCNoILMoFa&open=AZ68oVGBKloCNoILMoFa&pullRequest=2994
// return authorizationService.hasRole(UserRole.ROLE_REPO_MANAGER) && !$repositories.getDegradedReason();
// Check why we are not doing the same in the new implementation and if we need to add the degraded reason check back in
return this.hasRole(Authority.ROLE_REPO_MANAGER);
}

Expand Down Expand Up @@ -296,6 +299,46 @@
this.securityContextService.updateRestrictedPages(restrictedPages);
};

/**
* Retrieves a list of repositories that the user has write access to.
*
* @returns {RepositoryList} A list of repositories that the user can write to.
*/
getWritableRepositories(): RepositoryList {
return this.repositoryContextService.getRepositoryList().filterWithCallback(repository => this.canWriteRepo(repository));
}

/**
* Retrieves a list of repositories that the user has read access to, including those that are readable through
* GraphQL permissions.
*
* @returns {RepositoryList} A list of repositories that the user can read.
*/
getReadableRepositories(): RepositoryList {
return this.repositoryContextService.getRepositoryList().filterWithCallback(repository =>
this.canReadRepo(repository) || this.canReadGqlRepo(repository)
);
}

/**
* Retrieves a list of repositories that the user has access to, based on the specified parameters.
* @param includeRemote - If true, includes remote repositories in the list; otherwise, only local repositories are included.
* @param isRestricted - If true, returns repositories that the user has write access to; if false, returns repositories that the user has read access to.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of strange. If isRestricted is true I get unrestricted access repos (write/read) and if it is false, I am restricted only to read access.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this was a blind copy/paste from the core-errors. It's stupid indeed.

* @returns A list of repositories that the user has access to, filtered based on the provided parameters.
*/
getAccessibleRepositories(includeRemote = false, isRestricted = true): RepositoryList {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
let remoteLocationsFilter = (_repo: Repository) => true;
if (!includeRemote) {
remoteLocationsFilter = (repo) => !!repo.local;
}
if (isRestricted) {
return new RepositoryList(this.getWritableRepositories().filter(remoteLocationsFilter));
} else {
return new RepositoryList(this.getReadableRepositories().filter(remoteLocationsFilter));
}
}

private resolveAuthorities(authoritiesList?: string[]) {
// If no authorities list is provided, return empty array.
if (!authoritiesList) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div role="alert">
<div class="mb-1 mt-1 ml-1 mr-1">
<div class="card repository-errors">
<div class="alert lead info-message" ng-class="!getActiveRepository() ? 'alert-info' : 'alert-warning'">
<div class="alert info-message" ng-class="!getActiveRepository() ? 'alert-info' : 'alert-warning'">
<div ng-if="!getActiveRepository()" >{{'core.errors.no.connected.repository.warning.msg' | translate}}</div>
<div id="restrictedDiv" ng-if="getActiveRepository() && isRestricted">{{'core.errors.restricted.warning.msg' | translate}}
<span ng-if="!isLicenseValid()">{{'core.errors.not.valid.license.warning.msg' | translate}}
Expand Down Expand Up @@ -36,7 +36,7 @@
class="list-group-item list-group-item-action repository"
ng-class="{'remote': !repository.local}"
ng-mouseenter="showPopoverForRepo($event, repository)" ngx-mouseleave="setPopoverForRepo($event, repository, false)">
<div class="lead ellipsis-block" ng-click="setRepository(repository)">
<div class="ellipsis-block" ng-click="setRepository(repository)">
<span class="popover-anchor"
popover-popup-delay="500"
popover-trigger="show" popover-placement="{{$index % 2 === 0 ? 'bottom-right' : 'left-bottom'}}"
Expand Down
1 change: 1 addition & 0 deletions packages/root-config/scripts/validate-translations.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const packagesDir = path.join(baseDir, 'packages');
// Verified identical translations - no warnings or errors about them
const identicalTranslations = [
"#",
"Select",
"Active",
"Action",
"Actions",
Expand Down
1 change: 0 additions & 1 deletion packages/workbench/extra-webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,5 @@ module.exports = (config, options) => {
// singleSpaWebpackConfig.devServer.client.overlay = false;
// singleSpaWebpackConfig.devServer.liveReload = false;

// console.log('=============', JSON.stringify(singleSpaWebpackConfig, null, 2));
return singleSpaWebpackConfig;
};
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ <h1 class="title-container">
}
</h1>

<div class="page-banners">
@if (!selectedRepository()) {
<app-repository-required-banner></app-repository-required-banner>
}
</div>
<app-page-restrictions [title]="title()"></app-page-restrictions>
}

<ng-content></ng-content>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
gap: 0.5rem;
}

.page-banners {
app-page-restrictions {
display: block;
Comment thread
yordanalexandrov marked this conversation as resolved.

/* Compensate for the potential negative margin as the one coming from the sparql-editor page */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {CommonModule} from '@angular/common';
import {ActivatedRoute} from '@angular/router';
import {provideTranslocoForTesting} from '../../../testing-utils/transloco-utils';
import {provideNoopAnimations} from '@angular/platform-browser/animations';
import {Repository, RepositoryContextService, RepositoryList, ServiceProvider} from '@ontotext/workbench-api';
import {RepositoryContextService, RepositoryList, ServiceProvider} from '@ontotext/workbench-api';

function buildActivatedRouteMock(queryParams: Record<string, string> = {}, data: Record<string, unknown> = {}) {
return {
Expand Down Expand Up @@ -136,67 +136,4 @@ describe('PageLayoutComponent', () => {
it('should leave documentationLink undefined when not in route data', () => {
expect(component.documentationLink()).toBeUndefined();
});

describe('repository-required-banner visibility', () => {
it('should show the banner when no repository is selected', () => {
const banner = fixture.nativeElement.querySelector('app-repository-required-banner');
expect(banner).not.toBeNull();
});

it('should hide the banner when a repository is already selected on init', async () => {
TestBed.resetTestingModule();
await TestBed.configureTestingModule({
imports: [PageLayoutComponent, CommonModule, provideTranslocoForTesting()],
providers: [
{provide: ActivatedRoute, useValue: buildActivatedRouteMock()},
provideNoopAnimations()
]
}).compileComponents();

const repoContextService = ServiceProvider.get(RepositoryContextService);
const repository = new Repository({id: 'test', location: '', uri: 'http://test'});
repoContextService.updateRepositoryList(new RepositoryList([repository]));
await repoContextService.updateSelectedRepository(repository);

const f = TestBed.createComponent(PageLayoutComponent);
f.detectChanges();

const banner = f.nativeElement.querySelector('app-repository-required-banner');
expect(banner).toBeNull();
});

it('should hide the banner reactively when a repository is selected after init', async () => {
const repository = new Repository({id: 'test', location: '', uri: 'http://test'});
repositoryContextService.updateRepositoryList(new RepositoryList([repository]));

let banner = fixture.nativeElement.querySelector('app-repository-required-banner');
expect(banner).not.toBeNull();

await repositoryContextService.updateSelectedRepository(repository);
fixture.detectChanges();

banner = fixture.nativeElement.querySelector('app-repository-required-banner');
expect(banner).toBeNull();
});

it('should show the banner again when the selected repository is cleared', async () => {
const repository = new Repository({id: 'test', location: '', uri: 'http://test'});
repositoryContextService.updateRepositoryList(new RepositoryList([repository]));
await repositoryContextService.updateSelectedRepository(repository);
fixture.detectChanges();

expect(fixture.nativeElement.querySelector('app-repository-required-banner')).toBeNull();

await repositoryContextService.updateSelectedRepository(undefined);
fixture.detectChanges();

expect(fixture.nativeElement.querySelector('app-repository-required-banner')).not.toBeNull();
});

it('should unsubscribe from repository changes on destroy', () => {
const unsubscribeSpy = jest.spyOn(component['subscriptions'], 'unsubscribeAll');
component.ngOnDestroy();
expect(unsubscribeSpy).toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
import {Component, inject, OnDestroy, OnInit, signal} from '@angular/core';
import {Component, inject, OnInit, signal} from '@angular/core';
import {CommonModule} from '@angular/common';
import {ActivatedRoute} from '@angular/router';
import {ApplicationQueryParams} from '../../models/application-query-params';
import {TranslocoPipe} from '@jsverse/transloco';
import {PageInfoTooltipComponent} from '../page-info-tooltip/page-info-tooltip.component';
import {RepositoryRequiredBannerComponent} from '../repository-required-banner/repository-required-banner.component';
import {
service,
RepositoryContextService,
Repository,
SubscriptionList,
} from '@ontotext/workbench-api';
import {PageRestrictionsComponent} from '../page-restrictions/page-restrictions.component';

@Component({
selector: 'app-page-layout',
Expand All @@ -19,43 +13,27 @@ import {
CommonModule,
TranslocoPipe,
PageInfoTooltipComponent,
RepositoryRequiredBannerComponent
PageRestrictionsComponent
],
templateUrl: './page-layout.component.html',
styleUrl: './page-layout.component.scss'
})
export class PageLayoutComponent implements OnInit, OnDestroy {
private readonly repositoryContextService = service(RepositoryContextService);

export class PageLayoutComponent implements OnInit {
private readonly activatedRoute = inject(ActivatedRoute);

embedded = signal(false);
title = signal<string | undefined>(undefined);
helpInfo = signal<string | undefined>(undefined);
documentationLink = signal<string | undefined>(undefined);
selectedRepository = signal<Repository | undefined>(undefined);

private readonly subscriptions = new SubscriptionList();

ngOnInit(): void {
this.selectedRepository.set(this.repositoryContextService.getSelectedRepository());
this.subscriptions.add(
this.repositoryContextService.onSelectedRepositoryChanged((repo) => {
this.selectedRepository.set(repo);
})
);

this.getPageData();
const queryParams = this.activatedRoute.snapshot.queryParams;
if (queryParams.hasOwnProperty(ApplicationQueryParams.EMBEDDED)) {
this.embedded.set(true);
}
}

ngOnDestroy(): void {
this.subscriptions.unsubscribeAll();
}

private getPageData(): void {
const routeData = this.activatedRoute.snapshot.data;
this.title.set(routeData['title']);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<div class="page-restrictions">
@if (restrictions().length) {
<p-message [severity]="'info'" icon="ri-alert-line">
@for (reason of restrictions(); track reason.translationKey) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not big deal but strange

shouldn't the restrictions() give back a restriction which might/might not have a reason?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw it is actually RestrictionReason so it makes sense for the var to be reason if restrictions is actually restrictionReasons

<div>
{{ reason.translationKey | transloco: reason.translationParams }}
@if (reason.ctaKey) {
<a [routerLink]="reason.ctaLink">{{ reason.ctaKey | transloco }}</a>
}
</div>
}
</p-message>
}
@if (repositoryList() && !selectedRepository()) {
<app-repository-picker-list [isRestricted]="isRestricted()"></app-repository-picker-list>
}
</div>

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.page-restrictions {
margin-top: 1em;

&:empty {
margin-top: 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { provideRouter } from '@angular/router';

import { PageRestrictionsComponent } from './page-restrictions.component';
import {provideTranslocoForTesting} from '../../../testing-utils/transloco-utils';

describe('PageRestrictionsComponent', () => {
let component: PageRestrictionsComponent;
let fixture: ComponentFixture<PageRestrictionsComponent>;

beforeEach(async () => {
await TestBed.configureTestingModule({
imports: [PageRestrictionsComponent, provideTranslocoForTesting()],
providers: [provideRouter([])],
})
.compileComponents();

fixture = TestBed.createComponent(PageRestrictionsComponent);
component = fixture.componentInstance;
fixture.detectChanges();
});

it('should create', () => {
expect(component).toBeTruthy();
});
});
Loading