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.

Sign up for GitHub

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

Prevent request with page size of 9999 #3694

Merged
merged 23 commits into from
Jan 23, 2025
Merged

Prevent request with page size of 9999 #3694

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
369cb3e
[DURACOM-304] Refactored item-bitstreams.component by removing page s…
alisaismailati Nov 19, 2024
017d110
[DURACOM-304] Refactored edit-bitstream-page.component by removing pa…
alisaismailati Nov 19, 2024
a9d1518
[DURACOM-304] Refactored scripts-select.component by using infinite s…
alisaismailati Nov 20, 2024
850a951
[DURACOM-304] Refactored dynamic-list.component.ts by removing page s…
alisaismailati Nov 20, 2024
4cd5e0a
[DURACOM-304] Refactored relationship-type-data.service.ts by removin…
alisaismailati Nov 20, 2024
4830e46
[DURACOM-304] removed unneeded selectAll method (dynamic-lookup-relat…
alisaismailati Nov 21, 2024
ebb408e
[DURACOM-304] Refactored submission-section-cc-licenses.component.ts …
alisaismailati Nov 21, 2024
4642ed1
[DURACOM-304] lint fix
alisaismailati Nov 21, 2024
ac7af44
[DURACOM-304] test fix
alisaismailati Nov 21, 2024
b43e1f4
[DURACOM-304] fix accessibility issue on scripts-select
alisaismailati Nov 21, 2024
335fe62
[DURACOM-304] Refactor of bundle-data.service.ts by removing page siz…
alisaismailati Nov 21, 2024
d90a988
[DURACOM-304] other fix related to accessibility
alisaismailati Nov 21, 2024
f5ec5f9
[DURACOM-304] lint fix
alisaismailati Nov 21, 2024
d4064f6
Merge branch 'gitHub/main' into task/main/DURACOM-304
FrancescoMolinaro Dec 18, 2024
9f8083e
[DURACOM-304] resolve conflicts
FrancescoMolinaro Dec 18, 2024
3ac6cb8
Merge branch 'task/main/DURACOM-304' of github.com:4Science/dspace-an…
FrancescoMolinaro Dec 18, 2024
134ecd6
[DURACOM-304] fix lint
FrancescoMolinaro Dec 18, 2024
903daad
[DURACOM-304] add support for findAll method in dynamic-scrollable-dr…
FrancescoMolinaro Jan 14, 2025
5f2cc86
Merge branch 'main' of github.com:DSpace/dspace-angular into task/mai…
FrancescoMolinaro Jan 15, 2025
25b2f68
[DURACOM-304] refactor to use lazy data provider
FrancescoMolinaro Jan 15, 2025
c79affd
[DURACOM-304] improve loading logic for cc-licenses section and dynam…
FrancescoMolinaro Jan 15, 2025
9570cd9
[DURACOM-304] refactor, fix dynamic-list.component loading
FrancescoMolinaro Jan 17, 2025
2847dc3
[DURACOM-304] remove br
FrancescoMolinaro Jan 23, 2025
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
33 changes: 26 additions & 7 deletions src/app/bitstream-page/edit-bitstream-page/edit-bitstream-page.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@
import {
combineLatest,
combineLatest as observableCombineLatest,
EMPTY,
Observable,
of as observableOf,
Subscription,
} from 'rxjs';
import {
expand,
filter,
map,
reduce,
switchMap,
tap,
} from 'rxjs/operators';
Expand Down Expand Up @@ -178,7 +181,10 @@
/**
* Options for fetching all bitstream formats
*/
findAllOptions = { elementsPerPage: 9999 };
findAllOptions = {
elementsPerPage: 20,
currentPage: 1,
};

/**
* The Dynamic Input Model for the file's name
Expand Down Expand Up @@ -463,14 +469,27 @@
this.itemId = this.route.snapshot.queryParams.itemId;
this.entityType = this.route.snapshot.queryParams.entityType;
this.bitstreamRD$ = this.route.data.pipe(map((data: any) => data.bitstream));
this.bitstreamFormatsRD$ = this.bitstreamFormatService.findAll(this.findAllOptions);

const bitstream$ = this.bitstreamRD$.pipe(
this.bitstreamFormatsRD$ = this.bitstreamFormatService.findAll(this.findAllOptions).pipe(
getFirstSucceededRemoteData(),
getRemoteDataPayload(),
expand((response: RemoteData<PaginatedList<BitstreamFormat>>) => {
const pageInfo = response.payload.pageInfo;
if (pageInfo.currentPage < pageInfo.totalPages) {
const nextPageOptions = { ...this.findAllOptions, currentPage: pageInfo.currentPage + 1 };
return this.bitstreamFormatService.findAll(nextPageOptions).pipe(getFirstSucceededRemoteData());

Check warning on line 479 in src/app/bitstream-page/edit-bitstream-page/edit-bitstream-page.component.ts

View check run for this annotation

Codecov / codecov/patch

src/app/bitstream-page/edit-bitstream-page/edit-bitstream-page.component.ts#L478-L479 

Added lines #L478 - L479 were not covered by tests
} else {
return EMPTY;
}
}),
);

const allFormats$ = this.bitstreamFormatsRD$.pipe(
const bitstreamFormats$ = this.bitstreamFormatsRD$.pipe(
reduce((acc: BitstreamFormat[], response: RemoteData<PaginatedList<BitstreamFormat>>) => {
return acc.concat(response.payload.page);
}, []),
);

const bitstream$ = this.bitstreamRD$.pipe(
getFirstSucceededRemoteData(),
getRemoteDataPayload(),
);
Expand All @@ -493,14 +512,14 @@
this.subs.push(
observableCombineLatest(
bitstream$,
allFormats$,
bitstreamFormats$,
bundle$,
primaryBitstream$,
item$,
).pipe()
.subscribe(([bitstream, allFormats, bundle, primaryBitstream, item]) => {
this.bitstream = bitstream as Bitstream;
this.formats = allFormats.page;
this.formats = allFormats;
this.bundle = bundle;
// hasValue(primaryBitstream) because if there's no primaryBitstream on the bundle it will
// be a success response, but empty
Expand Down
5 changes: 3 additions & 2 deletions src/app/core/data/bitstream-data.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,12 @@ export class BitstreamDataService extends IdentifiableDataService<Bitstream> imp
* no valid cached version. Defaults to true
* @param reRequestOnStale Whether or not the request should automatically be re-
* requested after the response becomes stale
* @param options the {@link FindListOptions} for the request
* @return {Observable<Bitstream | null>}
* Return an observable that contains primary bitstream information or null
*/
public findPrimaryBitstreamByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true): Observable<Bitstream | null> {
return this.bundleService.findByItemAndName(item, bundleName, useCachedVersionIfAvailable, reRequestOnStale, followLink('primaryBitstream')).pipe(
public findPrimaryBitstreamByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions): Observable<Bitstream | null> {
return this.bundleService.findByItemAndName(item, bundleName, useCachedVersionIfAvailable, reRequestOnStale, options, followLink('primaryBitstream')).pipe(
getFirstCompletedRemoteData(),
switchMap((rd: RemoteData<Bundle>) => {
if (!rd.hasSucceeded) {
Expand Down
46 changes: 44 additions & 2 deletions src/app/core/data/bundle-data.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,11 @@ export class BundleDataService extends IdentifiableDataService<Bundle> implement
* requested after the response becomes stale
* @param linksToFollow List of {@link FollowLinkConfig} that indicate which
* {@link HALLink}s should be automatically resolved
* @param options the {@link FindListOptions} for the request
*/
// TODO should be implemented rest side
findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
return this.findAllByItem(item, { elementsPerPage: 9999 }, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(
findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
return this.findAllByItem(item, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this change will work. The method worked by retrieving all bundles and filtering on their name client side. If we no longer retrieve all bundles that filter won't work. The only way it can be fixed is with a rest endpoint I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @artlowel thanks for the catch!

I though we could have use the limitation for bitstreams in the config ( I believe is 5 per page ) but is actually not true, that changes case by case, my bad.

I have kept the implementation because I think the method should support anyway the pagination and instead of the previous loop with expand I set the pagination parameter of 9999, I know this goes in the opposite direction of the PR but I agree that, in terms of rendering time, one big call is preferable to n small calls.
The solution, as you mentioned, will have to be a new endpoint that returns all the bitstreams by bundle name.

In addition to this I have also adapted my changes to the dynamic-list.component.ts as I realized that a "load more" button was not the right approach.
The issue that I noticed was happening in case the user would select an option out of range, save the submission, then reload the page; in this case the user would have had to load all options before being able to see his own selection.

The new solution I adopted is to keep the paginated calls, render the first ones as soon as they are ready, then keep loading the next ones in the background, adding them to the view progressively.

I will look forward to your feedback and thanks again!

Copy link
Member

@artlowel artlowel Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @FrancescoMolinaro

I can't seem to find a submission config that actually ends up using dynamic-list.component.ts.

Before I tested it by simply replacing the tag component with the list component here, and checking the subject field. However that keeps loading infinitely for me now. That might be due to differences between tags and lists tough.

Could you please let me know the backend submission field config you're using to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @artlowel , to visualize a dynamic-list in the submission form I am using a configuration like the following:

<field> <dc-schema>dc</dc-schema> <dc-element>identifier</dc-element> <label>Identifiers</label> <input-type value-pairs-name="common_identifiers">list</input-type> <repeatable>true</repeatable> <required /> <hint>Test hint</hint> </field>

The needed part is an input type of value "list" and a vocabulary, then we have two possible list based on the "repeatable" parameter.
Let me know in case this shouldn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that config works.

I'd still remove the line break between the pages.

image
(In this screenshot I reduced the page size to 2, to get a better idea how pagination works)

After they've loaded it doesn't matter for the user where the page breaks were:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @artlowel , thanks again for the feedback, I have removed the br, I agree it would just weirdly separate the options.

map((rd: RemoteData<PaginatedList<Bundle>>) => {
if (hasValue(rd.payload) && hasValue(rd.payload.page)) {
const matchingBundle = rd.payload.page.find((bundle: Bundle) =>
Expand Down Expand Up @@ -114,6 +115,47 @@ export class BundleDataService extends IdentifiableDataService<Bundle> implement
);
}

// findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
// return this.findAllByItem(item, { elementsPerPage: 1, currentPage: 1 }, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(
// expand((rd: RemoteData<PaginatedList<Bundle>>) => {
// if (rd.hasSucceeded && hasValue(rd.payload) && hasValue(rd.payload.page) && rd.payload.currentPage < rd.payload.totalPages) {
// const nextPageOptions = { elementsPerPage: 1, currentPage: rd.payload.currentPage + 1 };
// return this.findAllByItem(item, nextPageOptions, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow);
// } else {
// return EMPTY;
// }
// }),
// map((rd: RemoteData<PaginatedList<Bundle>>) => {
// if (hasValue(rd.payload) && hasValue(rd.payload.page)) {
// const matchingBundle = rd.payload.page.find((bundle: Bundle) => bundle.name === bundleName);
// if (hasValue(matchingBundle)) {
// return new RemoteData(
// rd.timeCompleted,
// rd.msToLive,
// rd.lastUpdated,
// RequestEntryState.Success,
// null,
// matchingBundle,
// 200
// );
// } else {
// return new RemoteData(
// rd.timeCompleted,
// rd.msToLive,
// rd.lastUpdated,
// RequestEntryState.Error,
// `The bundle with name ${bundleName} was not found.`,
// null,
// 404
// );
// }
// } else {
// return rd as any;
// }
// })
// );
// }
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be removed


/**
* Get the bitstreams endpoint for a bundle
* @param bundleId
Expand Down
24 changes: 21 additions & 3 deletions src/app/core/data/relationship-type-data.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { Injectable } from '@angular/core';
import {
combineLatest as observableCombineLatest,
EMPTY,
from,
Observable,
} from 'rxjs';
import {
expand,
map,
mergeMap,
switchMap,
reduce,
toArray,
} from 'rxjs/operators';

Expand Down Expand Up @@ -75,11 +78,26 @@
*/
getRelationshipTypeByLabelAndTypes(relationshipTypeLabel: string, firstItemType: string, secondItemType: string): Observable<RelationshipType> {
// Retrieve all relationship types from the server in a single page
return this.findAllData.findAll({ currentPage: 1, elementsPerPage: 9999 }, true, true, followLink('leftType'), followLink('rightType'))
const initialPageInfo = { currentPage: 1, elementsPerPage: 20 };
return this.findAllData.findAll(initialPageInfo, true, true, followLink('leftType'), followLink('rightType'))
.pipe(
getFirstSucceededRemoteData(),
// Emit each type in the page array separately
switchMap((typeListRD: RemoteData<PaginatedList<RelationshipType>>) => typeListRD.payload.page),
expand((typeListRD: RemoteData<PaginatedList<RelationshipType>>) => {
const currentPage = typeListRD.payload.pageInfo.currentPage;
const totalPages = typeListRD.payload.pageInfo.totalPages;
if (currentPage < totalPages) {
const nextPageInfo = { currentPage: currentPage + 1, elementsPerPage: 20 };
return this.findAllData.findAll(nextPageInfo, true, true, followLink('leftType'), followLink('rightType')).pipe(

Check warning on line 91 in src/app/core/data/relationship-type-data.service.ts

View check run for this annotation

Codecov / codecov/patch

src/app/core/data/relationship-type-data.service.ts#L90-L91 

Added lines #L90 - L91 were not covered by tests
getFirstSucceededRemoteData(),
);
} else {
return EMPTY;
}
}),
// Collect all pages into a single array
reduce((acc: RelationshipType[], typeListRD: RemoteData<PaginatedList<RelationshipType>>) => acc.concat(typeListRD.payload.page), []),
mergeMap((relationshipTypes: RelationshipType[]) => from(relationshipTypes)),
// Check each type individually, to see if it matches the provided types
mergeMap((relationshipType: RelationshipType) => {
if (relationshipType.leftwardType === relationshipTypeLabel) {
Expand Down
2 changes: 2 additions & 0 deletions src/app/core/metadata/head-tag.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import { coreSelector } from '../core.selectors';
import { CoreState } from '../core-state.model';
import { BundleDataService } from '../data/bundle-data.service';
import { AuthorizationDataService } from '../data/feature-authorization/authorization-data.service';
import { FindListOptions } from '../data/find-list-options.model';
import { PaginatedList } from '../data/paginated-list.model';
import { RemoteData } from '../data/remote-data';
import { RootDataService } from '../data/root-data.service';
Expand Down Expand Up @@ -331,6 +332,7 @@ export class HeadTagService {
'ORIGINAL',
true,
true,
new FindListOptions(),
followLink('primaryBitstream'),
followLink('bitstreams', {
findListOptions: {
Expand Down
3 changes: 3 additions & 0 deletions src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
[isFirstTable]="isFirst"
aria-describedby="reorder-description">
</ds-item-edit-bitstream-bundle>
<div class="d-flex justify-content-center" *ngIf="showLoadMoreLink$ | async">
<div class="btn btn-link py-3" (click)="loadBundles()"> {{'item.edit.bitstreams.load-more.link' | translate}}</div>
Copy link
Member

Choose a reason for hiding this comment

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

We've just done a major overhaul of this page to improve the accessibility in #3464

Please use an actual button here, so it can be accessed using the keyboard

</div>
</div>
<div *ngIf="bundles?.length === 0"
class="alert alert-info w-100 d-inline-block mt-4" role="alert">
Expand Down
Loading
Loading