Skip to content

Commit 2dc857b

Browse files
milanmajchrakclaude
andcommitted
UoE/Remove debug console output and avoidable failed requests
Make the running app look professional by removing development console logging and eliminating avoidable failed network requests. Each issue was reproduced against the live dev and production instances and traced to its source. Console noise removed (fired during normal browser use): - "Bitstream: <object>" logged once per file on every item page - "Environment extended with app config" on every page load - debug logs across the submission, upload, access-control and datashare services, the submission footer and the section container Avoidable failed requests eliminated (the request is now avoided, since a browser always logs a network 404/401 that JS cannot suppress): - 404 google.analytics.key on every page -> gated behind a new info.enableGoogleAnalytics flag (default false; DataShare does not use GA) - 404 bulkedit.export.max.items on /search -> only requested once the admin export button is shown - 401 qualityassurancesources on item pages for anonymous users -> gated on the CanSeeQA authorization inside the QA component - 404 /favicon.ico -> the DataShare favicon is served from the web root Specs updated for the new GA flag and CanSeeQA gate (with added coverage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9fe5e6a commit 2dc857b

21 files changed

Lines changed: 176 additions & 58 deletions

angular.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,12 @@
4040
"aot": true,
4141
"assets": [
4242
"src/assets",
43-
"src/robots.txt"
43+
"src/robots.txt",
44+
{
45+
"glob": "favicon.ico",
46+
"input": "src/themes/datashare/assets/images",
47+
"output": "/"
48+
}
4449
],
4550
"styles": [
4651
"src/styles/startup.scss",

config/config.example.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,10 @@ info:
465465
enableEndUserAgreement: true
466466
enablePrivacyStatement: true
467467
enableCOARNotifySupport: true
468+
# Only enable Google Analytics on installations that configure a `google.analytics.key`
469+
# property on the backend. When false (default) the UI does not probe for that property,
470+
# avoiding a 404 request on every page.
471+
enableGoogleAnalytics: false
468472

469473
# Whether to enable Markdown (https://commonmark.org/) and MathJax (https://www.mathjax.org/)
470474
# display in supported metadata fields. By default, only dc.description.abstract is supported.

src/app/datashare/datashare-submission-form-section-container.service.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ export class DatashareSubmissionFormSectionContainerService {
1919
* @param id The ID of the panel to open, or null to close all panels
2020
*/
2121
setOpenPanelId(id: string | null): void {
22-
console.log('Setting open panel ID to:', id);
2322
this.openPanelId.set(id);
2423
}
2524
}

src/app/datashare/datashare-submission.service.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@ export class DatashareSubmissionService {
2323
constructor(private notificationsService: NotificationsService,
2424
private translate: TranslateService,
2525
) {
26-
console.log('DatashareSubmissionService created');
2726
}
2827

2928
/**
3029
* Update the deposit button visibility state
3130
*/
3231
updatehasUploadFilesErrors(show: boolean): void {
33-
console.log('Updating hasUploadFilesErrors to:', show);
3432
this._hasUploadFilesErrorsSignal.set(show);
3533
}
3634

src/app/item-page/simple/field-components/file-section/file-section.component.ts

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -154,10 +154,6 @@ export class FileSectionComponent implements OnInit {
154154
this.notificationsService.error(this.translateService.get('file-section.error.header'), `${bitstreamsRD.statusCode} ${bitstreamsRD.errorMessage}`);
155155
} else if (hasValue(bitstreamsRD.payload)) {
156156
const current: Bitstream[] = this.bitstreams$.getValue();
157-
// For debugging.
158-
bitstreamsRD.payload.page.forEach(bitstream => {
159-
console.log('Bitstream:', bitstream);
160-
});
161157
this.bitstreams$.next([...current, ...bitstreamsRD.payload.page]);
162158
this.isLoading = false;
163159
this.isLastPage = this.currentPage === bitstreamsRD.payload.totalPages;
@@ -169,10 +165,6 @@ export class FileSectionComponent implements OnInit {
169165
this.notificationsService.error(this.translateService.get('file-section.error.header'), `${licenseRD.statusCode} ${licenseRD.errorMessage}`);
170166
} else if (hasValue(licenseRD.payload)) {
171167
const updated: Bitstream[] = this.bitstreams$.getValue();
172-
// For debugging.
173-
licenseRD.payload.page.forEach(bitstream => {
174-
console.log('Bitstream:', bitstream);
175-
});
176168
this.bitstreams$.next([...updated, ...licenseRD.payload.page]);
177169
}
178170
this.isLoading = false;

src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { SplitPipe } from 'src/app/shared/utils/split.pipe';
1313
import { APP_DATA_SERVICES_MAP } from '../../../../config/app-config.interface';
1414
import { RemoteDataBuildService } from '../../../core/cache/builders/remote-data-build.service';
1515
import { ObjectCacheService } from '../../../core/cache/object-cache.service';
16+
import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service';
1617
import { RequestService } from '../../../core/data/request.service';
1718
import { QualityAssuranceSourceObject } from '../../../core/notifications/qa/models/quality-assurance-source.model';
1819
import { QualityAssuranceSourceDataService } from '../../../core/notifications/qa/source/quality-assurance-source-data.service';
@@ -28,6 +29,7 @@ describe('QaEventNotificationComponent', () => {
2829
let component: QaEventNotificationComponent;
2930
let fixture: ComponentFixture<QaEventNotificationComponent>;
3031
let qualityAssuranceSourceDataServiceStub: any;
32+
let authorizationServiceStub: any;
3133

3234
const obj = Object.assign(new QualityAssuranceSourceObject(), {
3335
id: 'sourceName:target',
@@ -41,14 +43,18 @@ describe('QaEventNotificationComponent', () => {
4143
beforeEach(async () => {
4244

4345
qualityAssuranceSourceDataServiceStub = {
44-
getSourcesByTarget: () => objPL,
46+
getSourcesByTarget: jasmine.createSpy('getSourcesByTarget').and.returnValue(objPL),
47+
};
48+
authorizationServiceStub = {
49+
isAuthorized: jasmine.createSpy('isAuthorized').and.returnValue(of(true)),
4550
};
4651
await TestBed.configureTestingModule({
4752
imports: [CommonModule, TranslateModule.forRoot(), QaEventNotificationComponent, SplitPipe],
4853
providers: [
4954
{ provide: APP_DATA_SERVICES_MAP, useValue: {} },
5055
{ provide: ActivatedRoute, useValue: new ActivatedRouteStub() },
5156
{ provide: QualityAssuranceSourceDataService, useValue: qualityAssuranceSourceDataServiceStub },
57+
{ provide: AuthorizationDataService, useValue: authorizationServiceStub },
5258
{ provide: RequestService, useValue: {} },
5359
{ provide: NotificationsService, useValue: {} },
5460
{ provide: HALEndpointService, useValue: new HALEndpointServiceStub('test') },
@@ -57,6 +63,12 @@ describe('QaEventNotificationComponent', () => {
5763
provideMockStore({}),
5864
],
5965
})
66+
// The component declares QualityAssuranceSourceDataService in its own `providers`,
67+
// which would otherwise shadow the stub above with a real instance. Override it so
68+
// the component uses the stub we can assert against.
69+
.overrideComponent(QaEventNotificationComponent, {
70+
set: { providers: [{ provide: QualityAssuranceSourceDataService, useValue: qualityAssuranceSourceDataServiceStub }] },
71+
})
6072
.compileComponents();
6173
fixture = TestBed.createComponent(QaEventNotificationComponent);
6274
component = fixture.componentInstance;
@@ -78,4 +90,22 @@ describe('QaEventNotificationComponent', () => {
7890
const route = component.getQualityAssuranceRoute();
7991
expect(route).toBe('/notifications/quality-assurance');
8092
});
93+
94+
it('should request QA sources when the user is authorized to see QA', () => {
95+
authorizationServiceStub.isAuthorized.and.returnValue(of(true));
96+
qualityAssuranceSourceDataServiceStub.getSourcesByTarget.calls.reset();
97+
let result: QualityAssuranceSourceObject[];
98+
component.getQualityAssuranceSources$().subscribe((sources) => result = sources);
99+
expect(qualityAssuranceSourceDataServiceStub.getSourcesByTarget).toHaveBeenCalled();
100+
expect(result).toEqual([obj]);
101+
});
102+
103+
it('should NOT request QA sources when the user is not authorized', () => {
104+
authorizationServiceStub.isAuthorized.and.returnValue(of(false));
105+
qualityAssuranceSourceDataServiceStub.getSourcesByTarget.calls.reset();
106+
let result: QualityAssuranceSourceObject[];
107+
component.getQualityAssuranceSources$().subscribe((sources) => result = sources);
108+
expect(qualityAssuranceSourceDataServiceStub.getSourcesByTarget).not.toHaveBeenCalled();
109+
expect(result).toEqual([]);
110+
});
81111
});

src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,20 @@ import {
1212
} from '@angular/core';
1313
import { RouterLink } from '@angular/router';
1414
import { TranslateModule } from '@ngx-translate/core';
15-
import { Observable } from 'rxjs';
15+
import {
16+
Observable,
17+
of as observableOf,
18+
} from 'rxjs';
1619
import {
1720
catchError,
1821
map,
22+
switchMap,
1923
} from 'rxjs/operators';
2024

2125
import { getNotificatioQualityAssuranceRoute } from '../../../admin/admin-routing-paths';
2226
import { RequestParam } from '../../../core/cache/models/request-param.model';
27+
import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service';
28+
import { FeatureID } from '../../../core/data/feature-authorization/feature-id';
2329
import { FindListOptions } from '../../../core/data/find-list-options.model';
2430
import { PaginatedList } from '../../../core/data/paginated-list.model';
2531
import { RemoteData } from '../../../core/data/remote-data';
@@ -61,6 +67,7 @@ export class QaEventNotificationComponent implements OnChanges {
6167

6268
constructor(
6369
private qualityAssuranceSourceDataService: QualityAssuranceSourceDataService,
70+
private authorizationService: AuthorizationDataService,
6471
) {}
6572

6673
/**
@@ -77,20 +84,31 @@ export class QaEventNotificationComponent implements OnChanges {
7784
* Note: sourceId is composed as: id: "sourceName:<target>"
7885
*/
7986
getQualityAssuranceSources$(): Observable<QualityAssuranceSourceObject[]> {
80-
const findListTopicOptions: FindListOptions = {
81-
searchParams: [new RequestParam('target', this.item.uuid)],
82-
};
83-
return this.qualityAssuranceSourceDataService.getSourcesByTarget(findListTopicOptions, false)
84-
.pipe(
85-
getFirstCompletedRemoteData(),
86-
map((data: RemoteData<PaginatedList<QualityAssuranceSourceObject>>) => {
87-
if (data.hasSucceeded) {
88-
return data.payload.page;
89-
}
90-
return [];
91-
}),
92-
catchError(() => []),
93-
);
87+
// Quality Assurance sources are only available to authorized users. Checking the
88+
// authorization first avoids issuing the `qualityassurancesources/search/byTarget`
89+
// request for anonymous/unauthorized users, which would otherwise return a 401 that
90+
// shows up as a failed request in the browser. Authorized users keep the same behavior.
91+
return this.authorizationService.isAuthorized(FeatureID.CanSeeQA).pipe(
92+
switchMap((canSeeQA: boolean) => {
93+
if (!canSeeQA) {
94+
return observableOf([] as QualityAssuranceSourceObject[]);
95+
}
96+
const findListTopicOptions: FindListOptions = {
97+
searchParams: [new RequestParam('target', this.item.uuid)],
98+
};
99+
return this.qualityAssuranceSourceDataService.getSourcesByTarget(findListTopicOptions, false)
100+
.pipe(
101+
getFirstCompletedRemoteData(),
102+
map((data: RemoteData<PaginatedList<QualityAssuranceSourceObject>>) => {
103+
if (data.hasSucceeded) {
104+
return data.payload.page;
105+
}
106+
return [];
107+
}),
108+
);
109+
}),
110+
catchError(() => observableOf([] as QualityAssuranceSourceObject[])),
111+
);
94112
}
95113

96114
/**

src/app/shared/access-control-form-container/access-control-form-container.component.ts

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,6 @@ export class AccessControlFormContainerComponent<T extends DSpaceObject> impleme
9898
* Will be used from a parent component to read the value of the form
9999
*/
100100
getFormValue() {
101-
console.log({
102-
bitstream: this.bitstreamAccessCmp.getValue(),
103-
item: this.itemAccessCmp.getValue(),
104-
state: this.state,
105-
});
106101
return {
107102
bitstream: this.bitstreamAccessCmp.getValue(),
108103
item: this.itemAccessCmp.getValue(),
@@ -137,9 +132,7 @@ export class AccessControlFormContainerComponent<T extends DSpaceObject> impleme
137132
this.bulkAccessControlService.executeScript(
138133
[ this.itemRD.payload.uuid ],
139134
file,
140-
).pipe(take(1)).subscribe((res) => {
141-
console.log('success', res);
142-
});
135+
).pipe(take(1)).subscribe();
143136
}
144137

145138
/**

src/app/shared/access-control-form-container/bulk-access-control.service.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,6 @@ export class BulkAccessControlService {
5959
* @param file
6060
*/
6161
executeScript(uuids: string[], file: File): Observable<boolean> {
62-
console.log('execute', { uuids, file });
63-
6462
const params: ProcessParameter[] = [
6563
{ name: '-f', value: file.name },
6664
];

src/app/shared/cookies/browser-klaro.service.spec.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import cloneDeep from 'lodash/cloneDeep';
66
import { of as observableOf } from 'rxjs';
77
import { TestScheduler } from 'rxjs/testing';
88

9+
import { environment } from '../../../environments/environment';
910
import { AuthService } from '../../core/auth/auth.service';
1011
import { RestResponse } from '../../core/cache/response.models';
1112
import { ConfigurationDataService } from '../../core/data/configuration-data.service';
@@ -310,6 +311,9 @@ describe('BrowserKlaroService', () => {
310311
let GOOGLE_ANALYTICS_KEY;
311312
let REGISTRATION_VERIFICATION_ENABLED_KEY;
312313
beforeEach(() => {
314+
// The Google Analytics key is only probed when GA is enabled for the installation;
315+
// these tests exercise that probe-and-filter path.
316+
environment.info.enableGoogleAnalytics = true;
313317
GOOGLE_ANALYTICS_KEY = clone((service as any).GOOGLE_ANALYTICS_KEY);
314318
REGISTRATION_VERIFICATION_ENABLED_KEY = clone((service as any).REGISTRATION_VERIFICATION_ENABLED_KEY);
315319
spyOn((service as any), 'getUser$').and.returnValue(observableOf(user));
@@ -320,6 +324,10 @@ describe('BrowserKlaroService', () => {
320324
configurationDataService.findByPropertyName = findByPropertyName;
321325
});
322326

327+
afterEach(() => {
328+
environment.info.enableGoogleAnalytics = false;
329+
});
330+
323331
it('should not filter googleAnalytics when servicesToHide are empty', () => {
324332
const filteredConfig = (service as any).filterConfigServices([]);
325333
expect(filteredConfig).toContain(jasmine.objectContaining({ name: googleAnalytics }));
@@ -401,5 +409,19 @@ describe('BrowserKlaroService', () => {
401409
service.initialize();
402410
expect(service.klaroConfig.services).not.toContain(jasmine.objectContaining({ name: googleAnalytics }));
403411
});
412+
it('should hide googleAnalytics without requesting the key when GA is disabled', () => {
413+
environment.info.enableGoogleAnalytics = false;
414+
const findByPropertyNameSpy = jasmine.createSpy('findByPropertyName').and.returnValue(
415+
createSuccessfulRemoteDataObject$({
416+
...new ConfigurationProperty(),
417+
name: trackingIdTestValue,
418+
values: ['false'],
419+
}),
420+
);
421+
configurationDataService.findByPropertyName = findByPropertyNameSpy;
422+
service.initialize();
423+
expect(findByPropertyNameSpy).not.toHaveBeenCalledWith(GOOGLE_ANALYTICS_KEY);
424+
expect(service.klaroConfig.services).not.toContain(jasmine.objectContaining({ name: googleAnalytics }));
425+
});
404426
});
405427
});

0 commit comments

Comments
 (0)