diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 136e099e6..dcb41d614 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,6 +1,5 @@ class Api::V1::AccountsController < Api::BaseController include AuthHelper - include CacheKeysHelper skip_before_action :authenticate_user!, :set_current_user, :handle_with_exception, only: [:create], raise: false @@ -52,7 +51,7 @@ class Api::V1::AccountsController < Api::BaseController def cache_keys expires_in 10.seconds, public: false, stale_while_revalidate: 5.minutes - render json: { cache_keys: cache_keys_for_account }, status: :ok + render json: { cache_keys: @account.cache_keys }, status: :ok end def update @@ -97,14 +96,6 @@ class Api::V1::AccountsController < Api::BaseController raise CustomExceptions::Account::InvalidParams.new({}) end - def cache_keys_for_account - { - label: fetch_value_for_key(params[:id], Label.name.underscore), - inbox: fetch_value_for_key(params[:id], Inbox.name.underscore), - team: fetch_value_for_key(params[:id], Team.name.underscore) - } - end - def fetch_account @account = current_user.accounts.find(params[:id]) @current_account_user = @account.account_users.find_by(user_id: current_user.id) diff --git a/app/javascript/dashboard/api/CacheEnabledApiClient.js b/app/javascript/dashboard/api/CacheEnabledApiClient.js index 9af939c00..420a1d64e 100644 --- a/app/javascript/dashboard/api/CacheEnabledApiClient.js +++ b/app/javascript/dashboard/api/CacheEnabledApiClient.js @@ -5,7 +5,22 @@ import ApiClient from './ApiClient'; class CacheEnabledApiClient extends ApiClient { constructor(resource, options = {}) { super(resource, options); - this.dataManager = new DataManager(this.accountIdFromRoute); + this.accountDataManager = null; + } + + // These clients are module level singletons, so they are constructed before the router + // has settled on an account: a boot at /app redirects to /app/accounts/:id without + // re-evaluating the bundle. Resolving the store once in the constructor therefore pointed + // every account at a single `cw-store-` database, where a shared default cache key could + // hand one account another's rows. The account has to come from the route at read time. + get dataManager() { + const accountId = this.accountIdFromRoute; + + if (this.accountDataManager?.accountId !== accountId) { + this.accountDataManager = new DataManager(accountId); + } + + return this.accountDataManager; } // eslint-disable-next-line class-methods-use-this @@ -69,7 +84,7 @@ class CacheEnabledApiClient extends ApiClient { try { await this.dataManager.initDb(); - this.dataManager.replace({ + await this.dataManager.replace({ modelName: this.cacheModelName, data: this.extractDataFromResponse(response), }); diff --git a/app/javascript/dashboard/api/cannedResponse.js b/app/javascript/dashboard/api/cannedResponse.js index 1299a395c..fbe580653 100644 --- a/app/javascript/dashboard/api/cannedResponse.js +++ b/app/javascript/dashboard/api/cannedResponse.js @@ -1,17 +1,24 @@ -/* global axios */ +import CacheEnabledApiClient from './CacheEnabledApiClient'; -import ApiClient from './ApiClient'; - -class CannedResponse extends ApiClient { +class CannedResponse extends CacheEnabledApiClient { constructor() { super('canned_responses', { accountScoped: true }); } - get({ searchKey, signal } = {}) { - return axios.get(this.url, { - params: searchKey ? { search: searchKey } : undefined, - signal, - }); + // eslint-disable-next-line class-methods-use-this + get cacheModelName() { + return 'canned_response'; + } + + // The index endpoint returns a bare array instead of a payload wrapper + // eslint-disable-next-line class-methods-use-this + extractDataFromResponse(response) { + return response.data; + } + + // eslint-disable-next-line class-methods-use-this + marshallData(dataToParse) { + return { data: dataToParse }; } } diff --git a/app/javascript/dashboard/api/specs/CacheEnabledApiClient.spec.js b/app/javascript/dashboard/api/specs/CacheEnabledApiClient.spec.js new file mode 100644 index 000000000..6518226c1 --- /dev/null +++ b/app/javascript/dashboard/api/specs/CacheEnabledApiClient.spec.js @@ -0,0 +1,223 @@ +// The subclasses below are test doubles standing in for the two response shapes the +// cached clients have to support +/* eslint-disable max-classes-per-file */ +import axios from 'axios'; +import { deleteDB } from 'idb'; +import CacheEnabledApiClient from '../CacheEnabledApiClient'; + +global.axios = axios; +vi.mock('axios'); + +const ACCOUNT_ID = '7'; + +// Most cached resources answer with a `payload` wrapper +class WrappedClient extends CacheEnabledApiClient { + constructor() { + super('inboxes', { accountScoped: true }); + } + + // eslint-disable-next-line class-methods-use-this + get cacheModelName() { + return 'inbox'; + } +} + +// Canned responses answer with a bare array, so they override both hooks. The cached read +// has to hand back the same shape as the network read or callers break on a cache hit. +class BareArrayClient extends CacheEnabledApiClient { + constructor() { + super('canned_responses', { accountScoped: true }); + } + + // eslint-disable-next-line class-methods-use-this + get cacheModelName() { + return 'canned_response'; + } + + // eslint-disable-next-line class-methods-use-this + extractDataFromResponse(response) { + return response.data; + } + + // eslint-disable-next-line class-methods-use-this + marshallData(dataToParse) { + return { data: dataToParse }; + } +} + +describe('CacheEnabledApiClient', () => { + const inboxes = [ + { id: 1, name: 'inbox-1' }, + { id: 2, name: 'inbox-2' }, + ]; + const cannedResponses = [{ id: 1, short_code: 'hello', content: 'Hi there' }]; + + let openClients = []; + + const stubEndpoints = ({ cacheKeys, payload }) => { + axios.get.mockImplementation(url => + url.includes('cache_keys') + ? Promise.resolve({ data: { cache_keys: cacheKeys } }) + : Promise.resolve({ data: payload }) + ); + }; + + const listRequests = () => + axios.get.mock.calls.filter(([url]) => !url.includes('cache_keys')); + + const buildClient = Klass => { + const client = new Klass(); + openClients.push(client); + return client; + }; + + beforeEach(async () => { + window.history.pushState({}, '', `/app/accounts/${ACCOUNT_ID}/dashboard`); + await deleteDB(`cw-store-${ACCOUNT_ID}`); + openClients = []; + }); + + afterEach(() => { + openClients.forEach(client => client.dataManager.db?.close()); + }); + + it('throws when a subclass does not declare a cache model', () => { + class Incomplete extends CacheEnabledApiClient {} + + expect(() => buildClient(Incomplete).cacheModelName).toThrow(); + }); + + it('does not serve one account its data to another account', async () => { + // The bundle boots at /app and the router redirects into an account without + // re-evaluating it, so the singleton client outlives the account in the route + window.history.pushState({}, '', '/app'); + const client = buildClient(BareArrayClient); + await deleteDB('cw-store-'); + await deleteDB('cw-store-9'); + + // A never-written cache key is the same '0000000000' for every account + const sharedKey = '0000000000'; + stubEndpoints({ + cacheKeys: { canned_response: sharedKey }, + payload: cannedResponses, + }); + window.history.pushState({}, '', `/app/accounts/${ACCOUNT_ID}/dashboard`); + await client.get(true); + client.dataManager.db?.close(); + + const otherAccountResponses = [ + { id: 99, short_code: 'other-account', content: 'Should stay private' }, + ]; + window.history.pushState({}, '', '/app/accounts/9/dashboard'); + axios.get.mockClear(); + stubEndpoints({ + cacheKeys: { canned_response: sharedKey }, + payload: otherAccountResponses, + }); + const response = await client.get(true); + + expect(response.data).toEqual(otherAccountResponses); + expect(listRequests()).toHaveLength(1); + }); + + it('skips the cache entirely when asked for a network read', async () => { + stubEndpoints({ + cacheKeys: { inbox: 'key-1' }, + payload: { payload: inboxes }, + }); + const client = buildClient(WrappedClient); + + const response = await client.get(false); + + expect(response.data.payload).toEqual(inboxes); + // No cache key lookup either, so the request count is exactly one + expect(axios.get).toHaveBeenCalledTimes(1); + }); + + it('fetches from the network and stores locally when the cache is cold', async () => { + stubEndpoints({ + cacheKeys: { inbox: 'key-1' }, + payload: { payload: inboxes }, + }); + const client = buildClient(WrappedClient); + + const response = await client.get(true); + + expect(response.data.payload).toEqual(inboxes); + expect(listRequests()).toHaveLength(1); + expect(await client.dataManager.get({ modelName: 'inbox' })).toEqual( + inboxes + ); + expect(await client.dataManager.getCacheKey('inbox')).toBe('key-1'); + }); + + it('serves local data without a list request while the cache key is unchanged', async () => { + stubEndpoints({ + cacheKeys: { inbox: 'key-1' }, + payload: { payload: inboxes }, + }); + const client = buildClient(WrappedClient); + await client.get(true); + + axios.get.mockClear(); + const response = await client.get(true); + + expect(response.data.payload).toEqual(inboxes); + expect(listRequests()).toHaveLength(0); + }); + + it('refetches and overwrites local data once the cache key moves', async () => { + stubEndpoints({ + cacheKeys: { inbox: 'key-1' }, + payload: { payload: inboxes }, + }); + const client = buildClient(WrappedClient); + await client.get(true); + + const renamed = [{ id: 1, name: 'inbox-1-renamed' }]; + axios.get.mockClear(); + stubEndpoints({ + cacheKeys: { inbox: 'key-2' }, + payload: { payload: renamed }, + }); + const response = await client.get(true); + + expect(response.data.payload).toEqual(renamed); + expect(listRequests()).toHaveLength(1); + expect(await client.dataManager.get({ modelName: 'inbox' })).toEqual( + renamed + ); + expect(await client.dataManager.getCacheKey('inbox')).toBe('key-2'); + }); + + it('returns the same shape from the cache as from the network for a bare array resource', async () => { + stubEndpoints({ + cacheKeys: { canned_response: 'key-1' }, + payload: cannedResponses, + }); + const client = buildClient(BareArrayClient); + + const fromNetwork = await client.get(true); + axios.get.mockClear(); + const fromCache = await client.get(true); + + expect(fromNetwork.data).toEqual(cannedResponses); + expect(fromCache.data).toEqual(cannedResponses); + expect(listRequests()).toHaveLength(0); + }); + + it('falls back to the network when IndexedDB is unavailable', async () => { + stubEndpoints({ + cacheKeys: { inbox: 'key-1' }, + payload: { payload: inboxes }, + }); + const client = buildClient(WrappedClient); + vi.spyOn(client.dataManager, 'initDb').mockRejectedValue( + new Error('IndexedDB is disabled in private mode') + ); + + const response = await client.get(true); + + expect(response.data.payload).toEqual(inboxes); + }); +}); diff --git a/app/javascript/dashboard/components/widgets/conversation/CannedResponse.vue b/app/javascript/dashboard/components/widgets/conversation/CannedResponse.vue index 2fa2b17fd..0fb542eab 100644 --- a/app/javascript/dashboard/components/widgets/conversation/CannedResponse.vue +++ b/app/javascript/dashboard/components/widgets/conversation/CannedResponse.vue @@ -1,9 +1,8 @@