fix(#7250): race condition in loading contacts

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'),
+  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'),
@@ -22,11 +23,16 @@ export class ContactsActions {
     private store: Store
   ) {}
 
+  setContactIdToLoad(id) {
+    return this.store.dispatch(Actions.setContactIdToLoad(id));
+  }
+
   updateContactsList(contacts) {
     return this.store.dispatch(Actions.updateContactsList(contacts));
   }
 
   clearSelection() {
+    this.store.dispatch(Actions.setContactIdToLoad(null));
     return this.store.dispatch(Actions.setSelectedContact(null));
   }
 

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 contactIdToLoad;
 
   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.getContactIdToLoad),
+    ).subscribe(([ selectedContact, contactIdToLoad ]) => {
+      this.selectedContact = selectedContact;
+      this.contactIdToLoad = contactIdToLoad;
+    });
   }
 
   selectContact = createEffect(() => {
@@ -59,6 +64,8 @@ export class ContactsEffects {
 
         const loadContact = this
           .loadContact(id)
+          .then(() => this.verifySelectedContactNotChanged(id))
+          .then(() => this.setTitle())
           .then(() => this.loadChildren(id, userFacilityId))
           .then(() => this.loadReports(id, forms))
           .then(() => this.loadTargetDoc(id))
@@ -74,34 +81,38 @@ export class ContactsEffects {
             }
             console.error('Error selecting contact', err);
             this.globalActions.unsetSelected();
-            return of(this.contactsActions.setSelectedContact(null));
+            return of(this.contactsActions.clearSelection());
           });
 
         return of(loadContact);
       }),
     );
   }, { dispatch: false });
 
-  private setTitle(selected) {
+  private setTitle() {
     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';
+    const title = deceasedTitle || this.selectedContact.type?.name_key || 'contact.profile';
     this.globalActions.setTitle(this.translateService.instant(title));
   }
 
   private loadContact(id) {
+    this.contactsActions.setContactIdToLoad(id);
     return this.contactViewModelGeneratorService
       .getContact(id, { merge: false })
       .then(model => {
-        this.globalActions.settingSelected();
-        this.contactsActions.setSelectedContact(model);
-        this.setTitle(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.contactIdToLoad !== id ? Promise.reject({code: 'SELECTED_CONTACT_CHANGED'}) : Promise.resolve();
   }
 
   private loadChildren(contactId, userFacilityId) {

webapp/src/ts/modules/contacts/contacts-content.component.ts

@@ -96,7 +96,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy {
 
   ngOnDestroy() {
     this.subscription.unsubscribe();
-    this.contactsActions.setSelectedContact(null);
+    this.contactsActions.clearSelection();
     this.globalActions.setRightActionBar({});
   }
 
@@ -206,7 +206,7 @@ export class ContactsContentComponent implements OnInit, OnDestroy {
 
         $('.tooltip').remove();
       } else {
-        this.contactsActions.setSelectedContact(null);
+        this.contactsActions.clearSelection();
         this.globalActions.unsetSelected();
       }
     });

webapp/src/ts/reducers/contacts.ts

@@ -7,6 +7,7 @@ import { Actions } from '@mm-actions/contacts';
 const initialState = {
   contacts: [],
   contactsById: new Map(),
+  contactIdToLoad: null,
   selected: null,
   filters: {},
   loadingSelectedChildren: false,
@@ -140,6 +141,7 @@ const receiveSelectedContactTargetDoc = (state, targetDoc) => {
 
 const _contactsReducer = createReducer(
   initialState,
+  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)),

webapp/src/ts/selectors/index.ts

@@ -80,6 +80,7 @@ export const Selectors = {
   contactListContains: createSelector(getContactsState, (contactsState) => {
     return (id) => contactsState.contactsById.has(id);
   }),
+  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),

webapp/tests/karma/ts/effects/contacts.effects.spec.ts

@@ -75,13 +75,16 @@ describe('Contacts effects', () => {
   describe('selectContact', () => {
     let setLoadingSelectedContact;
     let setContactsLoadingSummary;
-    let clearSelection;
+    let clearSelectionStub;
+    let setContactIdToLoadStub;
 
     const simulateContactsReducer = () => {
       let selectedContact = null;
+      let contactIdToLoad = null;
 
       const refreshState = () => {
         store.overrideSelector(Selectors.getSelectedContact, selectedContact);
+        store.overrideSelector(Selectors.getContactIdToLoad, contactIdToLoad);
         store.refreshState();
       };
       refreshState();
@@ -116,21 +119,27 @@ describe('Contacts effects', () => {
         selectedContact = { ...selectedContact, summary };
         refreshState();
       });
+
+      setContactIdToLoadStub.callsFake(id => {
+        contactIdToLoad = id;
+        refreshState();
+      });
     };
 
     beforeEach(() => {
       setLoadingSelectedContact = sinon.stub(ContactsActions.prototype, 'setLoadingSelectedContact');
       setContactsLoadingSummary = sinon.stub(ContactsActions.prototype, 'setContactsLoadingSummary');
+      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 () => {
@@ -153,6 +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(setContactIdToLoadStub.calledOnce).to.be.true;
+      expect(setContactIdToLoadStub.args[0][0]).to.equal('contactid');
     });
 
     it('should load the contact when silent', async () => {
@@ -173,13 +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(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();
@@ -189,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);

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,19 @@ 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 clearSelectionStub = sinon.stub(ContactsActions.prototype, 'clearSelection');
+    sinon.resetHistory();
+
+    component.ngOnDestroy();
+
+    expect(unsubscribeSpy.calledOnce).to.be.true;
+    expect(clearSelectionStub.calledOnce).to.be.true;
+    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 +186,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 +203,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 +219,18 @@ 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 clearSelectionStub = sinon.stub(ContactsActions.prototype, 'clearSelection');
+    store.overrideSelector(Selectors.getFilters, undefined);
+    sinon.resetHistory();
+
+    component.ngOnInit();
+    flush();
+
+    expect(globalActions.unsetSelected.calledOnce).to.be.true;
+    expect(clearSelectionStub.calledOnce).to.be.true;
+  }));
+
   describe('Change feed process', () => {
     let change;