From 8ab206449e0ff4173485c263c43330b4ac154328 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Thu, 27 Apr 2023 15:36:33 -0600 Subject: [PATCH 1/6] checking selected conact before setting title --- webapp/src/ts/effects/contacts.effects.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/webapp/src/ts/effects/contacts.effects.ts b/webapp/src/ts/effects/contacts.effects.ts index 9abc91d75a3..6068e1017c6 100644 --- a/webapp/src/ts/effects/contacts.effects.ts +++ b/webapp/src/ts/effects/contacts.effects.ts @@ -59,6 +59,7 @@ export class ContactsEffects { const loadContact = this .loadContact(id) + .then(() => this.setTitle(id)) .then(() => this.loadChildren(id, userFacilityId)) .then(() => this.loadReports(id, forms)) .then(() => this.loadTargetDoc(id)) @@ -82,12 +83,16 @@ export class ContactsEffects { ); }, { dispatch: false }); - private setTitle(selected) { - const routeSnapshot = this.routeSnapshotService.get(); - const deceasedTitle = routeSnapshot?.data?.name === 'contacts.deceased' - ? this.translateService.instant('contact.deceased.title') : null; - const title = deceasedTitle || selected.type?.name_key || 'contact.profile'; - this.globalActions.setTitle(this.translateService.instant(title)); + private setTitle(contactId) { + return this + .verifySelectedContactNotChanged(contactId) + .then(() => { + const routeSnapshot = this.routeSnapshotService.get(); + const deceasedTitle = routeSnapshot?.data?.name === 'contacts.deceased' + ? this.translateService.instant('contact.deceased.title') : null; + const title = deceasedTitle || this.selectedContact.type?.name_key || 'contact.profile'; + this.globalActions.setTitle(this.translateService.instant(title)); + }); } private loadContact(id) { @@ -96,7 +101,6 @@ export class ContactsEffects { .then(model => { this.globalActions.settingSelected(); this.contactsActions.setSelectedContact(model); - this.setTitle(model); }); } From efc66e2458e3fa51e02a738b766780fd30911094 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Thu, 27 Apr 2023 17:53:22 -0600 Subject: [PATCH 2/6] catching contact id to fetch for validations --- webapp/src/ts/actions/contacts.ts | 5 ++++ webapp/src/ts/effects/contacts.effects.ts | 35 +++++++++++++++++------ webapp/src/ts/reducers/contacts.ts | 2 ++ webapp/src/ts/selectors/index.ts | 1 + 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/webapp/src/ts/actions/contacts.ts b/webapp/src/ts/actions/contacts.ts index 36b715e26e4..e7127636905 100644 --- a/webapp/src/ts/actions/contacts.ts +++ b/webapp/src/ts/actions/contacts.ts @@ -9,6 +9,7 @@ export const Actions = { selectContact: createMultiValueAction('SELECT_CONTACT'), setSelectedContact: createSingleValueAction('SET_SELECTED_CONTACT', 'selected'), setContactsLoadingSummary: createSingleValueAction('SET_CONTACT_LOADING_SUMMARY', 'value'), + setContactIdToFetch: createSingleValueAction('SET_CONTACT_ID_TO_FETCH', 'id'), setLoadingSelectedContact: createAction('SET_LOADING_SELECTED_CONTACT'), receiveSelectedContactChildren: createSingleValueAction('RECEIVE_SELECTED_CONTACT_CHILDREN', 'children'), receiveSelectedContactReports: createSingleValueAction('RECEIVE_SELECTED_CONTACT_REPORTS', 'reports'), @@ -22,6 +23,10 @@ export class ContactsActions { private store: Store ) {} + setContactIdToFetch(id) { + return this.store.dispatch(Actions.setContactIdToFetch(id)); + } + updateContactsList(contacts) { return this.store.dispatch(Actions.updateContactsList(contacts)); } diff --git a/webapp/src/ts/effects/contacts.effects.ts b/webapp/src/ts/effects/contacts.effects.ts index 6068e1017c6..d2a3f9b2334 100644 --- a/webapp/src/ts/effects/contacts.effects.ts +++ b/webapp/src/ts/effects/contacts.effects.ts @@ -1,7 +1,7 @@ import { Injectable } from '@angular/core'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import { Store, select } from '@ngrx/store'; -import { of } from 'rxjs'; +import { combineLatest, of } from 'rxjs'; import { exhaustMap, withLatestFrom } from 'rxjs/operators'; import { Actions as ContactActionList, ContactsActions } from '@mm-actions/contacts'; @@ -16,10 +16,11 @@ import { TranslateService } from '@mm-services/translate.service'; @Injectable() export class ContactsEffects { - private contactsActions; - private globalActions; + private contactsActions: ContactsActions; + private globalActions: GlobalActions; private selectedContact; + private contactIdToFetch; constructor( private actions$: Actions, @@ -34,9 +35,13 @@ export class ContactsEffects { this.contactsActions = new ContactsActions(store); this.globalActions = new GlobalActions(store); - this.store - .select(Selectors.getSelectedContact) - .subscribe(selectedContact => this.selectedContact = selectedContact); + combineLatest( + this.store.select(Selectors.getSelectedContact), + this.store.select(Selectors.getContactIdToFetch), + ).subscribe(([ selectedContact, contactIdToFetch ]) => { + this.selectedContact = selectedContact; + this.contactIdToFetch = contactIdToFetch; + }); } selectContact = createEffect(() => { @@ -65,6 +70,12 @@ export class ContactsEffects { .then(() => this.loadTargetDoc(id)) .then(() => this.loadContactSummary(id)) .then(() => this.loadTasks(id)) + .then(() => { + if (id === this.contactIdToFetch) { + // Clear ID after loading contact + this.contactsActions.setContactIdToFetch(null); + } + }) .catch(err => { // If the selected contact has changed, just stop loading this one if (err.code === 'SELECTED_CONTACT_CHANGED') { @@ -75,6 +86,7 @@ export class ContactsEffects { } console.error('Error selecting contact', err); this.globalActions.unsetSelected(); + this.contactsActions.setContactIdToFetch(null); return of(this.contactsActions.setSelectedContact(null)); }); @@ -96,16 +108,21 @@ export class ContactsEffects { } private loadContact(id) { + this.contactsActions.setContactIdToFetch(id); return this.contactViewModelGeneratorService .getContact(id, { merge: false }) .then(model => { - this.globalActions.settingSelected(); - this.contactsActions.setSelectedContact(model); + return this + .verifySelectedContactNotChanged(model._id) + .then(() => { + this.globalActions.settingSelected(); + this.contactsActions.setSelectedContact(model); + }); }); } private verifySelectedContactNotChanged(id) { - return this.selectedContact?._id !== id ? Promise.reject({code: 'SELECTED_CONTACT_CHANGED'}) : Promise.resolve(); + return this.contactIdToFetch !== id ? Promise.reject({code: 'SELECTED_CONTACT_CHANGED'}) : Promise.resolve(); } private loadChildren(contactId, userFacilityId) { diff --git a/webapp/src/ts/reducers/contacts.ts b/webapp/src/ts/reducers/contacts.ts index 35a55ccaf5b..9fc5fa224e6 100644 --- a/webapp/src/ts/reducers/contacts.ts +++ b/webapp/src/ts/reducers/contacts.ts @@ -7,6 +7,7 @@ import { Actions } from '@mm-actions/contacts'; const initialState = { contacts: [], contactsById: new Map(), + contactIdToFetch: null, selected: null, filters: {}, loadingSelectedChildren: false, @@ -140,6 +141,7 @@ const receiveSelectedContactTargetDoc = (state, targetDoc) => { const _contactsReducer = createReducer( initialState, + on(Actions.setContactIdToFetch, (state, { payload: { id } }) => ({ ...state, contactIdToFetch: id })), on(Actions.updateContactsList, (state, { payload: { contacts } }) => updateContacts(state, contacts)), on(Actions.resetContactsList, (state) => ({ ...state, contacts: [], contactsById: new Map() })), on(Actions.removeContactFromList, (state, { payload: { contact } }) => removeContact(state, contact)), diff --git a/webapp/src/ts/selectors/index.ts b/webapp/src/ts/selectors/index.ts index af6cb088c09..7fcde3cf8c3 100644 --- a/webapp/src/ts/selectors/index.ts +++ b/webapp/src/ts/selectors/index.ts @@ -80,6 +80,7 @@ export const Selectors = { contactListContains: createSelector(getContactsState, (contactsState) => { return (id) => contactsState.contactsById.has(id); }), + getContactIdToFetch: createSelector(getContactsState, (contactState) => contactState.contactIdToFetch), getSelectedContact: createSelector(getContactsState, (contactState) => contactState.selected), getSelectedContactDoc: createSelector(getContactsState, (contactState) => contactState.selected?.doc), getSelectedContactSummary: createSelector(getContactsState, (contactState) => contactState.selected?.summary), From 4a4d9abe96e3b916265b9b5512b97118532b28c4 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Thu, 27 Apr 2023 18:09:20 -0600 Subject: [PATCH 3/6] catching contact id to fetch for validations --- webapp/src/ts/modules/contacts/contacts-content.component.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/webapp/src/ts/modules/contacts/contacts-content.component.ts b/webapp/src/ts/modules/contacts/contacts-content.component.ts index f6ef0bb9cad..ff268107fff 100644 --- a/webapp/src/ts/modules/contacts/contacts-content.component.ts +++ b/webapp/src/ts/modules/contacts/contacts-content.component.ts @@ -96,6 +96,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy { ngOnDestroy() { this.subscription.unsubscribe(); + this.contactsActions.setContactIdToFetch(null); this.contactsActions.setSelectedContact(null); this.globalActions.setRightActionBar({}); } @@ -206,6 +207,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy { $('.tooltip').remove(); } else { + this.contactsActions.setContactIdToFetch(null); this.contactsActions.setSelectedContact(null); this.globalActions.unsetSelected(); } From b8b925f68c7cda634fc169b7d13e6766cc3e5ce4 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Thu, 11 May 2023 16:51:42 +0700 Subject: [PATCH 4/6] fixing unit tests --- .../karma/ts/effects/contacts.effects.spec.ts | 7 +++ .../contacts-content.component.spec.ts | 48 +++++++++++++-- .../tests/karma/ts/reducers/contacts.spec.ts | 61 +++++++++++++++++++ webapp/tests/karma/ts/selectors/index.spec.ts | 5 ++ 4 files changed, 115 insertions(+), 6 deletions(-) diff --git a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts index 6dad4a3d91c..b51a1c07bcd 100644 --- a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts +++ b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts @@ -79,9 +79,11 @@ describe('Contacts effects', () => { const simulateContactsReducer = () => { let selectedContact = null; + let contactIdToFetch = null; const refreshState = () => { store.overrideSelector(Selectors.getSelectedContact, selectedContact); + store.overrideSelector(Selectors.getContactIdToFetch, contactIdToFetch); store.refreshState(); }; refreshState(); @@ -116,6 +118,11 @@ describe('Contacts effects', () => { selectedContact = { ...selectedContact, summary }; refreshState(); }); + + sinon.stub(ContactsActions.prototype, 'setContactIdToFetch').callsFake(id => { + contactIdToFetch = id; + refreshState(); + }); }; beforeEach(() => { diff --git a/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts b/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts index 52d114503e2..f301b0b3ad7 100644 --- a/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts +++ b/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts @@ -81,7 +81,8 @@ describe('Contacts content component', () => { }; globalActions = { setRightActionBar: sinon.spy(GlobalActions.prototype, 'setRightActionBar'), - updateRightActionBar: sinon.spy(GlobalActions.prototype, 'updateRightActionBar') + updateRightActionBar: sinon.spy(GlobalActions.prototype, 'updateRightActionBar'), + unsetSelected: sinon.spy(GlobalActions.prototype, 'unsetSelected'), }; mutingTransition = { isUnmuteForm: sinon.stub() }; contactMutedService = { getMuted: sinon.stub() }; @@ -105,7 +106,7 @@ describe('Contacts content component', () => { { selector: Selectors.getSelectedContactChildren, value: null }, { selector: Selectors.getFilters, value: {} }, ]; - activatedRoute = { params: of({ id: 'load contact' }), snapshot: { params: { id: 'load contact'} } }; + activatedRoute = { params: of({}), snapshot: { params: {} } }; router = { navigate: sinon.stub() }; responsiveService = { isMobile: sinon.stub() }; @@ -155,6 +156,23 @@ describe('Contacts content component', () => { expect(component).to.exist; }); + it('ngOnDestroy() should unsubscribe from observables and reset state', () => { + const unsubscribeSpy = sinon.spy(component.subscription, 'unsubscribe'); + const setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); + const setSelectedContactStub = sinon.stub(ContactsActions.prototype, 'setSelectedContact'); + sinon.resetHistory(); + + component.ngOnDestroy(); + + expect(unsubscribeSpy.calledOnce).to.be.true; + expect(setContactIdToFetchStub.calledOnce).to.be.true; + expect(setContactIdToFetchStub.args[0][0]).to.equal(null); + expect(setSelectedContactStub.calledOnce).to.be.true; + expect(setSelectedContactStub.args[0][0]).to.equal(null); + expect(globalActions.setRightActionBar.calledOnce).to.be.true; + expect(globalActions.setRightActionBar.args[0][0]).to.deep.equal({}); + }); + describe('load the user home place on mobile', () => { it(`should not load the user's home place when on mobile`, fakeAsync(() => { const selectContact = sinon.stub(ContactsActions.prototype, 'selectContact'); @@ -172,11 +190,14 @@ describe('Contacts content component', () => { it(`should not load the user's home place when a param id is set`, fakeAsync(() => { const selectContact = sinon.stub(ContactsActions.prototype, 'selectContact'); store.overrideSelector(Selectors.getUserFacilityId, 'homeplace'); + activatedRoute.params = of({ id: 'contact-1234' }); + activatedRoute.snapshot.params = { id: 'contact-1234' }; + component.ngOnInit(); flush(); - expect(selectContact.callCount).to.equal(1); - expect(selectContact.args[0][0]).to.equal('load contact'); + expect(selectContact.calledOnce).to.be.true; + expect(selectContact.args[0][0]).to.equal('contact-1234'); })); it(`should not load the user's home place when a search term exists`, fakeAsync(() => { @@ -186,8 +207,7 @@ describe('Contacts content component', () => { component.ngOnInit(); flush(); - expect(selectContact.callCount).to.equal(1); - expect(selectContact.args[0][0]).to.equal('load contact'); + expect(selectContact.notCalled).to.be.true; })); it(`should load the user's home place when a param id not set and no search term exists`, fakeAsync(() => { @@ -203,6 +223,22 @@ describe('Contacts content component', () => { expect(selectContact.args[0][0]).to.equal('homeplace'); })); + it('should unset selected contact when a param id not set and no search term exists', fakeAsync(() => { + const setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); + const setSelectedContactStub = sinon.stub(ContactsActions.prototype, 'setSelectedContact'); + store.overrideSelector(Selectors.getFilters, undefined); + sinon.resetHistory(); + + component.ngOnInit(); + flush(); + + expect(globalActions.unsetSelected.calledOnce).to.be.true; + expect(setContactIdToFetchStub.calledOnce).to.be.true; + expect(setContactIdToFetchStub.args[0][0]).to.equal(null); + expect(setSelectedContactStub.calledOnce).to.be.true; + expect(setSelectedContactStub.args[0][0]).to.equal(null); + })); + describe('Change feed process', () => { let change; diff --git a/webapp/tests/karma/ts/reducers/contacts.spec.ts b/webapp/tests/karma/ts/reducers/contacts.spec.ts index 03f3ceea291..b93b2f19cda 100644 --- a/webapp/tests/karma/ts/reducers/contacts.spec.ts +++ b/webapp/tests/karma/ts/reducers/contacts.spec.ts @@ -11,6 +11,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], + contactIdToFetch: null, filters: {}, loadingSummary: false, }; @@ -22,6 +23,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], + contactIdToFetch: null, filters: {}, loadingSummary: true, }); @@ -31,6 +33,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], + contactIdToFetch: null, filters: {}, loadingSummary: false, }); @@ -62,6 +65,7 @@ describe('Contacts Reducer', () => { ]), filters: {}, selected: null, + contactIdToFetch: null, loadingSelectedChildren: false, loadingSelectedReports: false, loadingSummary: false, @@ -340,6 +344,7 @@ describe('Contacts Reducer', () => { contactsById: new Map(), filters: {}, selected: { _id: 'selected_contact', some: 'data' }, + contactIdToFetch: null, loadingSummary: false, }); }); @@ -444,6 +449,7 @@ describe('Contacts Reducer', () => { selected: { summary: { some: 'summary' } }, + contactIdToFetch: null, loadingSummary: false, }); }); @@ -496,6 +502,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', children: [{ _id: 'child-1' }] }, + contactIdToFetch: null, loadingSummary: false, loadingSelectedChildren: false, }); @@ -520,6 +527,7 @@ describe('Contacts Reducer', () => { { _id: 'child-2' } ] }, + contactIdToFetch: null, loadingSummary: false, loadingSelectedChildren: false, }); @@ -541,6 +549,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', reports: [{ _id: 'report-1' }] }, + contactIdToFetch: null, loadingSummary: false, loadingSelectedReports: false }); @@ -565,6 +574,7 @@ describe('Contacts Reducer', () => { { _id: 'report-2' } ] }, + contactIdToFetch: null, loadingSelectedReports: false, loadingSummary: false, }); @@ -633,6 +643,7 @@ describe('Contacts Reducer', () => { { forId: 'contact-3' } ] }, + contactIdToFetch: null, loadingSummary: false, }); }); @@ -660,6 +671,7 @@ describe('Contacts Reducer', () => { { forId: 'contact-1' } ] }, + contactIdToFetch: null, loadingSummary: false, }); }); @@ -680,6 +692,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', targetDoc: { _id: 'doc-1' } }, + contactIdToFetch: null, loadingSummary: false, }); }); @@ -701,6 +714,54 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', targetDoc: { _id: 'doc-2' } }, + contactIdToFetch: null, + loadingSummary: false, + }); + }); + }); + + describe('setContactIdToFetch', () => { + it('should set contactIdToFetch in the state', () => { + state.contactIdToFetch = null; + + const newState = contactsReducer(state, Actions.setContactIdToFetch('selected_contact_1')); + + expect(newState).to.deep.equal({ + contacts: [], + contactsById: new Map(), + filters: {}, + selected: [], + contactIdToFetch: 'selected_contact_1', + loadingSummary: false, + }); + }); + + it('should update contactIdToFetch in the state', () => { + state.contactIdToFetch = 'selected_contact_1'; + + const newState = contactsReducer(state, Actions.setContactIdToFetch('selected_contact_2')); + + expect(newState).to.deep.equal({ + contacts: [], + contactsById: new Map(), + filters: {}, + selected: [], + contactIdToFetch: 'selected_contact_2', + loadingSummary: false, + }); + }); + + it('should unset contactIdToFetch in the state', () => { + state.contactIdToFetch = 'selected_contact_1'; + + const newState = contactsReducer(state, Actions.setContactIdToFetch(null)); + + expect(newState).to.deep.equal({ + contacts: [], + contactsById: new Map(), + filters: {}, + selected: [], + contactIdToFetch: null, loadingSummary: false, }); }); diff --git a/webapp/tests/karma/ts/selectors/index.spec.ts b/webapp/tests/karma/ts/selectors/index.spec.ts index 107bf6d4065..bef8b9d674f 100644 --- a/webapp/tests/karma/ts/selectors/index.spec.ts +++ b/webapp/tests/karma/ts/selectors/index.spec.ts @@ -93,6 +93,7 @@ const state = { reports: [{ _id: 'report1' }], tasks: [{ _id: 'task1' }], }, + contactIdToFetch: 'contact3', loadingSelectedReports: 'is loading reports', loadingSummary: 'is loading summary', }, @@ -379,6 +380,10 @@ describe('Selectors', () => { it('should null check selected contact', () => { expect(Selectors.getSelectedContactChildren.projector({})).to.deep.equal(undefined); }); + + it('should getContactIdToFetch', () => { + expect(Selectors.getContactIdToFetch.projector(state.contacts)).to.deep.equal('contact3'); + }); }); describe('analytics', () => { From 761441b8f1a97d341bebe906758d08e4eedef430 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Thu, 11 May 2023 17:01:48 +0700 Subject: [PATCH 5/6] fixing unit tests --- webapp/tests/karma/ts/effects/contacts.effects.spec.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts index b51a1c07bcd..d4b57956603 100644 --- a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts +++ b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts @@ -76,6 +76,7 @@ describe('Contacts effects', () => { let setLoadingSelectedContact; let setContactsLoadingSummary; let clearSelection; + let setContactIdToFetchStub; const simulateContactsReducer = () => { let selectedContact = null; @@ -119,7 +120,7 @@ describe('Contacts effects', () => { refreshState(); }); - sinon.stub(ContactsActions.prototype, 'setContactIdToFetch').callsFake(id => { + setContactIdToFetchStub.callsFake(id => { contactIdToFetch = id; refreshState(); }); @@ -128,6 +129,7 @@ describe('Contacts effects', () => { beforeEach(() => { setLoadingSelectedContact = sinon.stub(ContactsActions.prototype, 'setLoadingSelectedContact'); setContactsLoadingSummary = sinon.stub(ContactsActions.prototype, 'setContactsLoadingSummary'); + setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); simulateContactsReducer(); }); @@ -160,6 +162,9 @@ describe('Contacts effects', () => { expect(setContactsLoadingSummary.args).to.deep.equal([[true], [false]]); expect(settingSelected.callCount).to.equal(1); expect(settingSelected.args[0]).to.deep.equal([]); + expect(setContactIdToFetchStub.calledTwice).to.be.true; + expect(setContactIdToFetchStub.args[0][0]).to.equal('contactid'); + expect(setContactIdToFetchStub.args[1][0]).to.equal(null); }); it('should load the contact when silent', async () => { @@ -180,6 +185,9 @@ describe('Contacts effects', () => { expect(setLoadingSelectedContact.callCount).to.equal(0); expect(settingSelected.callCount).to.equal(1); expect(settingSelected.args[0]).to.deep.equal([]); + expect(setContactIdToFetchStub.calledTwice).to.be.true; + expect(setContactIdToFetchStub.args[0][0]).to.equal('contactid'); + expect(setContactIdToFetchStub.args[1][0]).to.equal(null); }); it('should handle missing contacts', fakeAsync(() => { From 36bb8fd69fcaab9fc7e47e748e7bda46cb367e18 Mon Sep 17 00:00:00 2001 From: latin-panda <66472237+latin-panda@users.noreply.github.com> Date: Mon, 15 May 2023 12:04:21 +0700 Subject: [PATCH 6/6] Improvement and feedback --- webapp/src/ts/actions/contacts.ts | 7 +-- webapp/src/ts/effects/contacts.effects.ts | 40 ++++++-------- .../contacts/contacts-content.component.ts | 6 +-- webapp/src/ts/reducers/contacts.ts | 4 +- webapp/src/ts/selectors/index.ts | 2 +- .../karma/ts/effects/contacts.effects.spec.ts | 34 ++++++------ .../contacts-content.component.spec.ts | 16 ++---- .../tests/karma/ts/reducers/contacts.spec.ts | 54 +++++++++---------- webapp/tests/karma/ts/selectors/index.spec.ts | 6 +-- 9 files changed, 73 insertions(+), 96 deletions(-) diff --git a/webapp/src/ts/actions/contacts.ts b/webapp/src/ts/actions/contacts.ts index e7127636905..e49afe98723 100644 --- a/webapp/src/ts/actions/contacts.ts +++ b/webapp/src/ts/actions/contacts.ts @@ -9,7 +9,7 @@ export const Actions = { selectContact: createMultiValueAction('SELECT_CONTACT'), setSelectedContact: createSingleValueAction('SET_SELECTED_CONTACT', 'selected'), setContactsLoadingSummary: createSingleValueAction('SET_CONTACT_LOADING_SUMMARY', 'value'), - setContactIdToFetch: createSingleValueAction('SET_CONTACT_ID_TO_FETCH', 'id'), + setContactIdToLoad: createSingleValueAction('SET_CONTACT_ID_TO_LOAD', 'id'), setLoadingSelectedContact: createAction('SET_LOADING_SELECTED_CONTACT'), receiveSelectedContactChildren: createSingleValueAction('RECEIVE_SELECTED_CONTACT_CHILDREN', 'children'), receiveSelectedContactReports: createSingleValueAction('RECEIVE_SELECTED_CONTACT_REPORTS', 'reports'), @@ -23,8 +23,8 @@ export class ContactsActions { private store: Store ) {} - setContactIdToFetch(id) { - return this.store.dispatch(Actions.setContactIdToFetch(id)); + setContactIdToLoad(id) { + return this.store.dispatch(Actions.setContactIdToLoad(id)); } updateContactsList(contacts) { @@ -32,6 +32,7 @@ export class ContactsActions { } clearSelection() { + this.store.dispatch(Actions.setContactIdToLoad(null)); return this.store.dispatch(Actions.setSelectedContact(null)); } diff --git a/webapp/src/ts/effects/contacts.effects.ts b/webapp/src/ts/effects/contacts.effects.ts index d2a3f9b2334..4174494650a 100644 --- a/webapp/src/ts/effects/contacts.effects.ts +++ b/webapp/src/ts/effects/contacts.effects.ts @@ -20,7 +20,7 @@ export class ContactsEffects { private globalActions: GlobalActions; private selectedContact; - private contactIdToFetch; + private contactIdToLoad; constructor( private actions$: Actions, @@ -37,10 +37,10 @@ export class ContactsEffects { combineLatest( this.store.select(Selectors.getSelectedContact), - this.store.select(Selectors.getContactIdToFetch), - ).subscribe(([ selectedContact, contactIdToFetch ]) => { + this.store.select(Selectors.getContactIdToLoad), + ).subscribe(([ selectedContact, contactIdToLoad ]) => { this.selectedContact = selectedContact; - this.contactIdToFetch = contactIdToFetch; + this.contactIdToLoad = contactIdToLoad; }); } @@ -64,18 +64,13 @@ export class ContactsEffects { const loadContact = this .loadContact(id) - .then(() => this.setTitle(id)) + .then(() => this.verifySelectedContactNotChanged(id)) + .then(() => this.setTitle()) .then(() => this.loadChildren(id, userFacilityId)) .then(() => this.loadReports(id, forms)) .then(() => this.loadTargetDoc(id)) .then(() => this.loadContactSummary(id)) .then(() => this.loadTasks(id)) - .then(() => { - if (id === this.contactIdToFetch) { - // Clear ID after loading contact - this.contactsActions.setContactIdToFetch(null); - } - }) .catch(err => { // If the selected contact has changed, just stop loading this one if (err.code === 'SELECTED_CONTACT_CHANGED') { @@ -86,8 +81,7 @@ export class ContactsEffects { } console.error('Error selecting contact', err); this.globalActions.unsetSelected(); - this.contactsActions.setContactIdToFetch(null); - return of(this.contactsActions.setSelectedContact(null)); + return of(this.contactsActions.clearSelection()); }); return of(loadContact); @@ -95,20 +89,16 @@ export class ContactsEffects { ); }, { dispatch: false }); - private setTitle(contactId) { - return this - .verifySelectedContactNotChanged(contactId) - .then(() => { - const routeSnapshot = this.routeSnapshotService.get(); - const deceasedTitle = routeSnapshot?.data?.name === 'contacts.deceased' - ? this.translateService.instant('contact.deceased.title') : null; - const title = deceasedTitle || this.selectedContact.type?.name_key || 'contact.profile'; - this.globalActions.setTitle(this.translateService.instant(title)); - }); + private setTitle() { + const routeSnapshot = this.routeSnapshotService.get(); + const deceasedTitle = routeSnapshot?.data?.name === 'contacts.deceased' + ? this.translateService.instant('contact.deceased.title') : null; + const title = deceasedTitle || this.selectedContact.type?.name_key || 'contact.profile'; + this.globalActions.setTitle(this.translateService.instant(title)); } private loadContact(id) { - this.contactsActions.setContactIdToFetch(id); + this.contactsActions.setContactIdToLoad(id); return this.contactViewModelGeneratorService .getContact(id, { merge: false }) .then(model => { @@ -122,7 +112,7 @@ export class ContactsEffects { } private verifySelectedContactNotChanged(id) { - return this.contactIdToFetch !== id ? Promise.reject({code: 'SELECTED_CONTACT_CHANGED'}) : Promise.resolve(); + return this.contactIdToLoad !== id ? Promise.reject({code: 'SELECTED_CONTACT_CHANGED'}) : Promise.resolve(); } private loadChildren(contactId, userFacilityId) { diff --git a/webapp/src/ts/modules/contacts/contacts-content.component.ts b/webapp/src/ts/modules/contacts/contacts-content.component.ts index ff268107fff..08df1779f71 100644 --- a/webapp/src/ts/modules/contacts/contacts-content.component.ts +++ b/webapp/src/ts/modules/contacts/contacts-content.component.ts @@ -96,8 +96,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy { ngOnDestroy() { this.subscription.unsubscribe(); - this.contactsActions.setContactIdToFetch(null); - this.contactsActions.setSelectedContact(null); + this.contactsActions.clearSelection(); this.globalActions.setRightActionBar({}); } @@ -207,8 +206,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy { $('.tooltip').remove(); } else { - this.contactsActions.setContactIdToFetch(null); - this.contactsActions.setSelectedContact(null); + this.contactsActions.clearSelection(); this.globalActions.unsetSelected(); } }); diff --git a/webapp/src/ts/reducers/contacts.ts b/webapp/src/ts/reducers/contacts.ts index 9fc5fa224e6..fcb7a3ddfd2 100644 --- a/webapp/src/ts/reducers/contacts.ts +++ b/webapp/src/ts/reducers/contacts.ts @@ -7,7 +7,7 @@ import { Actions } from '@mm-actions/contacts'; const initialState = { contacts: [], contactsById: new Map(), - contactIdToFetch: null, + contactIdToLoad: null, selected: null, filters: {}, loadingSelectedChildren: false, @@ -141,7 +141,7 @@ const receiveSelectedContactTargetDoc = (state, targetDoc) => { const _contactsReducer = createReducer( initialState, - on(Actions.setContactIdToFetch, (state, { payload: { id } }) => ({ ...state, contactIdToFetch: id })), + on(Actions.setContactIdToLoad, (state, { payload: { id } }) => ({ ...state, contactIdToLoad: id })), on(Actions.updateContactsList, (state, { payload: { contacts } }) => updateContacts(state, contacts)), on(Actions.resetContactsList, (state) => ({ ...state, contacts: [], contactsById: new Map() })), on(Actions.removeContactFromList, (state, { payload: { contact } }) => removeContact(state, contact)), diff --git a/webapp/src/ts/selectors/index.ts b/webapp/src/ts/selectors/index.ts index 7fcde3cf8c3..063117ebf83 100644 --- a/webapp/src/ts/selectors/index.ts +++ b/webapp/src/ts/selectors/index.ts @@ -80,7 +80,7 @@ export const Selectors = { contactListContains: createSelector(getContactsState, (contactsState) => { return (id) => contactsState.contactsById.has(id); }), - getContactIdToFetch: createSelector(getContactsState, (contactState) => contactState.contactIdToFetch), + getContactIdToLoad: createSelector(getContactsState, (contactState) => contactState.contactIdToLoad), getSelectedContact: createSelector(getContactsState, (contactState) => contactState.selected), getSelectedContactDoc: createSelector(getContactsState, (contactState) => contactState.selected?.doc), getSelectedContactSummary: createSelector(getContactsState, (contactState) => contactState.selected?.summary), diff --git a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts index d4b57956603..2ad9162f42a 100644 --- a/webapp/tests/karma/ts/effects/contacts.effects.spec.ts +++ b/webapp/tests/karma/ts/effects/contacts.effects.spec.ts @@ -75,16 +75,16 @@ describe('Contacts effects', () => { describe('selectContact', () => { let setLoadingSelectedContact; let setContactsLoadingSummary; - let clearSelection; - let setContactIdToFetchStub; + let clearSelectionStub; + let setContactIdToLoadStub; const simulateContactsReducer = () => { let selectedContact = null; - let contactIdToFetch = null; + let contactIdToLoad = null; const refreshState = () => { store.overrideSelector(Selectors.getSelectedContact, selectedContact); - store.overrideSelector(Selectors.getContactIdToFetch, contactIdToFetch); + store.overrideSelector(Selectors.getContactIdToLoad, contactIdToLoad); store.refreshState(); }; refreshState(); @@ -120,8 +120,8 @@ describe('Contacts effects', () => { refreshState(); }); - setContactIdToFetchStub.callsFake(id => { - contactIdToFetch = id; + setContactIdToLoadStub.callsFake(id => { + contactIdToLoad = id; refreshState(); }); }; @@ -129,17 +129,17 @@ describe('Contacts effects', () => { beforeEach(() => { setLoadingSelectedContact = sinon.stub(ContactsActions.prototype, 'setLoadingSelectedContact'); setContactsLoadingSummary = sinon.stub(ContactsActions.prototype, 'setContactsLoadingSummary'); - setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); + setContactIdToLoadStub = sinon.stub(ContactsActions.prototype, 'setContactIdToLoad'); + clearSelectionStub = sinon.stub(ContactsActions.prototype, 'clearSelection'); simulateContactsReducer(); }); it('should deselect when no provided id', waitForAsync(() => { - clearSelection = sinon.stub(ContactsActions.prototype, 'clearSelection'); actions$ = of(ContactActionList.selectContact({ })); effects.selectContact.subscribe(); - expect(clearSelection.callCount).to.equal(1); - expect(contactViewModelGeneratorService.getContact.callCount).to.equal(0); + expect(clearSelectionStub.calledOnce).to.be.true; + expect(contactViewModelGeneratorService.getContact.notCalled).to.be.true; })); it('should load the contact when not silent', async () => { @@ -162,9 +162,8 @@ describe('Contacts effects', () => { expect(setContactsLoadingSummary.args).to.deep.equal([[true], [false]]); expect(settingSelected.callCount).to.equal(1); expect(settingSelected.args[0]).to.deep.equal([]); - expect(setContactIdToFetchStub.calledTwice).to.be.true; - expect(setContactIdToFetchStub.args[0][0]).to.equal('contactid'); - expect(setContactIdToFetchStub.args[1][0]).to.equal(null); + expect(setContactIdToLoadStub.calledOnce).to.be.true; + expect(setContactIdToLoadStub.args[0][0]).to.equal('contactid'); }); it('should load the contact when silent', async () => { @@ -185,16 +184,14 @@ describe('Contacts effects', () => { expect(setLoadingSelectedContact.callCount).to.equal(0); expect(settingSelected.callCount).to.equal(1); expect(settingSelected.args[0]).to.deep.equal([]); - expect(setContactIdToFetchStub.calledTwice).to.be.true; - expect(setContactIdToFetchStub.args[0][0]).to.equal('contactid'); - expect(setContactIdToFetchStub.args[1][0]).to.equal(null); + expect(setContactIdToLoadStub.calledOnce).to.be.true; + expect(setContactIdToLoadStub.args[0][0]).to.equal('contactid'); }); it('should handle missing contacts', fakeAsync(() => { const consoleErrorMock = sinon.stub(console, 'error'); const setSnackbarContent = sinon.stub(GlobalActions.prototype, 'setSnackbarContent'); const unsetSelected = sinon.stub(GlobalActions.prototype, 'unsetSelected'); - const setSelectedContact:any = ContactsActions.prototype.setSelectedContact; contactViewModelGeneratorService.getContact.rejects({ code: 404, error: 'not found'}); actions$ = of(ContactActionList.selectContact({ id: 'contactid', silent: false })); effects.selectContact.subscribe(); @@ -204,8 +201,7 @@ describe('Contacts effects', () => { expect(consoleErrorMock.args[0][0]).to.equal('Error selecting contact'); expect(setSnackbarContent.callCount).to.equal(1); expect(unsetSelected.callCount).to.equal(1); - expect(setSelectedContact.callCount).to.equal(1); - expect(setSelectedContact.args[0][0]).to.equal(null); + expect(clearSelectionStub.calledOnce).to.be.true; expect(contactViewModelGeneratorService.getContact.callCount).to.equal(1); expect(contactViewModelGeneratorService.loadChildren.callCount).to.equal(0); expect(contactViewModelGeneratorService.loadReports.callCount).to.equal(0); diff --git a/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts b/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts index f301b0b3ad7..ea9cbb6f992 100644 --- a/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts +++ b/webapp/tests/karma/ts/modules/contacts/contacts-content.component.spec.ts @@ -158,17 +158,13 @@ describe('Contacts content component', () => { it('ngOnDestroy() should unsubscribe from observables and reset state', () => { const unsubscribeSpy = sinon.spy(component.subscription, 'unsubscribe'); - const setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); - const setSelectedContactStub = sinon.stub(ContactsActions.prototype, 'setSelectedContact'); + const clearSelectionStub = sinon.stub(ContactsActions.prototype, 'clearSelection'); sinon.resetHistory(); component.ngOnDestroy(); expect(unsubscribeSpy.calledOnce).to.be.true; - expect(setContactIdToFetchStub.calledOnce).to.be.true; - expect(setContactIdToFetchStub.args[0][0]).to.equal(null); - expect(setSelectedContactStub.calledOnce).to.be.true; - expect(setSelectedContactStub.args[0][0]).to.equal(null); + expect(clearSelectionStub.calledOnce).to.be.true; expect(globalActions.setRightActionBar.calledOnce).to.be.true; expect(globalActions.setRightActionBar.args[0][0]).to.deep.equal({}); }); @@ -224,8 +220,7 @@ describe('Contacts content component', () => { })); it('should unset selected contact when a param id not set and no search term exists', fakeAsync(() => { - const setContactIdToFetchStub = sinon.stub(ContactsActions.prototype, 'setContactIdToFetch'); - const setSelectedContactStub = sinon.stub(ContactsActions.prototype, 'setSelectedContact'); + const clearSelectionStub = sinon.stub(ContactsActions.prototype, 'clearSelection'); store.overrideSelector(Selectors.getFilters, undefined); sinon.resetHistory(); @@ -233,10 +228,7 @@ describe('Contacts content component', () => { flush(); expect(globalActions.unsetSelected.calledOnce).to.be.true; - expect(setContactIdToFetchStub.calledOnce).to.be.true; - expect(setContactIdToFetchStub.args[0][0]).to.equal(null); - expect(setSelectedContactStub.calledOnce).to.be.true; - expect(setSelectedContactStub.args[0][0]).to.equal(null); + expect(clearSelectionStub.calledOnce).to.be.true; })); describe('Change feed process', () => { diff --git a/webapp/tests/karma/ts/reducers/contacts.spec.ts b/webapp/tests/karma/ts/reducers/contacts.spec.ts index b93b2f19cda..d13d10eb7bd 100644 --- a/webapp/tests/karma/ts/reducers/contacts.spec.ts +++ b/webapp/tests/karma/ts/reducers/contacts.spec.ts @@ -11,7 +11,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], - contactIdToFetch: null, + contactIdToLoad: null, filters: {}, loadingSummary: false, }; @@ -23,7 +23,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], - contactIdToFetch: null, + contactIdToLoad: null, filters: {}, loadingSummary: true, }); @@ -33,7 +33,7 @@ describe('Contacts Reducer', () => { contacts: [], contactsById: new Map(), selected: [], - contactIdToFetch: null, + contactIdToLoad: null, filters: {}, loadingSummary: false, }); @@ -65,7 +65,7 @@ describe('Contacts Reducer', () => { ]), filters: {}, selected: null, - contactIdToFetch: null, + contactIdToLoad: null, loadingSelectedChildren: false, loadingSelectedReports: false, loadingSummary: false, @@ -344,7 +344,7 @@ describe('Contacts Reducer', () => { contactsById: new Map(), filters: {}, selected: { _id: 'selected_contact', some: 'data' }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); @@ -449,7 +449,7 @@ describe('Contacts Reducer', () => { selected: { summary: { some: 'summary' } }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); @@ -502,7 +502,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', children: [{ _id: 'child-1' }] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, loadingSelectedChildren: false, }); @@ -527,7 +527,7 @@ describe('Contacts Reducer', () => { { _id: 'child-2' } ] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, loadingSelectedChildren: false, }); @@ -549,7 +549,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', reports: [{ _id: 'report-1' }] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, loadingSelectedReports: false }); @@ -574,7 +574,7 @@ describe('Contacts Reducer', () => { { _id: 'report-2' } ] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSelectedReports: false, loadingSummary: false, }); @@ -643,7 +643,7 @@ describe('Contacts Reducer', () => { { forId: 'contact-3' } ] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); @@ -671,7 +671,7 @@ describe('Contacts Reducer', () => { { forId: 'contact-1' } ] }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); @@ -692,7 +692,7 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', targetDoc: { _id: 'doc-1' } }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); @@ -714,54 +714,54 @@ describe('Contacts Reducer', () => { _id: 'selected_contact', targetDoc: { _id: 'doc-2' } }, - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); }); - describe('setContactIdToFetch', () => { - it('should set contactIdToFetch in the state', () => { - state.contactIdToFetch = null; + describe('setContactIdToLoad', () => { + it('should set contactIdToLoad in the state', () => { + state.contactIdToLoad = null; - const newState = contactsReducer(state, Actions.setContactIdToFetch('selected_contact_1')); + const newState = contactsReducer(state, Actions.setContactIdToLoad('selected_contact_1')); expect(newState).to.deep.equal({ contacts: [], contactsById: new Map(), filters: {}, selected: [], - contactIdToFetch: 'selected_contact_1', + contactIdToLoad: 'selected_contact_1', loadingSummary: false, }); }); - it('should update contactIdToFetch in the state', () => { - state.contactIdToFetch = 'selected_contact_1'; + it('should update contactIdToLoad in the state', () => { + state.contactIdToLoad = 'selected_contact_1'; - const newState = contactsReducer(state, Actions.setContactIdToFetch('selected_contact_2')); + const newState = contactsReducer(state, Actions.setContactIdToLoad('selected_contact_2')); expect(newState).to.deep.equal({ contacts: [], contactsById: new Map(), filters: {}, selected: [], - contactIdToFetch: 'selected_contact_2', + contactIdToLoad: 'selected_contact_2', loadingSummary: false, }); }); - it('should unset contactIdToFetch in the state', () => { - state.contactIdToFetch = 'selected_contact_1'; + it('should unset contactIdToLoad in the state', () => { + state.contactIdToLoad = 'selected_contact_1'; - const newState = contactsReducer(state, Actions.setContactIdToFetch(null)); + const newState = contactsReducer(state, Actions.setContactIdToLoad(null)); expect(newState).to.deep.equal({ contacts: [], contactsById: new Map(), filters: {}, selected: [], - contactIdToFetch: null, + contactIdToLoad: null, loadingSummary: false, }); }); diff --git a/webapp/tests/karma/ts/selectors/index.spec.ts b/webapp/tests/karma/ts/selectors/index.spec.ts index bef8b9d674f..f41d86ca869 100644 --- a/webapp/tests/karma/ts/selectors/index.spec.ts +++ b/webapp/tests/karma/ts/selectors/index.spec.ts @@ -93,7 +93,7 @@ const state = { reports: [{ _id: 'report1' }], tasks: [{ _id: 'task1' }], }, - contactIdToFetch: 'contact3', + contactIdToLoad: 'contact3', loadingSelectedReports: 'is loading reports', loadingSummary: 'is loading summary', }, @@ -381,8 +381,8 @@ describe('Selectors', () => { expect(Selectors.getSelectedContactChildren.projector({})).to.deep.equal(undefined); }); - it('should getContactIdToFetch', () => { - expect(Selectors.getContactIdToFetch.projector(state.contacts)).to.deep.equal('contact3'); + it('should contactIdToLoad', () => { + expect(Selectors.getContactIdToLoad.projector(state.contacts)).to.deep.equal('contact3'); }); });