From 472222c195d5d953d673519f7ba8e00cf56fc4ca Mon Sep 17 00:00:00 2001 From: Kerry Date: Thu, 28 Apr 2022 14:03:51 +0200 Subject: [PATCH] LLS: error handling on stopping beacon (#8406) * shared stopping error state for timeline, maxi and room warnign Signed-off-by: Kerry Archibald * check for stopping errors in roomlist share warning Signed-off-by: Kerry Archibald * lint Signed-off-by: Kerry Archibald * test stopping errors in OwnBeaconStore Signed-off-by: Kerry Archibald * update LeftPanelLiveShareWarning tests for stopping errors Signed-off-by: Kerry Archibald * reinstate try/catch for stopping beacons in create Signed-off-by: Kerry Archibald * remove unnecessary and buggy beacon stopping on creation Signed-off-by: Kerry Archibald --- .../beacon/LeftPanelLiveShareWarning.tsx | 38 +++++-- .../views/beacon/RoomLiveShareWarning.tsx | 3 +- src/components/views/messages/MBeaconBody.tsx | 1 + src/i18n/strings/en_EN.json | 1 + src/stores/OwnBeaconStore.ts | 44 ++++++--- src/utils/beacon/useOwnLiveBeacons.ts | 21 ++-- .../beacon/LeftPanelLiveShareWarning-test.tsx | 55 ++++++++++- test/stores/OwnBeaconStore-test.ts | 99 ++++++++++++++++--- 8 files changed, 213 insertions(+), 49 deletions(-) diff --git a/src/components/views/beacon/LeftPanelLiveShareWarning.tsx b/src/components/views/beacon/LeftPanelLiveShareWarning.tsx index e13c1620ca..43b00a8fa7 100644 --- a/src/components/views/beacon/LeftPanelLiveShareWarning.tsx +++ b/src/components/views/beacon/LeftPanelLiveShareWarning.tsx @@ -16,6 +16,7 @@ limitations under the License. import classNames from 'classnames'; import React from 'react'; +import { BeaconIdentifier, Room } from 'matrix-js-sdk/src/matrix'; import { useEventEmitterState } from '../../../hooks/useEventEmitter'; import { _t } from '../../../languageHandler'; @@ -34,10 +35,14 @@ interface Props { * Choose the most relevant beacon * and get its roomId */ -const chooseBestBeaconRoomId = (liveBeaconIds, errorBeaconIds): string | undefined => { +const chooseBestBeaconRoomId = ( + liveBeaconIds: BeaconIdentifier[], + updateErrorBeaconIds: BeaconIdentifier[], + locationErrorBeaconIds: BeaconIdentifier[], +): Room['roomId'] | undefined => { // both lists are ordered by creation timestamp in store // so select latest beacon - const beaconId = errorBeaconIds?.[0] ?? liveBeaconIds?.[0]; + const beaconId = updateErrorBeaconIds?.[0] ?? locationErrorBeaconIds?.[0] ?? liveBeaconIds?.[0]; if (!beaconId) { return undefined; } @@ -46,6 +51,16 @@ const chooseBestBeaconRoomId = (liveBeaconIds, errorBeaconIds): string | undefin return beacon?.roomId; }; +const getLabel = (hasStoppingErrors: boolean, hasLocationErrors: boolean): string => { + if (hasStoppingErrors) { + return _t('An error occurred while stopping your live location'); + } + if (hasLocationErrors) { + return _t('An error occured whilst sharing your live location'); + } + return _t('You are sharing your live location'); +}; + const LeftPanelLiveShareWarning: React.FC = ({ isMinimized }) => { const isMonitoringLiveLocation = useEventEmitterState( OwnBeaconStore.instance, @@ -59,6 +74,14 @@ const LeftPanelLiveShareWarning: React.FC = ({ isMinimized }) => { () => OwnBeaconStore.instance.getLiveBeaconIdsWithLocationPublishError(), ); + const beaconIdsWithStoppingError = useEventEmitterState( + OwnBeaconStore.instance, + OwnBeaconStoreEvent.BeaconUpdateError, + () => OwnBeaconStore.instance.getLiveBeaconIds().filter( + beaconId => OwnBeaconStore.instance.beaconUpdateErrors.has(beaconId), + ), + ); + const liveBeaconIds = useEventEmitterState( OwnBeaconStore.instance, OwnBeaconStoreEvent.LivenessChange, @@ -66,12 +89,15 @@ const LeftPanelLiveShareWarning: React.FC = ({ isMinimized }) => { ); const hasLocationPublishErrors = !!beaconIdsWithLocationPublishError.length; + const hasStoppingErrors = !!beaconIdsWithStoppingError.length; if (!isMonitoringLiveLocation) { return null; } - const relevantBeaconRoomId = chooseBestBeaconRoomId(liveBeaconIds, beaconIdsWithLocationPublishError); + const relevantBeaconRoomId = chooseBestBeaconRoomId( + liveBeaconIds, beaconIdsWithStoppingError, beaconIdsWithLocationPublishError, + ); const onWarningClick = relevantBeaconRoomId ? () => { dispatcher.dispatch({ @@ -81,14 +107,12 @@ const LeftPanelLiveShareWarning: React.FC = ({ isMinimized }) => { }); } : undefined; - const label = hasLocationPublishErrors ? - _t('An error occured whilst sharing your live location') : - _t('You are sharing your live location'); + const label = getLabel(hasStoppingErrors, hasLocationPublishErrors); return = ({ l }; return
diff --git a/src/components/views/messages/MBeaconBody.tsx b/src/components/views/messages/MBeaconBody.tsx index bd7e10f044..fb82cff29e 100644 --- a/src/components/views/messages/MBeaconBody.tsx +++ b/src/components/views/messages/MBeaconBody.tsx @@ -146,6 +146,7 @@ const MBeaconBody: React.FC = React.forwardRef(({ mxEvent }, ref) => className='mx_MBeaconBody_chin' beacon={beacon} displayStatus={displayStatus} + withIcon /> : ; beaconLocationPublishErrorCounts: Map; + beaconUpdateErrors: Map; beaconsByRoomId: Map>; liveBeaconIds: BeaconIdentifier[]; }; @@ -99,6 +101,7 @@ export class OwnBeaconStore extends AsyncStoreWithClient { * Reset on successful publish of location */ public readonly beaconLocationPublishErrorCounts = new Map(); + public readonly beaconUpdateErrors = new Map(); /** * ids of live beacons * ordered by creation time descending @@ -144,6 +147,7 @@ export class OwnBeaconStore extends AsyncStoreWithClient { this.beaconsByRoomId.clear(); this.liveBeaconIds = []; this.beaconLocationPublishErrorCounts.clear(); + this.beaconUpdateErrors.clear(); } protected async onReady(): Promise { @@ -215,7 +219,6 @@ export class OwnBeaconStore extends AsyncStoreWithClient { } await this.updateBeaconEvent(beacon, { live: false }); - // prune from local store removeLocallyCreateBeaconEventId(beacon.beaconInfoId); }; @@ -300,7 +303,10 @@ export class OwnBeaconStore extends AsyncStoreWithClient { * Live beacon ids that do not have wire errors */ private get healthyLiveBeaconIds() { - return this.liveBeaconIds.filter(beaconId => !this.beaconHasLocationPublishError(beaconId)); + return this.liveBeaconIds.filter(beaconId => + !this.beaconHasLocationPublishError(beaconId) && + !this.beaconUpdateErrors.has(beaconId), + ); } private initialiseBeaconState = () => { @@ -393,19 +399,6 @@ export class OwnBeaconStore extends AsyncStoreWithClient { ); storeLocallyCreateBeaconEventId(event_id); - - // try to stop any other live beacons - // in this room - this.beaconsByRoomId.get(roomId)?.forEach(beaconId => { - if (this.getBeaconById(beaconId)?.isLive) { - try { - // don't await, this is best effort - this.stopBeacon(beaconId); - } catch (error) { - logger.error('Failed to stop live beacons', error); - } - } - }); }; /** @@ -510,6 +503,11 @@ export class OwnBeaconStore extends AsyncStoreWithClient { * MatrixClient api */ + /** + * Updates beacon with provided content update + * Records error in beaconUpdateErrors + * rethrows + */ private updateBeaconEvent = async (beacon: Beacon, update: Partial): Promise => { const { description, timeout, timestamp, live, assetType } = { ...beacon.beaconInfo, @@ -523,7 +521,21 @@ export class OwnBeaconStore extends AsyncStoreWithClient { timestamp, ); - await this.matrixClient.unstable_setLiveBeacon(beacon.roomId, updateContent); + try { + await this.matrixClient.unstable_setLiveBeacon(beacon.roomId, updateContent); + // cleanup any errors + const hadError = this.beaconUpdateErrors.has(beacon.identifier); + if (hadError) { + this.beaconUpdateErrors.delete(beacon.identifier); + this.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon.identifier, false); + } + } catch (error) { + logger.error('Failed to update beacon', error); + this.beaconUpdateErrors.set(beacon.identifier, error); + this.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon.identifier, true); + + throw error; + } }; /** diff --git a/src/utils/beacon/useOwnLiveBeacons.ts b/src/utils/beacon/useOwnLiveBeacons.ts index fcbd8b38a5..1eb892f0c3 100644 --- a/src/utils/beacon/useOwnLiveBeacons.ts +++ b/src/utils/beacon/useOwnLiveBeacons.ts @@ -39,7 +39,6 @@ type LiveBeaconsState = { */ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeaconsState => { const [stoppingInProgress, setStoppingInProgress] = useState(false); - const [error, setError] = useState(); const hasLocationPublishError = useEventEmitterState( OwnBeaconStore.instance, @@ -48,10 +47,22 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon liveBeaconIds.some(OwnBeaconStore.instance.beaconHasLocationPublishError), ); + const hasStopSharingError = useEventEmitterState( + OwnBeaconStore.instance, + OwnBeaconStoreEvent.BeaconUpdateError, + () => + liveBeaconIds.some(id => OwnBeaconStore.instance.beaconUpdateErrors.has(id)), + ); + + useEffect(() => { + if (hasStopSharingError) { + setStoppingInProgress(false); + } + }, [hasStopSharingError]); + // reset stopping in progress on change in live ids useEffect(() => { setStoppingInProgress(false); - setError(undefined); }, [liveBeaconIds]); // select the beacon with latest expiry to display expiry time @@ -64,10 +75,6 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon try { await Promise.all(liveBeaconIds.map(beaconId => OwnBeaconStore.instance.stopBeacon(beaconId))); } catch (error) { - // only clear loading in case of error - // to avoid flash of not-loading state - // after beacons have been stopped but we wait for sync - setError(error); setStoppingInProgress(false); } }; @@ -82,6 +89,6 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon beacon, stoppingInProgress, hasLocationPublishError, - hasStopSharingError: !!error, + hasStopSharingError, }; }; diff --git a/test/components/views/beacon/LeftPanelLiveShareWarning-test.tsx b/test/components/views/beacon/LeftPanelLiveShareWarning-test.tsx index a9fe8e55d7..d02f4d0c3d 100644 --- a/test/components/views/beacon/LeftPanelLiveShareWarning-test.tsx +++ b/test/components/views/beacon/LeftPanelLiveShareWarning-test.tsx @@ -18,7 +18,7 @@ import React from 'react'; import { mocked } from 'jest-mock'; import { mount } from 'enzyme'; import { act } from 'react-dom/test-utils'; -import { Beacon } from 'matrix-js-sdk/src/matrix'; +import { Beacon, BeaconIdentifier } from 'matrix-js-sdk/src/matrix'; import LeftPanelLiveShareWarning from '../../../../src/components/views/beacon/LeftPanelLiveShareWarning'; import { OwnBeaconStore, OwnBeaconStoreEvent } from '../../../../src/stores/OwnBeaconStore'; @@ -33,6 +33,7 @@ jest.mock('../../../../src/stores/OwnBeaconStore', () => { public getLiveBeaconIdsWithLocationPublishError = jest.fn().mockReturnValue([]); public getBeaconById = jest.fn(); public getLiveBeaconIds = jest.fn().mockReturnValue([]); + public readonly beaconUpdateErrors = new Map(); } return { // @ts-ignore @@ -59,6 +60,8 @@ describe('', () => { beforeEach(() => { jest.spyOn(global.Date, 'now').mockReturnValue(now); jest.spyOn(dispatcher, 'dispatch').mockClear().mockImplementation(() => { }); + + OwnBeaconStore.instance.beaconUpdateErrors.clear(); }); afterAll(() => { @@ -191,5 +194,55 @@ describe('', () => { expect(component.html()).toBe(null); }); + + describe('stopping errors', () => { + it('renders stopping error', () => { + OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error')); + const component = getComponent(); + expect(component.text()).toEqual('An error occurred while stopping your live location'); + }); + + it('starts rendering stopping error on beaconUpdateError emit', () => { + const component = getComponent(); + // no error + expect(component.text()).toEqual('You are sharing your live location'); + + act(() => { + OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error')); + OwnBeaconStore.instance.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon2.identifier, true); + }); + + expect(component.text()).toEqual('An error occurred while stopping your live location'); + }); + + it('renders stopping error when beacons have stopping and location errors', () => { + mocked(OwnBeaconStore.instance).getLiveBeaconIdsWithLocationPublishError.mockReturnValue( + [beacon1.identifier], + ); + OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error')); + const component = getComponent(); + expect(component.text()).toEqual('An error occurred while stopping your live location'); + }); + + it('goes to room of latest beacon with stopping error when clicked', () => { + mocked(OwnBeaconStore.instance).getLiveBeaconIdsWithLocationPublishError.mockReturnValue( + [beacon1.identifier], + ); + OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error')); + const component = getComponent(); + const dispatchSpy = jest.spyOn(dispatcher, 'dispatch'); + + act(() => { + component.simulate('click'); + }); + + expect(dispatchSpy).toHaveBeenCalledWith({ + action: Action.ViewRoom, + metricsTrigger: undefined, + // stopping error beacon's room + room_id: beacon2.roomId, + }); + }); + }); }); }); diff --git a/test/stores/OwnBeaconStore-test.ts b/test/stores/OwnBeaconStore-test.ts index 5430c11c68..d44c12620c 100644 --- a/test/stores/OwnBeaconStore-test.ts +++ b/test/stores/OwnBeaconStore-test.ts @@ -765,6 +765,64 @@ describe('OwnBeaconStore', () => { ); }); + it('records error when stopping beacon event fails to send', async () => { + jest.spyOn(logger, 'error').mockImplementation(() => {}); + makeRoomsWithStateEvents([ + alicesRoom1BeaconInfo, + ]); + const store = await makeOwnBeaconStore(); + const emitSpy = jest.spyOn(store, 'emit'); + const error = new Error('oups'); + mockClient.unstable_setLiveBeacon.mockRejectedValue(error); + + await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error); + + expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error); + expect(emitSpy).toHaveBeenCalledWith( + OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), true, + ); + }); + + it('clears previous error and emits when stopping beacon works on retry', async () => { + jest.spyOn(logger, 'error').mockImplementation(() => {}); + makeRoomsWithStateEvents([ + alicesRoom1BeaconInfo, + ]); + const store = await makeOwnBeaconStore(); + const emitSpy = jest.spyOn(store, 'emit'); + const error = new Error('oups'); + mockClient.unstable_setLiveBeacon.mockRejectedValueOnce(error); + + await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error); + expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error); + + await store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo)); + + // error cleared + expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toBeFalsy(); + + // emit called for error clearing + expect(emitSpy).toHaveBeenCalledWith( + OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), false, + ); + }); + + it('does not emit BeaconUpdateError when stopping succeeds and beacon did not have errors', async () => { + jest.spyOn(logger, 'error').mockImplementation(() => {}); + makeRoomsWithStateEvents([ + alicesRoom1BeaconInfo, + ]); + const store = await makeOwnBeaconStore(); + const emitSpy = jest.spyOn(store, 'emit'); + // error cleared + expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toBeFalsy(); + + // emit called for error clearing + expect(emitSpy).not.toHaveBeenCalledWith( + OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), false, + ); + }); + it('updates beacon to live:false when it is expired but live property is true', async () => { makeRoomsWithStateEvents([ alicesRoom1BeaconInfo, @@ -1051,6 +1109,31 @@ describe('OwnBeaconStore', () => { ); }); + it('stops publishing positions when a beacon has a stopping error', async () => { + // reject stopping beacon + const error = new Error('oups'); + mockClient.unstable_setLiveBeacon.mockRejectedValue(error); + makeRoomsWithStateEvents([ + alicesRoom1BeaconInfo, + ]); + const store = await makeOwnBeaconStore(); + // wait for store to settle + await flushPromisesWithFakeTimers(); + + // 2 positions from watchPosition in this period + await advanceAndFlushPromises(5000); + + // attempt to stop the beacon + await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error); + expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error); + + // 2 more positions in this period + await advanceAndFlushPromises(50000); + + // only two positions pre-stopping were sent + expect(mockClient.sendEvent).toHaveBeenCalledTimes(3); + }); + it('restarts publishing a beacon after resetting location publish error', async () => { // always fails to send events mockClient.sendEvent.mockRejectedValue(new Error('oups')); @@ -1266,21 +1349,5 @@ describe('OwnBeaconStore', () => { // didn't throw, no error log expect(loggerErrorSpy).not.toHaveBeenCalled(); }); - - it('stops live beacons for room after creating new beacon', async () => { - // room1 already has a beacon - makeRoomsWithStateEvents([ - alicesRoom1BeaconInfo, - ]); - // but it was not created on this device - localStorageGetSpy.mockReturnValue(undefined); - - const store = await makeOwnBeaconStore(); - const content = makeBeaconInfoContent(100); - await store.createLiveBeacon(room1Id, content); - - // update beacon called - expect(mockClient.unstable_setLiveBeacon).toHaveBeenCalled(); - }); }); });