Skip to content
Merged
Prev Previous commit
Next Next commit
refactor: TT-106 Use the ngOnDestroy to destroy subscriptions running…
… in the background when changing components and create test to validate the call to unsubscribe in the ngOnDestroy
  • Loading branch information
VanessaIniguezG authored and scastillo-jp committed Feb 17, 2021
commit b0c9e788f0d76c0d08fb1bcfb027f62b1ae8af42
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Observable, of } from 'rxjs';
import { Observable, of, Subscription } from 'rxjs';
import { LoadActiveEntry, EntryActionTypes, UpdateEntry } from './../../store/entry.actions';
import { ActivityManagementActionTypes } from './../../../activities-management/store/activity-management.actions';
import {waitForAsync, ComponentFixture, TestBed} from '@angular/core/testing';
Expand All @@ -16,7 +16,6 @@ import { formatDate } from '@angular/common';
import { NgxMaterialTimepickerModule } from 'ngx-material-timepicker';
import * as moment from 'moment';
import { DATE_FORMAT_YEAR } from 'src/environments/environment';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';

describe('EntryFieldsComponent', () => {
type Merged = TechnologyState & ProjectState;
Expand All @@ -26,7 +25,6 @@ describe('EntryFieldsComponent', () => {
let mockTechnologySelector;
let mockProjectsSelector;
let entryForm;
let featureManagerService: FeatureManagerService;
const actionSub: ActionsSubject = new ActionsSubject();
const toastrServiceStub = {
error: (message?: string, title?: string, override?: Partial<IndividualConfig>) => { },
Expand Down Expand Up @@ -117,7 +115,6 @@ describe('EntryFieldsComponent', () => {
entryForm = TestBed.inject(FormBuilder);
mockTechnologySelector = store.overrideSelector(allTechnologies, state.technologies);
mockProjectsSelector = store.overrideSelector(getCustomerProjects, state.projects);
// featureManagerService = TestBed.inject(FeatureManagerService);
}));

beforeEach(() => {
Expand Down Expand Up @@ -153,16 +150,6 @@ describe('EntryFieldsComponent', () => {
expect(component.selectedTechnologies).toEqual([]);
});

// const exponentialGrowth = [true, false];
// exponentialGrowth.map((toggleValue) => {
// it(`when FeatureToggle is ${toggleValue} should return ${toggleValue}`, () => {
// spyOn(featureManagerService, 'isToggleEnabled').and.returnValue(of(toggleValue));
// const isFeatureToggleActivated: Promise<boolean> = component.isFeatureToggleActivated();
// expect(featureManagerService.isToggleEnabled).toHaveBeenCalled();
// isFeatureToggleActivated.then((value) => expect(value).toEqual(toggleValue));
// });
// });

it('displays error message when the date selected is in the future', () => {
const mockEntry = { ...entry,
start_date : moment().format(DATE_FORMAT_YEAR),
Expand Down Expand Up @@ -424,4 +411,19 @@ describe('EntryFieldsComponent', () => {

expect(component.selectedTechnologies).toBe(initialTechnologies);
});

it('calls unsubscribe on ngDestroy', () => {
component.loadActivitiesSubscription = new Subscription();
component.loadActiveEntrySubscription = new Subscription();
component.actionSetDateSubscription = new Subscription();
spyOn(component.loadActivitiesSubscription, 'unsubscribe');
spyOn(component.loadActiveEntrySubscription, 'unsubscribe');
spyOn(component.actionSetDateSubscription, 'unsubscribe');

component.ngOnDestroy();

expect(component.loadActivitiesSubscription.unsubscribe).toHaveBeenCalled();
expect(component.loadActiveEntrySubscription.unsubscribe).toHaveBeenCalled();
expect(component.actionSetDateSubscription.unsubscribe).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { Activity, NewEntry } from '../../../shared/models';
import { ProjectState } from '../../../customer-management/components/projects/components/store/project.reducer';
import { TechnologyState } from '../../../shared/store/technology.reducers';
import { ActivityState, LoadActivities } from '../../../activities-management/store';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';
import * as entryActions from '../../store/entry.actions';
import { get } from 'lodash';
import * as moment from 'moment';
Expand Down Expand Up @@ -38,7 +37,6 @@ export class EntryFieldsComponent implements OnInit, OnDestroy {
actionSetDateSubscription: Subscription;

constructor(
private featureManagerService: FeatureManagerService,
private formBuilder: FormBuilder,
private store: Store<Merged>,
private actionsSubject$: ActionsSubject,
Expand Down Expand Up @@ -175,17 +173,9 @@ export class EntryFieldsComponent implements OnInit, OnDestroy {
this.store.dispatch(new entryActions.UpdateEntryRunning({ ...this.newData, technologies: $event }));
}


ngOnDestroy(): void {
if (this.isFeatureToggleActivated()) {
this.loadActivitiesSubscription.unsubscribe();
this.loadActiveEntrySubscription.unsubscribe();
this.actionSetDateSubscription.unsubscribe();
}
}

isFeatureToggleActivated() {
return this.featureManagerService.isToggleEnabledForUser('exponential-growth').pipe(
map((enabled) => enabled));
ngOnDestroy(): void {
this.loadActivitiesSubscription.unsubscribe();
this.loadActiveEntrySubscription.unsubscribe();
this.actionSetDateSubscription.unsubscribe();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@ import { getCustomerProjects } from '../../../customer-management/components/pro
import { FilterProjectPipe } from '../../../shared/pipes';
import { UpdateEntryRunning } from '../../store/entry.actions';
import { ProjectListHoverComponent } from './project-list-hover.component';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';

describe('ProjectListHoverComponent', () => {
let component: ProjectListHoverComponent;
let fixture: ComponentFixture<ProjectListHoverComponent>;
let store: MockStore<ProjectState>;
let mockProjectsSelector;
let featureManagerService: FeatureManagerService;
const toastrServiceStub = {
error: (message?: string, title?: string, override?: Partial<IndividualConfig>) => { }
};
Expand Down Expand Up @@ -58,7 +56,6 @@ describe('ProjectListHoverComponent', () => {
fixture = TestBed.createComponent(ProjectListHoverComponent);
component = fixture.componentInstance;
fixture.detectChanges();
featureManagerService = TestBed.inject(FeatureManagerService);
});

it('should create', () => {
Expand Down Expand Up @@ -103,36 +100,19 @@ describe('ProjectListHoverComponent', () => {

it('calls unsubscribe on ngDestroy', () => {
component.updateEntrySubscription = new Subscription();
component.projectsSubscription = new Subscription();
component.activeEntrySubscription = new Subscription();
spyOn(component.updateEntrySubscription, 'unsubscribe');
spyOn(component.projectsSubscription, 'unsubscribe');
spyOn(component.activeEntrySubscription, 'unsubscribe');

component.ngOnDestroy();

expect(component.updateEntrySubscription.unsubscribe).toHaveBeenCalled();
expect(component.projectsSubscription.unsubscribe).toHaveBeenCalled();
expect(component.activeEntrySubscription.unsubscribe).toHaveBeenCalled();
});

// it('calls projectsSubscribe unsubscribe on ngDestroy', () => {
// component.projectsSubscription = new Subscription();
// spyOn(component.projectsSubscribe, 'unsubscribe');

// component.ngOnDestroy();

// expect(component.projectsSubscribe.unsubscribe).toHaveBeenCalledTimes(1);
// });

// fit('When Component is created, should call the feature toggle method', () => {
// // component.exponentialGrowth = true;
// component.projectsSubscribe = new Subscription();
// component.activeEntrySubscribe = new Subscription();
// spyOn(component.projectsSubscribe, 'unsubscribe');
// spyOn(component.activeEntrySubscribe, 'unsubscribe');

// component.ngOnDestroy();

// expect(component.projectsSubscribe.unsubscribe).toHaveBeenCalled();
// expect(component.activeEntrySubscribe.unsubscribe).toHaveBeenCalled();

// });

it('sets customer name and project name on setSelectedProject', () => {
spyOn(component.projectsForm, 'setValue');
component.activeEntry = { project_id : 'p1'};
Expand All @@ -141,61 +121,9 @@ describe('ProjectListHoverComponent', () => {
component.setSelectedProject();

expect(component.projectsForm.setValue)
.toHaveBeenCalledWith({ project_id: 'customer - xyz'});
.toHaveBeenCalledWith({ project_id: 'customer - xyz'});
});

var toggleValue = true;
fit(`when FeatureToggle is ${toggleValue} should return ${toggleValue}`, () => {
spyOn(featureManagerService, 'isToggleEnabledForUser').and.returnValue(of(toggleValue));
component.projectsSubscription = new Subscription();
component.activeEntrySubscription = new Subscription();

spyOn(component.projectsSubscription, 'unsubscribe');
spyOn(component.activeEntrySubscription, 'unsubscribe');

component.ngOnDestroy();

expect(component.projectsSubscription.unsubscribe).toHaveBeenCalled();
expect(component.activeEntrySubscription.unsubscribe).toHaveBeenCalled();
});

toggleValue = false;
fit(`when FeatureToggle is ${toggleValue} should return ${toggleValue}`, () => {
spyOn(component, 'isFeatureToggleActivated').and.returnValue(of(toggleValue));
component.projectsSubscription = new Subscription();
component.activeEntrySubscription = new Subscription();

spyOn(component.projectsSubscription, 'unsubscribe');
spyOn(component.activeEntrySubscription, 'unsubscribe');

component.ngOnDestroy();

expect(component.projectsSubscription.unsubscribe).not.toHaveBeenCalled();
expect(component.activeEntrySubscription.unsubscribe).not.toHaveBeenCalled();
});

// const exponentialGrowth = [true, false];
// exponentialGrowth.map((toggleValue) => {
// it(`when FeatureToggle is ${toggleValue} should return ${toggleValue}`, () => {
// spyOn(featureManagerService, 'isToggleEnabled').and.returnValue(of(toggleValue));
// const isFeatureToggleActivated: Promise<boolean> = component.isFeatureToggleActivated();
// expect(featureManagerService.isToggleEnabled).toHaveBeenCalled();
// isFeatureToggleActivated.then((value) => expect(value).toEqual(toggleValue));
// });
// });

// it('deleteProject, should dispatch DeleteProject action', () => {
// component.projectsSubscription = new Subscription();
// const subscription = spyOn(component.projectsSubscription, 'unsubscribe');

// component.idToDelete = project.id;
// spyOn(store, 'dispatch');
// component.deleteProject();
// component.ngOnDestroy();
// expect(subscription).toHaveBeenCalledTimes(1);
// expect(store.dispatch).toHaveBeenCalledTimes(1);
// expect(store.dispatch).toHaveBeenCalledWith(new DeleteProject(project.id));
// });
// TODO Fix this test since it is throwing this error
// Expected spy dispatch to have been called with:
// [CreateEntry({ payload: Object({ project_id: '1', start_date: '2020-07-27T22:30:26.743Z', timezone_offset: 300 }),
Expand All @@ -217,5 +145,4 @@ describe('ProjectListHoverComponent', () => {
// })
// );
// });

});
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import * as entryActions from '../../store/entry.actions';
import { getIsLoading, getProjects } from './../../../customer-management/components/projects/components/store/project.selectors';
import { EntryActionTypes } from './../../store/entry.actions';
import { getActiveTimeEntry } from './../../store/entry.selectors';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';
@Component({
selector: 'app-project-list-hover',
templateUrl: './project-list-hover.component.html',
Expand All @@ -28,8 +27,7 @@ export class ProjectListHoverComponent implements OnInit, OnDestroy {
projectsSubscription: Subscription;
activeEntrySubscription: Subscription;

constructor(private featureManagerService: FeatureManagerService,
private formBuilder: FormBuilder, private store: Store<ProjectState>,
constructor(private formBuilder: FormBuilder, private store: Store<ProjectState>,
private actionsSubject$: ActionsSubject, private toastrService: ToastrService) {
this.projectsForm = this.formBuilder.group({ project_id: null, });
this.isLoading$ = this.store.pipe(delay(0), select(getIsLoading));
Expand Down Expand Up @@ -78,7 +76,7 @@ export class ProjectListHoverComponent implements OnInit, OnDestroy {
if (project.id === this.activeEntry.project_id) {
this.projectsForm.setValue(
{ project_id: `${project.customer_name} - ${project.name}`, }
);
);
}
});
}
Expand Down Expand Up @@ -110,15 +108,8 @@ export class ProjectListHoverComponent implements OnInit, OnDestroy {
}

ngOnDestroy(): void {
if(this.isFeatureToggleActivated()){
this.projectsSubscription.unsubscribe();
this.activeEntrySubscription.unsubscribe();
}
this.projectsSubscription.unsubscribe();
this.activeEntrySubscription.unsubscribe();
this.updateEntrySubscription.unsubscribe();
}

isFeatureToggleActivated() {
return this.featureManagerService.isToggleEnabledForUser('exponential-growth').pipe(
map((enabled) => enabled === true ? true : false));
}
}
17 changes: 3 additions & 14 deletions src/app/modules/time-clock/pages/time-clock.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ import { AzureAdB2CService } from '../../login/services/azure.ad.b2c.service';
import { ActionsSubject } from '@ngrx/store';
import { EntryFieldsComponent } from '../components/entry-fields/entry-fields.component';
import { ToastrService } from 'ngx-toastr';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';

describe('TimeClockComponent', () => {
let component: TimeClockComponent;
let fixture: ComponentFixture<TimeClockComponent>;
let store: MockStore<ProjectState>;
let azureAdB2CService: AzureAdB2CService;
let featureManagerService: FeatureManagerService;
const actionSub: ActionsSubject = new ActionsSubject();

let injectedToastrService;
Expand Down Expand Up @@ -72,7 +70,6 @@ describe('TimeClockComponent', () => {
fixture.detectChanges();
azureAdB2CService = TestBed.inject(AzureAdB2CService);
injectedToastrService = TestBed.inject(ToastrService);
// featureManagerService = TestBed.inject(FeatureManagerService);
});

it('should be created', () => {
Expand All @@ -99,12 +96,14 @@ describe('TimeClockComponent', () => {
expect(component.reloadSummariesOnClockOut).toHaveBeenCalled();
});

it('unsubscribe clockOutSubscription onDestroy', () => {
it('unsubscribe clockOutSubscription, storeSubscription onDestroy', () => {
spyOn(component.clockOutSubscription, 'unsubscribe');
spyOn(component.storeSubscription, 'unsubscribe');

component.ngOnDestroy();

expect(component.clockOutSubscription.unsubscribe).toHaveBeenCalled();
expect(component.storeSubscription.unsubscribe).toHaveBeenCalled();
});

it('onInit checks if isLogin and gets the userName', () => {
Expand Down Expand Up @@ -146,14 +145,4 @@ describe('TimeClockComponent', () => {

expect(injectedToastrService.error).toHaveBeenCalled();
});

// const exponentialGrowth = [true, false];
// exponentialGrowth.map((toggleValue) => {
// it(`when FeatureToggle is ${toggleValue} should return true`, () => {
// spyOn(featureManagerService, 'isToggleEnabled').and.returnValue(of(toggleValue));
// const isFeatureToggleActivated: Promise<boolean> = component.isFeatureToggleActivated();
// expect(featureManagerService.isToggleEnabled).toHaveBeenCalled();
// isFeatureToggleActivated.then((value) => expect(value).toEqual(toggleValue));
// });
// });
});
18 changes: 5 additions & 13 deletions src/app/modules/time-clock/pages/time-clock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { EntryFieldsComponent } from '../components/entry-fields/entry-fields.co
import { Entry } from './../../shared/models/entry.model';
import { EntryActionTypes, LoadEntriesSummary, StopTimeEntryRunning } from './../store/entry.actions';
import { getActiveTimeEntry } from './../store/entry.selectors';
import { FeatureManagerService } from 'src/app/modules/shared/feature-toggles/feature-toggle-manager.service';
@Component({
selector: 'app-time-clock',
templateUrl: './time-clock.component.html',
Expand All @@ -26,15 +25,13 @@ export class TimeClockComponent implements OnInit, OnDestroy {
storeSubscription: Subscription;

constructor(
private featureManagerService: FeatureManagerService,
private azureAdB2CService: AzureAdB2CService,
private store: Store<Entry>,
private toastrService: ToastrService,
private actionsSubject$: ActionsSubject
) {}

ngOnInit(): void{

ngOnInit(): void {
this.username = this.azureAdB2CService.isLogin() ? this.azureAdB2CService.getName() : '';
this.storeSubscription = this.store.pipe(select(getActiveTimeEntry)).subscribe((activeTimeEntry) => {
this.activeTimeEntry = activeTimeEntry;
Expand Down Expand Up @@ -72,16 +69,11 @@ export class TimeClockComponent implements OnInit, OnDestroy {
}
}

ngOnDestroy(): void {
this.isFeatureToggleActivated() && this.storeSubscription.unsubscribe();
this.clockOutSubscription.unsubscribe();
this.storeSubscription.unsubscribe();
}

isFeatureToggleActivated() {
return this.featureManagerService.isToggleEnabledForUser('exponential-growth').pipe(
map((enabled) => enabled));
ngOnDestroy(): void {
this.clockOutSubscription.unsubscribe();
this.storeSubscription.unsubscribe();
}

}