Skip to content

Commit

Permalink
108588: Fixed browse by date tab's year dropdown always being empty
Browse files Browse the repository at this point in the history
- The data passed to the injector in BrowseByComponent was not updated by ngOnChanges
- Also refactored the injector logic to StartsWithLoaderComponent
  • Loading branch information
alexandrevryghem committed Feb 6, 2024
1 parent 6f51bd8 commit 6e29f30
Show file tree
Hide file tree
Showing 11 changed files with 167 additions and 130 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export class BrowseByMetadataComponent implements OnInit, OnChanges, OnDestroy {
* The list of StartsWith options
* Should be defined after ngOnInit is called!
*/
startsWithOptions;
startsWithOptions: (string | number)[];

/**
* The value we're browsing items for
Expand Down
3 changes: 2 additions & 1 deletion src/app/shared/browse-by/browse-by.component.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<ng-container *ngVar="(objects$ | async) as objects">
<h1 *ngIf="displayTitle">{{title | translate}}</h1>
<ng-container *ngComponentOutlet="getStartsWithComponent(); injector: objectInjector;"></ng-container>
<ds-starts-with-loader [paginationId]="paginationConfig?.id" [startsWithOptions]="startsWithOptions" [type]="type">
</ds-starts-with-loader>
<div *ngIf="objects?.hasSucceeded && !objects?.isLoading && objects?.payload?.page.length > 0" @fadeIn>
<div *ngIf="shouldDisplayResetButton$ |async" class="mb-2 reset">
<ds-themed-results-back-button [back]="back" [buttonLabel]="buttonLabel"></ds-themed-results-back-button>
Expand Down
57 changes: 11 additions & 46 deletions src/app/shared/browse-by/browse-by.component.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Component, EventEmitter, Injector, Input, OnDestroy, OnInit, Output } from '@angular/core';
import { Component, EventEmitter, Injector, Input, OnDestroy, OnInit, Output, OnChanges } from '@angular/core';
import { RemoteData } from '../../core/data/remote-data';
import { PaginatedList } from '../../core/data/paginated-list.model';
import { PaginationComponentOptions } from '../pagination/pagination-component-options.model';
import { SortDirection, SortOptions } from '../../core/cache/models/sort-options.model';
import { fadeIn, fadeInOut } from '../animations/fade';
import { BehaviorSubject, combineLatest as observableCombineLatest, Observable, Subscription } from 'rxjs';
import { ListableObject } from '../object-collection/shared/listable-object.model';
import { getStartsWithComponent, StartsWithType } from '../starts-with/starts-with-decorator';
import { StartsWithType } from '../starts-with/starts-with-decorator';
import { PaginationService } from '../../core/pagination/pagination.service';
import { ViewMode } from '../../core/shared/view-mode.model';
import { RouteService } from '../../core/services/route.service';
Expand All @@ -26,7 +26,7 @@ import { TranslateService } from '@ngx-translate/core';
/**
* Component to display a browse-by page for any ListableObject
*/
export class BrowseByComponent implements OnInit, OnDestroy {
export class BrowseByComponent implements OnInit, OnChanges, OnDestroy {

/**
* ViewMode that should be passed to {@link ListableObjectComponentLoaderComponent}.
Expand Down Expand Up @@ -67,7 +67,7 @@ export class BrowseByComponent implements OnInit, OnDestroy {
/**
* The list of options to render for the StartsWith component
*/
@Input() startsWithOptions = [];
@Input() startsWithOptions: (string | number)[] = [];

/**
* Whether or not the pagination should be rendered as simple previous and next buttons instead of the normal pagination
Expand Down Expand Up @@ -99,16 +99,6 @@ export class BrowseByComponent implements OnInit, OnDestroy {
*/
@Output() sortDirectionChange = new EventEmitter<SortDirection>();

/**
* An object injector used to inject the startsWithOptions to the switchable StartsWith component
*/
objectInjector: Injector;

/**
* Declare SortDirection enumeration to use it in the template
*/
public sortDirections = SortDirection;

/**
* Observable that tracks if the back button should be displayed based on the path parameters
*/
Expand Down Expand Up @@ -142,7 +132,7 @@ export class BrowseByComponent implements OnInit, OnDestroy {
*/
back = () => {
const page = +this.previousPage$.value > 1 ? +this.previousPage$.value : 1;
this.paginationService.updateRoute(this.paginationConfig.id, {page: page}, {[this.paginationConfig.id + '.return']: null, value: null, startsWith: null});
this.paginationService.updateRoute(this.paginationConfig.id, { page: page }, { [this.paginationConfig.id + '.return']: null, value: null, startsWith: null });
};

/**
Expand All @@ -159,44 +149,19 @@ export class BrowseByComponent implements OnInit, OnDestroy {
this.next.emit(true);
}

/**
* Change the page size
* @param size
*/
doPageSizeChange(size) {
this.paginationService.updateRoute(this.paginationConfig.id,{pageSize: size});
}

/**
* Change the sort direction
* @param direction
*/
doSortDirectionChange(direction) {
this.paginationService.updateRoute(this.paginationConfig.id,{sortDirection: direction});
}

/**
* Get the switchable StartsWith component dependant on the type
*/
getStartsWithComponent() {
return getStartsWithComponent(this.type);
}

ngOnInit(): void {
this.objectInjector = Injector.create({
providers: [
{ provide: 'startsWithOptions', useFactory: () => (this.startsWithOptions), deps:[] },
{ provide: 'paginationId', useFactory: () => (this.paginationConfig?.id), deps:[] }
],
parent: this.injector
});

const startsWith$ = this.routeService.getQueryParameterValue('startsWith');
const value$ = this.routeService.getQueryParameterValue('value');

this.shouldDisplayResetButton$ = observableCombineLatest([startsWith$, value$]).pipe(
map(([startsWith, value]) => hasValue(startsWith) || hasValue(value))
);
}

ngOnChanges(): void {
if (this.sub) {
this.sub.unsubscribe();
}
this.sub = this.routeService.getQueryParameterValue(this.paginationConfig.id + '.return').subscribe(this.previousPage$);
}

Expand Down
3 changes: 2 additions & 1 deletion src/app/shared/shared.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ import {
import { BitstreamListItemComponent } from './object-list/bitstream-list-item/bitstream-list-item.component';
import { NgxPaginationModule } from 'ngx-pagination';
import { DynamicComponentLoaderDirective } from './abstract-component-loader/dynamic-component-loader.directive';
import { StartsWithLoaderComponent } from './starts-with/starts-with-loader.component';

const MODULES = [
CommonModule,
Expand Down Expand Up @@ -374,7 +375,7 @@ const COMPONENTS = [
ThemedStatusBadgeComponent,
BadgesComponent,
ThemedBadgesComponent,

StartsWithLoaderComponent,
ItemSelectComponent,
CollectionSelectComponent,
MetadataRepresentationLoaderComponent,
Expand Down
37 changes: 14 additions & 23 deletions src/app/shared/starts-with/date/starts-with-date.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ import { CommonModule } from '@angular/common';
import { RouterTestingModule } from '@angular/router/testing';
import { TranslateModule } from '@ngx-translate/core';
import { NgbModule } from '@ng-bootstrap/ng-bootstrap';
import { ActivatedRoute, NavigationExtras, Router } from '@angular/router';
import { ActivatedRoute, Router } from '@angular/router';
import { NO_ERRORS_SCHEMA } from '@angular/core';
import { of as observableOf } from 'rxjs';
import { By } from '@angular/platform-browser';
import { StartsWithDateComponent } from './starts-with-date.component';
import { ActivatedRouteStub } from '../../testing/active-router.stub';
Expand All @@ -17,29 +16,25 @@ import { PaginationServiceStub } from '../../testing/pagination-service.stub';
describe('StartsWithDateComponent', () => {
let comp: StartsWithDateComponent;
let fixture: ComponentFixture<StartsWithDateComponent>;
let route: ActivatedRoute;
let router: Router;
let paginationService;

const options = [2019, 2018, 2017, 2016, 2015];
let route: ActivatedRouteStub;
let paginationService: PaginationServiceStub;
let router: RouterStub;

const activatedRouteStub = Object.assign(new ActivatedRouteStub(), {
params: observableOf({}),
queryParams: observableOf({})
});
const options = [2019, 2018, 2017, 2016, 2015];

paginationService = new PaginationServiceStub();
beforeEach(waitForAsync(async () => {
route = new ActivatedRouteStub();
router = new RouterStub();
paginationService = new PaginationServiceStub();

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
await TestBed.configureTestingModule({
imports: [CommonModule, RouterTestingModule.withRoutes([]), TranslateModule.forRoot(), NgbModule],
declarations: [StartsWithDateComponent, EnumKeysPipe],
providers: [
{ provide: 'startsWithOptions', useValue: options },
{ provide: 'paginationId', useValue: 'page-id' },
{ provide: ActivatedRoute, useValue: activatedRouteStub },
{ provide: ActivatedRoute, useValue: route },
{ provide: PaginationService, useValue: paginationService },
{ provide: Router, useValue: new RouterStub() }
{ provide: Router, useValue: router },
],
schemas: [NO_ERRORS_SCHEMA]
}).compileComponents();
Expand All @@ -48,9 +43,9 @@ describe('StartsWithDateComponent', () => {
beforeEach(() => {
fixture = TestBed.createComponent(StartsWithDateComponent);
comp = fixture.componentInstance;
comp.paginationId = 'page-id';
comp.startsWithOptions = options;
fixture.detectChanges();
route = (comp as any).route;
router = (comp as any).router;
});

it('should create a FormGroup containing a startsWith FormControl', () => {
Expand Down Expand Up @@ -156,10 +151,6 @@ describe('StartsWithDateComponent', () => {
describe('when filling in the input form', () => {
let form;
const expectedValue = '2015';
const extras: NavigationExtras = {
queryParams: Object.assign({ startsWith: expectedValue }),
queryParamsHandling: 'merge'
};

beforeEach(() => {
form = fixture.debugElement.query(By.css('form'));
Expand Down
22 changes: 2 additions & 20 deletions src/app/shared/starts-with/date/starts-with-date.component.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { Component, Inject } from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';

import { Component, OnInit } from '@angular/core';
import { renderStartsWithFor, StartsWithType } from '../starts-with-decorator';
import { StartsWithAbstractComponent } from '../starts-with-abstract.component';
import { hasValue } from '../../empty.util';
import { PaginationService } from '../../../core/pagination/pagination.service';

/**
* A switchable component rendering StartsWith options for the type "Date".
Expand All @@ -16,7 +13,7 @@ import { PaginationService } from '../../../core/pagination/pagination.service';
templateUrl: './starts-with-date.component.html'
})
@renderStartsWithFor(StartsWithType.date)
export class StartsWithDateComponent extends StartsWithAbstractComponent {
export class StartsWithDateComponent extends StartsWithAbstractComponent implements OnInit {

/**
* A list of options for months to select from
Expand All @@ -33,14 +30,6 @@ export class StartsWithDateComponent extends StartsWithAbstractComponent {
*/
startsWithYear: number;

public constructor(@Inject('startsWithOptions') public startsWithOptions: any[],
@Inject('paginationId') public paginationId: string,
protected paginationService: PaginationService,
protected route: ActivatedRoute,
protected router: Router) {
super(startsWithOptions, paginationId, paginationService, route, router);
}

ngOnInit() {
this.monthOptions = [
'none',
Expand Down Expand Up @@ -133,13 +122,6 @@ export class StartsWithDateComponent extends StartsWithAbstractComponent {
}
}

/**
* Get startsWithYear as a number;
*/
getStartsWithYear() {
return this.startsWithYear;
}

/**
* Get startsWithMonth as a number;
*/
Expand Down
31 changes: 15 additions & 16 deletions src/app/shared/starts-with/starts-with-abstract.component.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Component, Inject, OnDestroy, OnInit } from '@angular/core';
import { Component, OnDestroy, OnInit, Input } from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';
import { Subscription } from 'rxjs';
import { UntypedFormControl, UntypedFormGroup } from '@angular/forms';
import { hasValue } from '../empty.util';
import { PaginationService } from '../../core/pagination/pagination.service';
import { StartsWithType } from './starts-with-decorator';

/**
* An abstract component to render StartsWith options
Expand All @@ -13,6 +14,13 @@ import { PaginationService } from '../../core/pagination/pagination.service';
template: ''
})
export abstract class StartsWithAbstractComponent implements OnInit, OnDestroy {

@Input() paginationId: string;

@Input() startsWithOptions: (string | number)[];

@Input() type: StartsWithType;

/**
* The currently selected startsWith in string format
*/
Expand All @@ -28,11 +36,11 @@ export abstract class StartsWithAbstractComponent implements OnInit, OnDestroy {
*/
subs: Subscription[] = [];

public constructor(@Inject('startsWithOptions') public startsWithOptions: any[],
@Inject('paginationId') public paginationId: string,
protected paginationService: PaginationService,
protected route: ActivatedRoute,
protected router: Router) {
public constructor(
protected paginationService: PaginationService,
protected route: ActivatedRoute,
protected router: Router,
) {
}

ngOnInit(): void {
Expand All @@ -55,15 +63,6 @@ export abstract class StartsWithAbstractComponent implements OnInit, OnDestroy {
return this.startsWith;
}

/**
* Set the startsWith by event
* @param event
*/
setStartsWithEvent(event: Event) {
this.startsWith = (event.target as HTMLInputElement).value;
this.setStartsWithParam();
}

/**
* Set the startsWith by string
* @param startsWith
Expand All @@ -82,7 +81,7 @@ export abstract class StartsWithAbstractComponent implements OnInit, OnDestroy {
if (resetPage) {
this.paginationService.updateRoute(this.paginationId, {page: 1}, { startsWith: this.startsWith });
} else {
this.router.navigate([], {
void this.router.navigate([], {
queryParams: Object.assign({ startsWith: this.startsWith }),
queryParamsHandling: 'merge'
});
Expand Down
Loading

0 comments on commit 6e29f30

Please sign in to comment.