Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixed and improved some user interface issues. #1666

Merged
merged 12 commits into from
Nov 28, 2024
1 change: 1 addition & 0 deletions src/app/app-config.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
datasetReduceEnabled: boolean;
datasetDetailsShowMissingProposalId: boolean;
datafilesActionsEnabled: boolean;
datafilesActions: any[];

Check warning on line 45 in src/app/app-config.service.ts

View workflow job for this annotation

GitHub Actions / eslint

Unexpected any. Specify a different type
editDatasetSampleEnabled: boolean;
editMetadataEnabled: boolean;
editPublishedData: boolean;
Expand Down Expand Up @@ -82,6 +82,7 @@
searchPublicDataEnabled: boolean;
searchSamples: boolean;
sftpHost: string | null;
largeDataFileAccessInstruction?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name to maxFileSizeWarning

shareEnabled: boolean;
shoppingCartEnabled: boolean;
shoppingCartOnHeader: boolean;
Expand Down
11 changes: 1 addition & 10 deletions src/app/datasets/datafiles/datafiles.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,7 @@
<ng-template [ngIf]="maxFileSize && tooLargeFile">
<span>
<mat-icon class="warning-icon"> warning </mat-icon>
One or more files are too large to be downloaded directly.
{{
sftpHost
? "These files are available via sftp host " +
sftpHost +
" in directory " +
sourcefolder +
"."
: ""
}}
<span [innerHTML]="replace_dynamic_values()"></span>
</span>
</ng-template>

Expand Down
10 changes: 9 additions & 1 deletion src/app/datasets/datafiles/datafiles.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ export class DatafilesComponent
this.appConfig.fileserverButtonLabel || "Download";
multipleDownloadAction: string | null = this.appConfig.multipleDownloadAction;
maxFileSize: number | null = this.appConfig.maxDirectDownloadSize;
sftpHost: string | null = this.appConfig.sftpHost;
sftpHost: string = this.appConfig.sftpHost || "no sftp host provided";
customSourcefolder: string | null =
this.appConfig.largeDataFileAccessInstruction;
Copy link
Contributor

Choose a reason for hiding this comment

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

Change it to:

  maxFileSizeWarning: string = 
    this.appConfig.maxFileSizeWarning || 
    `Some files are above the max size (<maxFileSize>)`

jwt: any;
auth_token: any;

Expand Down Expand Up @@ -204,6 +206,12 @@ export class DatafilesComponent
}
}

replace_dynamic_values() {
return `Some files are too big, but they can be downloaded
at our sftp server: <strong>${this.sftpHost}</strong>
at the folder: <strong>${this.customSourcefolder || this.sourcefolder}</strong>`;
}

