From 8c2f6331113e15ab5f69b685b0755bb72d68f3a6 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Sun, 24 Nov 2024 16:13:44 -0800 Subject: [PATCH 1/2] Correctly reverse failed save search --- src/app/dim-api/api-types.ts | 2 +- src/app/dim-api/reducer.ts | 78 +++++++++++++++++++++++++++--------- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/src/app/dim-api/api-types.ts b/src/app/dim-api/api-types.ts index 7a117a7d72..14bc707907 100644 --- a/src/app/dim-api/api-types.ts +++ b/src/app/dim-api/api-types.ts @@ -9,7 +9,7 @@ import { type AddUpdateInfo = U extends ProfileUpdate ? U & { /** The state before this update - if it fails we can use this to roll back */ - before?: U['payload']; + before: U['payload'] | undefined; /** The account (if any) this update refers to */ platformMembershipId?: string; destinyVersion?: DestinyVersion; diff --git a/src/app/dim-api/reducer.ts b/src/app/dim-api/reducer.ts index b6c2763ff1..6a3ab8cbce 100644 --- a/src/app/dim-api/reducer.ts +++ b/src/app/dim-api/reducer.ts @@ -941,10 +941,8 @@ function updateLoadout(state: DimApiState, loadout: DimLoadout, account: Destiny payload: newLoadout, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, + before: loadouts[loadout.id], }; - if (loadouts[loadout.id]) { - update.before = loadouts[loadout.id]; - } applyUpdateLocally(draft, update); draft.updateQueue.push(update); }); @@ -1087,6 +1085,7 @@ function tagCleanup(state: DimApiState, itemIdsToRemove: string[], account: Dest // "before" isn't really valuable here platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, + before: undefined, }; applyUpdateLocally(draft, updateAction); draft.updateQueue.push(updateAction); @@ -1142,6 +1141,7 @@ function searchUsed( query, type, }, + before: undefined, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1203,6 +1203,7 @@ function saveSearch( query, type, }, + before: undefined, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1217,6 +1218,11 @@ function saveSearch( saved, type, }, + before: { + query, + saved: !saved, + type, + }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1236,6 +1242,12 @@ function deleteSearch( query, type, }, + before: { + query, + type, + // TODO: How to get this?? + saved: false, + }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1422,9 +1434,17 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith const searches = draft.searches[destinyVersion!]; const existingSearch = searches.find((s) => s.query === query); - // This should always exist + // This might not exist if reversing a delete_search if (existingSearch) { existingSearch.saved = saved; + } else { + searches.push({ + query, + usageCount: 1, + saved, + lastUsage: Date.now(), + type: update.payload.type, + }); } break; } @@ -1432,21 +1452,41 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith } function reverseUpdateLocally(draft: Draft, update: ProfileUpdateWithRollback) { - switch (update.action) { - case 'delete_loadout': { - const { platformMembershipId, destinyVersion } = update; - const loadoutId = update.payload; - const profileKey = makeProfileKey(platformMembershipId!, destinyVersion!); - const loadouts = ensureProfile(draft, profileKey).loadouts; - loadouts[loadoutId] = update.before as Loadout; - break; + if (!update.before) { + return; + } + try { + switch (update.action) { + case 'delete_loadout': { + const { platformMembershipId, destinyVersion } = update; + const loadoutId = update.payload; + const profileKey = makeProfileKey(platformMembershipId!, destinyVersion!); + const loadouts = ensureProfile(draft, profileKey).loadouts; + loadouts[loadoutId] = update.before as Loadout; + break; + } + case 'delete_search': { + // delete_search reverses to save_search + applyUpdateLocally(draft, { + ...update, + action: 'save_search', + payload: update.before, + before: update.payload, + } as ProfileUpdateWithRollback); + break; + } + default: + applyUpdateLocally(draft, { + ...update, + payload: update.before, + before: update.payload, + } as ProfileUpdateWithRollback); + break; } - default: - applyUpdateLocally(draft, { - ...update, - payload: update.before, - before: update.payload, - } as ProfileUpdateWithRollback); - break; + } catch (e) { + // We don't want to endlessly retry the update if we fail to reverse. The + // next profile load will reset the info. + errorLog('reverseUpdateLocally', e, update); + reportException('reverseUpdateLocally', e, { update }); } } From 24c87bb771d982f3595c54bb5fdb7554d8f25ea2 Mon Sep 17 00:00:00 2001 From: Ben Hollis Date: Sun, 24 Nov 2024 17:28:44 -0800 Subject: [PATCH 2/2] Revamp dim-api reducer test and fix more reversing bugs --- src/app/dim-api/api-types.ts | 13 + src/app/dim-api/reducer.test.ts | 1161 +++++++++++++++---------------- src/app/dim-api/reducer.ts | 81 ++- 3 files changed, 641 insertions(+), 614 deletions(-) diff --git a/src/app/dim-api/api-types.ts b/src/app/dim-api/api-types.ts index 14bc707907..02f51b73f8 100644 --- a/src/app/dim-api/api-types.ts +++ b/src/app/dim-api/api-types.ts @@ -1,8 +1,10 @@ import { DeleteLoadoutUpdate, + DeleteSearchUpdate, DestinyVersion, Loadout, ProfileUpdate, + SearchType, } from '@destinyitemmanager/dim-api-types'; // https://stackoverflow.com/questions/51691235/typescript-map-union-type-to-another-union-type @@ -22,9 +24,20 @@ export interface DeleteLoadoutUpdateWithRollback extends DeleteLoadoutUpdate { destinyVersion: DestinyVersion; } +export interface DeleteSearchUpdateWithRollback extends DeleteSearchUpdate { + before: { + query: string; + type: SearchType; + saved: boolean; + }; + platformMembershipId: string; + destinyVersion: DestinyVersion; +} + /** * A version of ProfileUpdate that also includes rollback info in a "before" property. */ export type ProfileUpdateWithRollback = + | DeleteSearchUpdateWithRollback | DeleteLoadoutUpdateWithRollback | AddUpdateInfo; diff --git a/src/app/dim-api/reducer.test.ts b/src/app/dim-api/reducer.test.ts index da4292f1ca..548fb852c3 100644 --- a/src/app/dim-api/reducer.test.ts +++ b/src/app/dim-api/reducer.test.ts @@ -1,11 +1,27 @@ -import { SearchType } from '@destinyitemmanager/dim-api-types'; +import { ProfileUpdateResult, SearchType } from '@destinyitemmanager/dim-api-types'; import { DestinyAccount } from 'app/accounts/destiny-account'; -import { setItemHashTag, setItemTag } from 'app/inventory/actions'; +import { setItemHashNote, setItemHashTag, setItemNote, setItemTag } from 'app/inventory/actions'; +import { deleteLoadout, updateLoadout } from 'app/loadout/actions'; import { setSettingAction } from 'app/settings/actions'; +import { identity } from 'app/utils/functions'; import { BungieMembershipType, DestinyClass } from 'bungie-api-ts/destiny2'; -import { DeleteLoadoutUpdateWithRollback } from './api-types'; -import { finishedUpdates, prepareToFlushUpdates, saveSearch } from './basic-actions'; -import { DimApiState, initialState as apiInitialState, dimApi } from './reducer'; +import { produce, WritableDraft } from 'immer'; +import { ProfileUpdateWithRollback } from './api-types'; +import { + finishedUpdates, + prepareToFlushUpdates, + saveSearch, + searchDeleted, + searchUsed, +} from './basic-actions'; +import { + initialState as apiInitialState, + dimApi, + DimApiAction, + DimApiState, + ensureProfile, +} from './reducer'; +import { makeProfileKeyFromAccount } from './selectors'; const currentAccount: DestinyAccount = { membershipId: '98765', @@ -23,446 +39,370 @@ const initialState: DimApiState = { apiPermissionGranted: true, }; -describe('setSetting', () => { - it('changes settings', () => { - const state = initialState; - - const updatedState = dimApi(state, setSettingAction('showNewItems', true)); - - expect(updatedState.settings.showNewItems).toBe(true); - expect(updatedState.updateQueue).toEqual([ - { - action: 'setting', - payload: { - showNewItems: true, - }, - before: { - showNewItems: false, - }, - }, - ]); - }); -}); - -describe('setItemTag', () => { - it('sets tags if there were none before', () => { - const state = initialState; - - const updatedState = dimApi( - state, - setItemTag({ itemId: '1234', tag: 'favorite' }), - currentAccount, - ); - - expect(updatedState.profiles[currentAccountKey].tags['1234'].tag).toBe('favorite'); - expect(updatedState.updateQueue).toEqual([ - { - action: 'tag', - payload: { - id: '1234', - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, - }, - ]); - }); - - it('clears set tags', () => { - const state = initialState; - - let updatedState = dimApi( - state, - setItemTag({ itemId: '1234', tag: 'favorite' }), - currentAccount, - ); - - updatedState = dimApi( - updatedState, - setItemTag({ itemId: '1234', tag: undefined }), - currentAccount, - ); - - expect(updatedState.profiles[currentAccountKey].tags['1234']).toBeUndefined(); - expect(updatedState.updateQueue).toEqual([ - { - action: 'tag', - payload: { - id: '1234', - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, - }, - { - action: 'tag', - payload: { - id: '1234', - tag: null, - }, - before: { - id: '1234', - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, - }, - ]); - }); -}); - -describe('setItemHashTag', () => { - it('sets tags if there were none before', () => { - const state = initialState; - - const updatedState = dimApi( - state, - setItemHashTag({ itemHash: 1234, tag: 'favorite' }), - currentAccount, - ); - - expect(updatedState.itemHashTags[1234].tag).toBe('favorite'); - expect(updatedState.updateQueue).toEqual([ - { - action: 'item_hash_tag', - payload: { - hash: 1234, - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, - }, - ]); - }); - - it('clears set tags', () => { - const state = initialState; - - let updatedState = dimApi( - state, - setItemHashTag({ itemHash: 1234, tag: 'favorite' }), - currentAccount, - ); - - updatedState = dimApi( - updatedState, - setItemHashTag({ itemHash: 1234, tag: undefined }), - currentAccount, - ); - - expect(updatedState.itemHashTags[1234]).toBeUndefined(); - expect(updatedState.updateQueue).toEqual([ - { - action: 'item_hash_tag', - payload: { - hash: 1234, - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, +describe('dim api reducer', () => { + const cases: { + name: string; + /** + * Actions to run to set the initial state before our test actions. These + * will be "flushed" already. + */ + setup?: (state: WritableDraft) => void; + /** + * A list of actions to run, followed by prepareToFlushUpdates. The tuple + * option allows specifying a different account than `currentAccount`. + */ + actions: (DimApiAction | [DimApiAction, DestinyAccount])[]; + /** + * A function for checking expectations on the state after the action. + */ + checkState: (state: DimApiState) => void; + /** + * The expected queue after prepareToFlushUpdates. Only the action and + * payload need to be included. + */ + expectedQueue: Pick[]; + /** + * Set this to skip the reverse-update check. + */ + noReverse?: boolean; + }[] = [ + { + name: 'setSetting: changes settings', + actions: [setSettingAction('showNewItems', true)], + checkState: (state) => { + expect(state.settings.showNewItems).toBe(true); }, - { - action: 'item_hash_tag', - payload: { - hash: 1234, - tag: null, - }, - before: { - hash: 1234, - tag: 'favorite', - }, - platformMembershipId: currentAccount.membershipId, - destinyVersion: currentAccount.destinyVersion, - }, - ]); - }); -}); - -describe('prepareToFlushUpdates', () => { - it('can coalesce settings', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ - // Turn new items on + expectedQueue: [ { action: 'setting', payload: { showNewItems: true, }, - before: { - showNewItems: false, - }, }, - // Modify another setting + ], + }, + { + name: 'setItemTag: sets tags if there were none before', + actions: [setItemTag({ itemId: '1234', tag: 'favorite' })], + checkState: (state) => { + expect(state.profiles[currentAccountKey].tags['1234'].tag).toBe('favorite'); + }, + expectedQueue: [ { - action: 'setting', + action: 'tag', payload: { - itemSize: 50, - }, - before: { - itemSize: 48, + id: '1234', + tag: 'favorite', }, }, - // Turn new items back off + ], + }, + { + name: 'setItemTag: clears set tags', + actions: [ + setItemTag({ itemId: '1234', tag: 'favorite' }), + setItemTag({ itemId: '1234', tag: undefined }), + ], + checkState: (state) => { + expect(state.profiles[currentAccountKey].tags['1234']).toBeUndefined(); + }, + expectedQueue: [ { - action: 'setting', + action: 'tag', payload: { - showNewItems: false, - }, - before: { - showNewItems: true, + id: '1234', + tag: null, }, }, ], - }; - - const updatedState = dimApi(state, prepareToFlushUpdates()); - - expect(updatedState.updateInProgressWatermark).toBe(1); - // Expect that showNewItems change is eliminated, and there's only one update - const expected = [ - { - action: 'setting', - payload: { - itemSize: 50, - }, - before: { - itemSize: 48, - }, + }, + { + name: 'setItemHashTag: sets tags if there were none before', + actions: [setItemHashTag({ itemHash: 1234, tag: 'favorite' })], + checkState: (state) => { + expect(state.itemHashTags[1234].tag).toBe('favorite'); }, - ]; - expect(updatedState.updateQueue).toEqual(expected); - }); - - it('can handle multiple profile updates', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ - // Turn new items on + expectedQueue: [ { - action: 'setting', + action: 'item_hash_tag', payload: { - showNewItems: true, + hash: 1234, + tag: 'favorite', }, - before: { - showNewItems: false, + }, + ], + }, + { + name: 'setItemHashTag: clears set tags', + actions: [ + setItemHashTag({ itemHash: 1234, tag: 'favorite' }), + setItemHashTag({ itemHash: 1234, tag: undefined }), + ], + checkState: (state) => { + expect(state.itemHashTags[1234]?.tag).toBeUndefined(); + }, + expectedQueue: [ + { + action: 'item_hash_tag', + payload: { + hash: 1234, + tag: null, }, }, - // Save a tag for D2 + ], + }, + { + name: 'setItemTag/setNote: can set both tag and note', + actions: [ + setItemTag({ itemId: '1234', tag: 'favorite' }), + setItemNote({ itemId: '1234', note: 'foo' }), + ], + checkState: (state) => { + expect(state.profiles[currentAccountKey].tags['1234'].tag).toBe('favorite'); + expect(state.profiles[currentAccountKey].tags['1234'].notes).toBe('foo'); + }, + expectedQueue: [ { action: 'tag', payload: { id: '1234', tag: 'favorite', + notes: 'foo', }, - before: { - id: '1234', - }, - platformMembershipId: '3456', - destinyVersion: 2, }, - // Save a tag for D1, same profile + ], + }, + { + name: 'setItemTag/setNote: can set both tag and note', + actions: [ + setItemHashTag({ itemHash: 1234, tag: 'favorite' }), + setItemHashNote({ itemHash: 1234, note: 'foo' }), + ], + checkState: (state) => { + expect(state.itemHashTags[1234].tag).toBe('favorite'); + expect(state.itemHashTags[1234].notes).toBe('foo'); + }, + expectedQueue: [ { - action: 'tag', + action: 'item_hash_tag', payload: { - id: '1231903', - tag: 'keep', - }, - before: { - id: '1231903', + hash: 1234, + tag: 'favorite', + notes: 'foo', }, - platformMembershipId: '3456', - destinyVersion: 1, }, - // Save a tag for D2, same profile + ], + }, + { + name: 'searchUsed: can track valid queries', + actions: [searchUsed({ query: '(is:masterwork) (is:weapon)', type: SearchType.Item })], + checkState: (state) => { + const search = state.searches[2][0]; + expect(search.query).toBe('is:masterwork is:weapon'); + expect(search.usageCount).toBe(1); + expect(search.saved).toBe(false); + }, + expectedQueue: [ { - action: 'tag', + action: 'search', payload: { - id: '76543', - tag: 'junk', - }, - before: { - id: '76543', + query: 'is:masterwork is:weapon', + type: SearchType.Item, }, - platformMembershipId: '3456', - destinyVersion: 2, }, ], - }; - - const updatedState = dimApi(state, prepareToFlushUpdates()); - - expect(updatedState.updateInProgressWatermark).toBe(3); - // Expect that the queue is rearranged to have the D2 account updates together - const expected = [ - // Turn new items on - { - action: 'setting', - payload: { - showNewItems: true, - }, - before: { - showNewItems: false, - }, + noReverse: true, + }, + { + name: 'saveSearch: can save valid queries', + setup: (state) => { + state.searches[2] = [ + { + usageCount: 1, + lastUsage: 919191, + saved: false, + query: 'is:masterwork is:weapon', + type: SearchType.Item, + }, + ]; }, - // Save a tag for D2 - { - action: 'tag', - payload: { - id: '1234', - tag: 'favorite', - }, - before: { - id: '1234', - }, - platformMembershipId: '3456', - destinyVersion: 2, + actions: [ + saveSearch({ query: '(is:masterwork) (is:weapon)', saved: true, type: SearchType.Item }), + ], + checkState: (state) => { + const search = state.searches[2][0]; + expect(search.query).toBe('is:masterwork is:weapon'); + expect(search.saved).toBe(true); }, - // Save a tag for D2 - { - action: 'tag', - payload: { - id: '76543', - tag: 'junk', - }, - before: { - id: '76543', + expectedQueue: [ + { + action: 'save_search', + payload: { + query: 'is:masterwork is:weapon', + saved: true, + type: SearchType.Item, + }, }, - platformMembershipId: '3456', - destinyVersion: 2, + ], + }, + { + name: 'saveSearch: can unsave valid queries', + setup: (state) => { + state.searches[2] = [ + { + usageCount: 1, + lastUsage: 919191, + saved: true, + query: 'is:masterwork is:weapon', + type: SearchType.Item, + }, + ]; }, - // Save a tag for D1 - { - action: 'tag', - payload: { - id: '1231903', - tag: 'keep', - }, - before: { - id: '1231903', - }, - platformMembershipId: '3456', - destinyVersion: 1, + actions: [ + saveSearch({ query: '(is:masterwork) (is:weapon)', saved: false, type: SearchType.Item }), + ], + checkState: (state) => { + const search = state.searches[2][0]; + expect(search.query).toBe('is:masterwork is:weapon'); + expect(search.saved).toBe(false); }, - ]; - expect(updatedState.updateQueue).toEqual(expected); - }); - - it('can handle multiple profile updates with settings last', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ - // Save a tag for D2 + expectedQueue: [ { - action: 'tag', + action: 'save_search', payload: { - id: '1234', - tag: 'favorite', - }, - before: { - id: '1234', + query: 'is:masterwork is:weapon', + saved: false, + type: SearchType.Item, }, - platformMembershipId: '3456', - destinyVersion: 2, }, - // Save a tag for D2, same profile + ], + }, + { + name: 'saveSearch: does not save invalid queries', + actions: [saveSearch({ query: 'deepsight:incomplete', saved: true, type: SearchType.Item })], + checkState: (state) => { + expect(state.searches).toMatchObject({ + [1]: [], + [2]: [], + }); + }, + expectedQueue: [], + }, + { + name: 'saveSearch: can unsave previously saved invalid queries', + setup: (state) => { + state.searches[2] = [ + { + usageCount: 1, + lastUsage: 919191, + saved: true, + // This is invalid, but it might have been saved before we changed the rules + query: 'deepsight:incomplete', + type: SearchType.Item, + }, + ]; + }, + actions: [saveSearch({ query: 'deepsight:incomplete', saved: false, type: SearchType.Item })], + checkState: (state) => { + // FIXME maybe delete this outright? It'll be cleaned up the next time DIM loads the remote profile anyway... + const search = state.searches[2][0]; + expect(search.query).toBe('deepsight:incomplete'); + expect(search.saved).toBe(false); + }, + expectedQueue: [ { - action: 'tag', + action: 'save_search', payload: { - id: '76543', - tag: 'junk', - }, - before: { - id: '76543', + query: 'deepsight:incomplete', + saved: false, + type: SearchType.Item, }, - platformMembershipId: '3456', - destinyVersion: 2, }, - // Turn new items on + ], + }, + { + name: 'deleteSearch: can delete previously saved invalid queries', + setup: (state) => { + state.searches[2] = [ + { + usageCount: 1, + lastUsage: 1000, + saved: true, + // This is invalid, but it might have been saved before we changed the rules + query: 'deepsight:incomplete', + type: SearchType.Item, + }, + ]; + }, + actions: [searchDeleted({ query: 'deepsight:incomplete', type: SearchType.Item })], + checkState: (state) => { + // FIXME maybe delete this outright? It'll be cleaned up the next time DIM loads the remote profile anyway... + expect(state.searches[2].length).toBe(0); + }, + expectedQueue: [ { - action: 'setting', + action: 'delete_search', payload: { - showNewItems: true, - }, - before: { - showNewItems: false, + query: 'deepsight:incomplete', + type: SearchType.Item, }, }, ], - }; - - const updatedState = dimApi(state, prepareToFlushUpdates()); - - expect(updatedState.updateInProgressWatermark).toBe(3); - // Expect that the queue is rearranged to have the D2 account updates together - const expected = [ - // Save a tag for D2 - { - action: 'tag', - payload: { + }, + { + name: 'updateLoadout: can save a loadout', + actions: [ + updateLoadout({ id: '1234', - tag: 'favorite', - }, - before: { - id: '1234', - }, - platformMembershipId: '3456', - destinyVersion: 2, - }, - // Save a tag for D2 - { - action: 'tag', - payload: { - id: '76543', - tag: 'junk', - }, - before: { - id: '76543', - }, - platformMembershipId: '3456', - destinyVersion: 2, - }, - // Turn new items on - { - action: 'setting', - payload: { - showNewItems: true, - }, - before: { - showNewItems: false, - }, + name: 'before foo', + classType: DestinyClass.Warlock, + items: [], + clearSpace: false, + }), + ], + checkState: (state) => { + expect(state.profiles[currentAccountKey].loadouts['1234'].name).toBe('before foo'); }, - ]; - expect(updatedState.updateQueue).toEqual(expected); - }); - - it('can handle loadouts', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ - // Save a loadout for D2 + expectedQueue: [ { action: 'loadout', payload: { - id: '1234', - name: 'foo', - classType: DestinyClass.Warlock, - equipped: [], - unequipped: [], - clearSpace: false, - }, - before: { id: '1234', name: 'before foo', - classType: DestinyClass.Unknown, + classType: DestinyClass.Warlock, equipped: [], unequipped: [], clearSpace: false, }, - platformMembershipId: '3456', - destinyVersion: 2, }, - // Update the name + ], + }, + { + name: 'updateLoadout: can update a loadout', + setup: (state) => { + state.profiles[currentAccountKey].loadouts['1234'] = { + id: '1234', + name: 'before foo', + classType: DestinyClass.Warlock, + equipped: [], + unequipped: [], + clearSpace: false, + }; + return state; + }, + actions: [ + updateLoadout({ + id: '1234', + name: 'foo', // changed name + classType: DestinyClass.Warlock, + items: [], + clearSpace: false, + }), + ], + checkState: (state) => { + expect(state.profiles[currentAccountKey].loadouts['1234'].name).toBe('foo'); + }, + expectedQueue: [ { action: 'loadout', payload: { @@ -473,159 +413,290 @@ describe('prepareToFlushUpdates', () => { unequipped: [], clearSpace: false, }, - before: { - id: '1234', - name: 'foobar', - classType: DestinyClass.Warlock, - equipped: [], - unequipped: [], - clearSpace: false, - }, - platformMembershipId: '3456', - destinyVersion: 2, }, - // Delete it - { - action: 'delete_loadout', - payload: '1234', - before: { - id: '1234', - name: 'foo', - classType: DestinyClass.Warlock, - equipped: [], - unequipped: [], - clearSpace: false, - }, - platformMembershipId: '3456', - destinyVersion: 2, - } as DeleteLoadoutUpdateWithRollback, ], - }; - - const updatedState = dimApi(state, prepareToFlushUpdates()); - - expect(updatedState.updateInProgressWatermark).toBe(1); - - // Down to a single delete, with the original loadout as the before - const expected = [ - { - action: 'delete_loadout', - payload: '1234', - before: { + }, + { + name: 'updateLoadout: can delete a loadout', + setup: (state) => { + state.profiles[currentAccountKey].loadouts['1234'] = { id: '1234', name: 'before foo', - classType: DestinyClass.Unknown, + classType: DestinyClass.Warlock, equipped: [], unequipped: [], clearSpace: false, - }, - platformMembershipId: '3456', - destinyVersion: 2, + }; + return state; }, - ]; - expect(updatedState.updateQueue).toEqual(expected); - }); - - it('can handle tag stuff', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ + actions: [deleteLoadout('1234')], + checkState: (state) => { + expect(state.profiles[currentAccountKey].loadouts['1234']).toBeUndefined(); + }, + expectedQueue: [ { - action: 'tag', - payload: { - id: '1234', - tag: 'favorite', - }, - before: { - id: '1234', - tag: 'junk', - }, - platformMembershipId: '3456', - destinyVersion: 2, + action: 'delete_loadout', + payload: '1234', }, + ], + }, + ]; + + for (const { + name, + actions, + checkState, + expectedQueue = [], + setup = identity, + noReverse = false, + } of cases) { + it(name, () => { + // Set up the state + const setupState = produce(initialState, (draft) => { + const profileKey = makeProfileKeyFromAccount(currentAccount); + ensureProfile(draft, profileKey); + setup(draft); + draft.updateQueue = []; + draft.updateInProgressWatermark = 0; + return draft; + }); + + // Apply all the input actions and call prepareToFlushUpdates + const updatedState = [...actions, prepareToFlushUpdates()].reduce((s, action) => { + if (Array.isArray(action)) { + return dimApi(s, action[0], action[1]); + } + return dimApi(s, action, currentAccount); + }, setupState); + + // Run test-specific checks + checkState(updatedState); + + expect(updatedState.updateQueue.length).toEqual(expectedQueue.length); + let i = 0; + for (const entry of updatedState.updateQueue) { + const { action, payload } = entry; + const { action: expectedAction, payload: expectedPayload } = expectedQueue[i]; + expect(action).toBe(expectedAction); + if (typeof payload === 'string') { + expect(payload).toBe(expectedPayload); + } else { + expect(payload).toMatchObject(expectedPayload); + } + i++; + } + + // Fail all the updates in the queue and make sure they reverse back to the initial state + const reversed = dimApi( + updatedState, + finishedUpdates( + new Array(updatedState.updateInProgressWatermark).fill({ + status: 'Failed', + }), + ), + ); + if (!noReverse) { + expect(reversed).toEqual(setupState); + } + }); + } +}); + +describe('prepareToFlushUpdates', () => { + const cases: { + name: string; + /** + * Actions to run to set the initial state before our test actions. These + * will be "flushed" already. + */ + setupActions?: DimApiAction[]; + /** + * A list of actions to run, followed by prepareToFlushUpdates. The tuple + * option allows specifying a different account than `currentAccount`. + */ + actions: (DimApiAction | [DimApiAction, DestinyAccount])[]; + /** + * After prepareToFlushUpdates, the queue should look as if these actions + * had been run (e.g. reordered, or with multiple updates consolidated). + */ + expectedActions?: (DimApiAction | [DimApiAction, DestinyAccount])[]; + /** + * The expected queue after prepareToFlushUpdates. Use this if you can't + * express the queue state with expectedActions. + */ + expectedQueue?: ProfileUpdateWithRollback[]; + /** + * The expected inProgressWatermark value. If not set it defaults to + * expectedQueue.length. + */ + expectedInProgressWatermark?: number; + }[] = [ + { + name: 'can coalesce settings', + actions: [ + // Turn new items on + setSettingAction('showNewItems', true), + // Modify another setting + setSettingAction('itemSize', 35), + // Turn new items back off + setSettingAction('showNewItems', false), + ], + expectedActions: [ + // The showNewItems setting should cancel out + setSettingAction('itemSize', 35), + ], + }, + { + name: 'can handle multiple profile updates', + actions: [ + // Turn new items on + setSettingAction('showNewItems', true), + // Save a tag for D2 + setItemTag({ itemId: '1234', tag: 'favorite' }), + // Save a tag for D1, same profile + [setItemTag({ itemId: '1231903', tag: 'keep' }), { ...currentAccount, destinyVersion: 1 }], + // Save a tag for D2, same profile + setItemTag({ itemId: '76543', tag: 'junk' }), + ], + expectedInProgressWatermark: 3, // Because the D1 tag is outside the queue + expectedActions: [ + // Turn new items on + setSettingAction('showNewItems', true), + // Save a tag for D2 + setItemTag({ itemId: '1234', tag: 'favorite' }), + // Save a tag for D2, same profile + setItemTag({ itemId: '76543', tag: 'junk' }), + // The D1 tag should be moved to the end + [setItemTag({ itemId: '1231903', tag: 'keep' }), { ...currentAccount, destinyVersion: 1 }], + ], + }, + { + name: 'can handle multiple profile updates with settings last', + actions: [ + // Save a tag for D2 + setItemTag({ itemId: '1234', tag: 'favorite' }), + // Save a tag for D2, same profile + setItemTag({ itemId: '76543', tag: 'junk' }), + // Turn new items on + setSettingAction('showNewItems', true), + ], + // Exactly the same + expectedActions: [ + setItemTag({ itemId: '1234', tag: 'favorite' }), + setItemTag({ itemId: '76543', tag: 'junk' }), + setSettingAction('showNewItems', true), + ], + }, + { + name: 'can handle loadouts', + setupActions: [ + updateLoadout({ + id: '1234', + name: 'before foo', + classType: DestinyClass.Warlock, + items: [], + clearSpace: false, + }), + ], + actions: [ + updateLoadout({ + id: '1234', + name: 'foo', + classType: DestinyClass.Warlock, + items: [], + clearSpace: false, + }), + // Update the name + updateLoadout({ + id: '1234', + name: 'foobar', + classType: DestinyClass.Warlock, + items: [], + clearSpace: false, + }), + deleteLoadout('1234'), + ], + expectedActions: [deleteLoadout('1234')], + }, + { + name: 'can handle setting both tags and notes', + actions: [ + setItemTag({ itemId: '1234', tag: 'favorite' }), + setItemNote({ itemId: '1234', note: 'woohoo' }), + setItemTag({ itemId: '1234', tag: undefined }), + ], + expectedQueue: [ + // Tag and notes are coalesced into a single update { action: 'tag', payload: { id: '1234', + tag: null, notes: 'woohoo', + craftedDate: undefined, }, before: { - id: '1234', - tag: 'favorite', - }, - platformMembershipId: '3456', - destinyVersion: 2, - }, - { - action: 'tag', - payload: { id: '1234', tag: null, + notes: null, }, - before: { - id: '1234', - tag: 'favorite', - notes: 'woohoo', - }, - platformMembershipId: '3456', + platformMembershipId: currentAccount.membershipId, destinyVersion: 2, }, ], - }; - - const updatedState = dimApi(state, prepareToFlushUpdates()); - - expect(updatedState.updateInProgressWatermark).toBe(1); - const expected = [ - { - action: 'tag', - payload: { - id: '1234', - tag: null, - notes: 'woohoo', - }, - before: { - id: '1234', - tag: 'junk', - }, - platformMembershipId: '3456', - destinyVersion: 2, - }, - ]; - expect(updatedState.updateQueue).toEqual(expected); - }); + }, + ]; + + for (const { + name, + actions, + expectedQueue = [], + expectedActions = [], + setupActions = [], + expectedInProgressWatermark = expectedQueue.length + expectedActions.length, + } of cases) { + it(name, () => { + // Apply the setup actions + const setupState = [ + ...setupActions, + prepareToFlushUpdates(), + finishedUpdates( + new Array(setupActions.length).fill({ status: 'Success' }), + ), + ].reduce((s, action) => dimApi(s, action, currentAccount), initialState); + setupState.updateQueue = []; + setupState.updateInProgressWatermark = 0; + + // Apply all the input actions, then prepareToFlushUpdates + const updatedState = [...actions, prepareToFlushUpdates()].reduce((s, action) => { + if (Array.isArray(action)) { + return dimApi(s, action[0], action[1]); + } + return dimApi(s, action, currentAccount); + }, setupState); + expect(updatedState.updateInProgressWatermark).toBe(expectedInProgressWatermark); + + // Generate the expected queue + const resolvedExpectedQueue = expectedQueue.length + ? expectedQueue + : expectedActions.reduce((s, action) => { + if (Array.isArray(action)) { + return dimApi(s, action[0], action[1]); + } + return dimApi(s, action, currentAccount); + }, setupState).updateQueue; + + expect(updatedState.updateQueue).toEqual(resolvedExpectedQueue); + }); + } }); describe('finishedUpdates', () => { it('can mark success', () => { - const state: DimApiState = { - ...initialState, - updateQueue: [ - { - action: 'setting', - payload: { - showNewItems: true, - }, - before: { - showNewItems: false, - }, - }, - // Save a tag for D2 - { - action: 'tag', - payload: { - id: '1234', - tag: 'favorite', - }, - before: { - id: '1234', - }, - platformMembershipId: '3456', - destinyVersion: 2, - }, - ], - updateInProgressWatermark: 2, - }; + let state = dimApi(initialState, setSettingAction('showNewItems', true)); + state = dimApi(state, setItemTag({ itemId: '1234', tag: 'favorite' }), currentAccount); + state = dimApi(state, prepareToFlushUpdates()); + const updatedState = dimApi( state, finishedUpdates([{ status: 'Success' }, { status: 'Success' }]), @@ -635,87 +706,3 @@ describe('finishedUpdates', () => { expect(updatedState.updateQueue).toEqual([]); }); }); - -describe('saveSearch', () => { - it('can save valid queries', () => { - const state: DimApiState = { - ...initialState, - }; - const updatedState = dimApi( - state, - saveSearch({ query: '(is:masterwork) (is:weapon)', saved: true, type: SearchType.Item }), - currentAccount, - ); - - expect(updatedState.searches).toMatchObject({ - [1]: [], - [2]: [{ query: 'is:masterwork is:weapon', saved: true, type: SearchType.Item }], - }); - }); - - it('can unsave valid queries', () => { - const state: DimApiState = { - ...initialState, - }; - let updatedState = dimApi( - state, - saveSearch({ query: '(is:masterwork) (is:weapon)', saved: true, type: SearchType.Item }), - currentAccount, - ); - - updatedState = dimApi( - updatedState, - saveSearch({ query: '(is:masterwork) (is:weapon)', saved: false, type: SearchType.Item }), - currentAccount, - ); - - expect(updatedState.searches).toMatchObject({ - [1]: [], - [2]: [{ query: 'is:masterwork is:weapon', saved: false, type: SearchType.Item }], - }); - }); - - it('does not save invalid queries', () => { - const state: DimApiState = { - ...initialState, - }; - const updatedState = dimApi( - state, - saveSearch({ query: 'deepsight:incomplete', saved: true, type: SearchType.Item }), - currentAccount, - ); - expect(updatedState.searches).toMatchObject({ - [1]: [], - [2]: [], - }); - }); - - it('can unsave invalid queries', () => { - const state: DimApiState = { - ...initialState, - searches: { - [1]: [], - [2]: [ - { - usageCount: 1, - lastUsage: 919191, - saved: true, - query: 'deepsight:incomplete', - type: SearchType.Item, - }, - ], - }, - }; - const updatedState = dimApi( - state, - saveSearch({ query: 'deepsight:incomplete', saved: false, type: SearchType.Item }), - currentAccount, - ); - - // FIXME maybe delete this outright? It'll be cleaned up the next time DIM loads the remote profile anyway... - expect(updatedState.searches).toMatchObject({ - [1]: [], - [2]: [{ usageCount: 1, saved: false, query: 'deepsight:incomplete' }], - }); - }); -}); diff --git a/src/app/dim-api/reducer.ts b/src/app/dim-api/reducer.ts index 6a3ab8cbce..05cc3ad064 100644 --- a/src/app/dim-api/reducer.ts +++ b/src/app/dim-api/reducer.ts @@ -151,7 +151,7 @@ export const initialState: DimApiState = { updateInProgressWatermark: 0, }; -type DimApiAction = +export type DimApiAction = | ActionType | ActionType | ActionType @@ -758,7 +758,10 @@ function compactUpdate( ...existingUpdate.payload, ...update.payload, }, - before: existingUpdate.before, + before: { + ...update.before!, + ...existingUpdate.before!, + }, }; } break; @@ -772,7 +775,10 @@ function compactUpdate( ...existingUpdate.payload, ...update.payload, }, - before: existingUpdate.before, + before: { + ...update.before!, + ...existingUpdate.before!, + }, }; } break; @@ -980,7 +986,7 @@ function setTag( tag: tag ?? null, craftedDate: craftedDate ?? existingTag?.craftedDate, }, - before: existingTag ? { ...existingTag } : undefined, + before: existingTag ? { id: itemId, tag: existingTag.tag ?? null } : { id: itemId, tag: null }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1011,7 +1017,9 @@ function setItemHashTag( hash: itemHash, tag: tag ?? null, }, - before: existingTag ? { ...existingTag } : undefined, + before: existingTag + ? { hash: itemHash, tag: existingTag.tag ?? null } + : { hash: itemHash, tag: null }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1039,10 +1047,12 @@ function setNote( action: 'tag', payload: { id: itemId, - notes: notes && notes.length > 0 ? notes : null, + notes: notes || null, craftedDate: craftedDate ?? existingTag?.craftedDate, }, - before: existingTag ? { ...existingTag } : undefined, + before: existingTag + ? { id: itemId, notes: existingTag.notes || null } + : { id: itemId, notes: null }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1063,9 +1073,11 @@ function setItemHashNote( action: 'item_hash_tag', payload: { hash: itemHash, - notes: notes && notes.length > 0 ? notes : null, + notes: notes || null, }, - before: existingTag ? { ...existingTag } : undefined, + before: existingTag + ? { hash: itemHash, notes: existingTag.notes || null } + : { hash: itemHash, notes: null }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, }; @@ -1189,8 +1201,7 @@ function saveSearch( } query = canonical; - const searches = draft.searches[destinyVersion]; - const existingSearch = searches.find((s) => s.query === query); + const existingSearch = draft.searches[destinyVersion].find((s) => s.query === query); if (!existingSearch && saveable) { // Save this as a "used" search first. This may happen if it's a type of @@ -1236,6 +1247,18 @@ function deleteSearch( query: string, type: SearchType, ) { + const destinyVersion = account.destinyVersion; + // Note: memoized + const filtersMap = buildItemFiltersMap(destinyVersion); + + // Canonicalize the query so we always save it the same way + const { canonical } = parseAndValidateQuery(query, filtersMap, { + customStats: draft.settings.customStats ?? [], + } as FilterContext); + query = canonical; + + const existingSearch = draft.searches[destinyVersion].find((s) => s.query === query); + const updateAction: ProfileUpdateWithRollback = { action: 'delete_search', payload: { @@ -1244,9 +1267,8 @@ function deleteSearch( }, before: { query, + saved: existingSearch?.saved ?? false, type, - // TODO: How to get this?? - saved: false, }, platformMembershipId: account.membershipId, destinyVersion: account.destinyVersion, @@ -1289,7 +1311,7 @@ export function parseProfileKey(profileKey: string): [string, DestinyVersion] { return [match[1], parseInt(match[2], 10) as DestinyVersion]; } -function ensureProfile(draft: Draft, profileKey: string) { +export function ensureProfile(draft: Draft, profileKey: string) { if (!draft.profiles[profileKey]) { draft.profiles[profileKey] = { profileLastLoaded: 0, @@ -1304,10 +1326,6 @@ function ensureProfile(draft: Draft, profileKey: string) { function applyUpdateLocally(draft: Draft, update: ProfileUpdateWithRollback) { switch (update.action) { case 'setting': { - // Intentionally avoiding Object.assign because of immer - // for (const [key, value] of Object.entries(update.payload)) { - // draft.settings[key] = value; - // } Object.assign(draft.settings, update.payload); break; } @@ -1317,15 +1335,17 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith const searches = draft.searches[destinyVersion!]; const existingSearch = searches.find((s) => s.query === query); + const lastUsage = $DIM_FLAVOR === 'test' ? 1000 : Date.now(); + if (existingSearch) { - existingSearch.lastUsage = Date.now(); + existingSearch.lastUsage = lastUsage; existingSearch.usageCount++; } else { searches.push({ query, usageCount: 1, saved: false, - lastUsage: Date.now(), + lastUsage, type, }); } @@ -1367,7 +1387,7 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith if (!existingAnnotation.tag && !existingAnnotation.notes) { delete tags[itemId]; } - } else { + } else if (itemAnnotation.tag || itemAnnotation.notes) { tags[itemId] = itemAnnotation; } break; @@ -1390,7 +1410,7 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith if (!existingAnnotation.tag && !existingAnnotation.notes) { delete tags[itemAnnotation.hash]; } - } else { + } else if (itemAnnotation.tag || itemAnnotation.notes) { tags[itemAnnotation.hash] = itemAnnotation; } break; @@ -1433,6 +1453,7 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith const { destinyVersion } = update; const searches = draft.searches[destinyVersion!]; const existingSearch = searches.find((s) => s.query === query); + const lastUsage = $DIM_FLAVOR === 'test' ? 1000 : Date.now(); // This might not exist if reversing a delete_search if (existingSearch) { @@ -1442,7 +1463,7 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith query, usageCount: 1, saved, - lastUsage: Date.now(), + lastUsage, type: update.payload.type, }); } @@ -1451,10 +1472,7 @@ function applyUpdateLocally(draft: Draft, update: ProfileUpdateWith } } -function reverseUpdateLocally(draft: Draft, update: ProfileUpdateWithRollback) { - if (!update.before) { - return; - } +export function reverseUpdateLocally(draft: Draft, update: ProfileUpdateWithRollback) { try { switch (update.action) { case 'delete_loadout': { @@ -1475,6 +1493,12 @@ function reverseUpdateLocally(draft: Draft, update: ProfileUpdateWi } as ProfileUpdateWithRollback); break; } + case 'search': + // You can't reverse a search usage + break; + case 'tag_cleanup': + // You can't reverse a tag cleanup + break; default: applyUpdateLocally(draft, { ...update, @@ -1488,5 +1512,8 @@ function reverseUpdateLocally(draft: Draft, update: ProfileUpdateWi // next profile load will reset the info. errorLog('reverseUpdateLocally', e, update); reportException('reverseUpdateLocally', e, { update }); + if ($DIM_FLAVOR === 'test') { + throw e; + } } }