680 Fix change event reference for recent submission

[AndrewZachWood]

Fixes #680
I've updated the event change references to valid ones so the onPaginationChange(event) functions properly. Tested my additions in my local environment.

NOTE: The following error was reported before my changes but seems rather strange that this particular error is still present in my console given I've seen the pattern I used elsewhere in the code base without such an error. It's unclear to me why paginationConfig properties throw an error when attempting to be set. Would be great to get some insight on this:
core.js:4002 ERROR TypeError: Cannot read property 'currentPage' of undefined at CollectionPageComponent.push../src/app/+collection-page/collection-page.component.ts.CollectionPageComponent.onPaginationChange (collection-page.component.ts:112) at SafeSubscriber._next (collection-page.component.ts:102) at SafeSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.SafeSubscriber.__tryOrUnsub (Subscriber.js:194) at SafeSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.SafeSubscriber.next (Subscriber.js:132) at Subscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber._next (Subscriber.js:76) at Subscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:53) at TakeSubscriber.push../node_modules/rxjs/_esm5/internal/operators/take.js.TakeSubscriber._next (take.js:40) at TakeSubscriber.push../node_modules/rxjs/_esm5/internal/Subscriber.js.Subscriber.next (Subscriber.js:53) at BehaviorSubject.push../node_modules/rxjs/_esm5/internal/BehaviorSubject.js.BehaviorSubject._subscribe (BehaviorSubject.js:22) at BehaviorSubject.push../node_modules/rxjs/_esm5/internal/Observable.js.Observable._trySubscribe (Observable.js:43)

[tdonohue]

@AndrewZWood : I've looked at this and the code seems to work. I do still see the TypeError you mentioned. It seems to ONLY occur on the initial page load (for a Collection page) and not after clicking to the next page or similar. Here's the error I see:

ERROR TypeError: Cannot read property 'currentPage' of undefined
    at n.onPaginationChange (collection-page.component.ts:112)
    at t._next (collection-page.component.ts:102)
    at t.__tryOrUnsub (Subscriber.js.pre-build-optimizer.js:194)
    at t.next (Subscriber.js.pre-build-optimizer.js:132)
    at t._next (Subscriber.js.pre-build-optimizer.js:76)
    at t.next (Subscriber.js.pre-build-optimizer.js:53)
    at t._next (take.js.pre-build-optimizer.js:40)
    at t.next (Subscriber.js.pre-build-optimizer.js:53)
    at t._subscribe (BehaviorSubject.js.pre-build-optimizer.js:22)
    at t.e._trySubscribe (Observable.js.pre-build-optimizer.js:43)

If you look closely at that message, it looks to be calling out line 102 in the ngOnInit() function here:

dspace-angular/src/app/+collection-page/collection-page.component.ts

Line 102 in 6c3c1ad

this.onPaginationChange(params);

So, while your new code seems correct, I think the ngOnInit() needs updates. The ngOnInit() code in that area seems odd to me...I'm not very familiar with this code, but in other components using similar pagination (e.g. top-level-community-list.component.ts) I'm not seeing that same ngOnInit() code.

This might be something @artlowel could answer more quickly than I could...but I'm wondering myself if that ngOnInit() code is needed or simply needs larger refactoring.

[artlowel]

The issue here is this line. It passes the route params as an onPaginationChange event, but that's no longer valid. The structure of those events has changed. That change is the reason for this PR: it fixes the logic inside onPaginationChange.

If you create an interface for onPaginationChange events (they have a pagination: PaginationComponentOptions and a sort: SortOptions property), and add that as a type to the event param for onPaginationChange the compiler will point out these kinds of errors.

For now, that call using the route params can just be removed. The pagination component itself will parse the route for now. The ticket Tim mentioned above will ensure it moves to a more suitable location later.

[AndrewZachWood]

@tdonohue Updated code to reflect on @artlowel 's input. Error is no longer present and I have tested functionality in my local environment.

[tdonohue]

@AndrewZWood : It looks like you need to rebase on the latest main code as there are merge conflicts now. Keep in mind that we recently merged an update to Angular 10, which also changed some of the lint settings...so you may need to make minor formatting changes too after rebasing (if yarn lint throws errors).

[AndrewZachWood]

@tdonohue Sorry about that....rebased and checked against the newer lint settings.

[tdonohue]

👍 @AndrewZWood , I tested this today & it fixes the bug! The code also looks good to me, although we do need TypeDocs/comments to describe the new interface you are creating. Once those comments are added, this is ready to merge. Thanks!

[tdonohue]

Tiny thing, please add TypeDocs to describe this new interface. We require TypeDocs/comments for any public classes/interfaces/methods.

[tdonohue]

Thanks @AndrewZWood ! Looks good now. Merging