ngAfterViewInit() {
this.subscriptions.push(
this.dataset$.subscribe((dataset) => {
Expand Down
8 changes: 6 additions & 2 deletions src/app/datasets/dataset-detail/dataset-detail.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -268,13 +268,17 @@
<a (click)="onClickSample(sample.sampleId)">
<span>{{ sample.description }}</span>
</a>
<span
<!-- NOTE: Sample editing is currently disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep this or remove it completely from the code for now?

Copy link
Contributor Author

@Junjiequan Junjiequan Nov 20, 2024

Choose a reason for hiding this comment

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

I agree. Maybe it's better off to remove this code as we are not certain if we need this feature or not, instead of polluting the source code.
What's your thoughts? @nitrosx

Copy link
Contributor

Choose a reason for hiding this comment

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

I have mixed feelings... THINKING THINKING...
Let's remove it!!!

This feature not only contains bugs that need to be fixed,
but we also want to have a centralized edit button in one location.
-->
<!-- <span
class="sample-edit"
(click)="openSampleEditDialog()"
*ngIf="appConfig.editDatasetSampleEnabled && editingAllowed"
>
<mat-icon>edit</mat-icon>
</span>
</span> -->
</td>
</tr>
<tr *ngIf="dataset['instrumentId'] && instrument">
Expand Down
58 changes: 31 additions & 27 deletions src/app/datasets/dataset-detail/dataset-detail.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,33 +341,37 @@ describe("DatasetDetailComponent", () => {
});
});

describe("#openSampleEditDialog()", () => {
it("should open the sample edit dialog and dispatch updatePropertyAction", () => {
const dispatchSpy = spyOn(store, "dispatch");
component.dataset = new Dataset();
component.dataset.ownerGroup = "test";
component.sample = new Sample();
const sampleId = "testId";
component.sample.sampleId = sampleId;
const pid = "testPid";
component.dataset.pid = pid;
const property = { sampleId };
const dialogOpenSpy = spyOn(component.dialog, "open").and.returnValue({
afterClosed: () => of({ sample: { sampleId: "testId" } }),
} as MatDialogRef<SampleEditComponent>);
component.openSampleEditDialog();

expect(dialogOpenSpy).toHaveBeenCalledTimes(1);
expect(dialogOpenSpy).toHaveBeenCalledWith(SampleEditComponent, {
width: "1000px",
data: { ownerGroup: "test", sampleId: "testId" },
});
expect(dispatchSpy).toHaveBeenCalledTimes(1);
expect(dispatchSpy).toHaveBeenCalledWith(
updatePropertyAction({ pid, property }),
);
});
});
// NOTE: Sample editing is currently disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is better to remove this instead of just commenting it

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Remove it.
Let's clean up the code

// This feature not only contains bugs that need to be fixed,
// but we also want to have a centralized edit button in one location.

// describe("#openSampleEditDialog()", () => {
// it("should open the sample edit dialog and dispatch updatePropertyAction", () => {
// const dispatchSpy = spyOn(store, "dispatch");
// component.dataset = new Dataset();
// component.dataset.ownerGroup = "test";
// component.sample = new Sample();
// const sampleId = "testId";
// component.sample.sampleId = sampleId;
// const pid = "testPid";
// component.dataset.pid = pid;
// const property = { sampleId };
// const dialogOpenSpy = spyOn(component.dialog, "open").and.returnValue({
// afterClosed: () => of({ sample: { sampleId: "testId" } }),
// } as MatDialogRef<SampleEditComponent>);
// component.openSampleEditDialog();

// expect(dialogOpenSpy).toHaveBeenCalledTimes(1);
// expect(dialogOpenSpy).toHaveBeenCalledWith(SampleEditComponent, {
// width: "1000px",
// data: { ownerGroup: "test", sampleId: "testId" },
// });
// expect(dispatchSpy).toHaveBeenCalledTimes(1);
// expect(dispatchSpy).toHaveBeenCalledWith(
// updatePropertyAction({ pid, property }),
// );
// });
// });

describe("#onSaveMetadata()", () => {
it("should dispatch an updatePropertyAction", () => {
Expand Down
55 changes: 27 additions & 28 deletions src/app/datasets/dataset-detail/dataset-detail.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ENTER, COMMA, SPACE } from "@angular/cdk/keycodes";
import { MatChipInputEvent } from "@angular/material/chips";

import { MatDialog } from "@angular/material/dialog";
import { SampleEditComponent } from "datasets/sample-edit/sample-edit.component";
// import { SampleEditComponent } from "datasets/sample-edit/sample-edit.component";
import { DialogComponent } from "shared/modules/dialog/dialog.component";
import { combineLatest, fromEvent, Observable, Subscription } from "rxjs";
import { Store } from "@ngrx/store";
Expand Down Expand Up @@ -277,28 +277,31 @@ export class DatasetDetailComponent
this.router.navigateByUrl("/samples/" + id);
}

openSampleEditDialog() {
if (this.dataset) {
this.dialog
.open(SampleEditComponent, {
width: "1000px",
data: {
ownerGroup: this.dataset.ownerGroup,
sampleId: this.sample?.sampleId,
},
})
.afterClosed()
.subscribe((res) => {
if (res && this.dataset) {
const { sample } = res;
this.sample = sample;
const pid = this.dataset.pid;
const property = { sampleId: sample.sampleId };
this.store.dispatch(updatePropertyAction({ pid, property }));
}
});
}
}
// NOTE: Sample editing is currently disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it

// This feature not only contains bugs that need to be fixed,
// but we also want to have a centralized edit button in one location.
// openSampleEditDialog() {
// if (this.dataset) {
// this.dialog
// .open(SampleEditComponent, {
// width: "1000px",
// data: {
// ownerGroup: this.dataset.ownerGroup,
// sampleId: this.sample?.sampleId,
// },
// })
// .afterClosed()
// .subscribe((res) => {
// if (res && this.dataset) {
// const { sample } = res;
// this.sample = sample;
// const pid = this.dataset.pid;
// const property = { sampleId: sample.sampleId };
// this.store.dispatch(updatePropertyAction({ pid, property }));
// }
// });
// }
// }

onSlidePublic(event: MatSlideToggleChange) {
if (this.dataset) {
Expand Down Expand Up @@ -351,11 +354,7 @@ export class DatasetDetailComponent
}

getImageUrl(encoded: string) {
const mimeType = this.base64MimeType(encoded);
if (mimeType === "application/pdf") {
return "assets/images/pdf-icon.svg";
}
return encoded;
return this.attachmentService.getImageUrl(encoded);
}

openAttachment(encoded: string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
<mat-cell *matCellDef="let dataset">
<img
src="{{ dataset.pid | thumbnail | async }}"
height="60px"
height="42px"
loading="lazy"
/>
</mat-cell>
Expand Down
1 change: 0 additions & 1 deletion src/app/datasets/datasets.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ import { DatasetsFilterSettingsComponent } from "./datasets-filter/settings/data
import { CdkDrag, CdkDragHandle, CdkDropList } from "@angular/cdk/drag-drop";
import { FiltersModule } from "shared/modules/filters/filters.module";
import { userReducer } from "state-management/reducers/user.reducer";
import { AttachmentService } from "shared/services/attachment.service";

@NgModule({
imports: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,7 @@ export class FileUploaderComponent {
}

getImageUrl(encoded: string) {
const mimeType = this.base64MimeType(encoded);
if (mimeType === "application/pdf") {
return "assets/images/pdf-icon.svg";
}
return encoded;
return this.attachmentService.getImageUrl(encoded);
}

openAttachment(encoded: string) {
Expand Down
16 changes: 15 additions & 1 deletion src/app/shared/services/attachment.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,25 @@ describe("AttachmentService", () => {
});

it("should return null for non-string input", () => {
const mimeType = service.base64MimeType(null as any);
const mimeType = service.base64MimeType(null);
expect(mimeType).toBeNull();
});
});

describe("getImageUrl", () => {
it("should return the pdf icon if the file is pdf", () => {
const encoded = "data:application/pdf;base64,SGVsbG8sIFdvcmxkIQ==";
const imageUrl = service.getImageUrl(encoded);
expect(imageUrl).toBe("assets/images/pdf-icon.svg");
});

it("should return the encoded string if the file is not pdf", () => {
const encoded = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA";
const imageUrl = service.getImageUrl(encoded);
expect(imageUrl).toBe(encoded);
});
});

describe("openAttachment", () => {
it("should open a new window with the correct object URL", () => {
const encoded = "data:text/plain;base64,SGVsbG8sIFdvcmxkIQ==";
Expand Down
21 changes: 21 additions & 0 deletions src/app/shared/services/attachment.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import { Injectable } from "@angular/core";
@Injectable()
export class AttachmentService {
base64MimeType(encoded: string): string {
if (!encoded) {
return null;
}

let result = null;

if (typeof encoded !== "string") {
Expand All @@ -17,7 +21,24 @@ export class AttachmentService {

return result;
}

getImageUrl(encoded: string) {
if (!encoded) {
return null;
}

const mimeType = this.base64MimeType(encoded);
if (mimeType === "application/pdf") {
return "assets/images/pdf-icon.svg";
}
return encoded;
}

openAttachment(encoded: string) {
if (!encoded) {
return null;
}

const mimeType = this.base64MimeType(encoded);
const strippedData = encoded.replace(
new RegExp(`^data:${mimeType};base64,`),
Expand Down
4 changes: 4 additions & 0 deletions src/app/shared/services/thumbnail.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ThumbnailService } from "./thumbnail.service";
import { selectDatasetsPerPage } from "state-management/selectors/datasets.selectors";
import { MockStore, provideMockStore } from "@ngrx/store/testing";
import { of, throwError } from "rxjs";
import { AttachmentService } from "./attachment.service";

describe("ThumbnailService", () => {
let service: ThumbnailService;
Expand All @@ -27,6 +28,9 @@ describe("ThumbnailService", () => {
provide: AppConfigService,
useValue: { getConfig },
},
{
provide: AttachmentService,
},
provideMockStore({
selectors: [
{
Expand Down
4 changes: 3 additions & 1 deletion src/app/shared/services/thumbnail.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { distinctUntilChanged, firstValueFrom } from "rxjs";
import { DatasetApi } from "shared/sdk";
import { selectDatasetsPerPage } from "state-management/selectors/datasets.selectors";
import { AppConfigService } from "app-config.service";
import { AttachmentService } from "./attachment.service";

interface ThumbnailCache {
[pid: string]: {
Expand All @@ -25,6 +26,7 @@ export class ThumbnailService {
constructor(
private datasetApi: DatasetApi,
private store: Store,
private attachmentService: AttachmentService,
private appConfigService: AppConfigService,
) {
this.store
Expand Down Expand Up @@ -60,7 +62,7 @@ export class ThumbnailService {
try {
const encodedPid = encodeURIComponent(pid);
const res = await firstValueFrom(this.datasetApi.thumbnail(encodedPid));
const thumbnail = res?.thumbnail || null;
const thumbnail = this.attachmentService.getImageUrl(res?.thumbnail);

this.thumbnailCache[pid] = {
value: thumbnail,
Expand Down
Loading