-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(headless): TriggerRedirection #875
Changes from 37 commits
1467908
6c6615d
036665f
17846c7
f544823
a5ff2ce
af6bd09
04ae168
71e9dd6
5b50a9a
ea483c2
6c8680b
2c078b7
351735a
6c209b6
ffde6d1
2a578d5
bcd9cb8
ec4174a
10ca7af
bcb1d36
9bb98b7
40e68d0
6291827
23a5c7d
ee88fdd
2b056de
52ac917
700a724
792bde7
7417d85
94e0453
85d6b15
99aded8
ab8db94
dd2e378
f72d205
064035c
0eed201
d00a8ac
0a33602
eb5d528
c77e399
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import { | ||
RedirectionTrigger, | ||
buildRedirectionTrigger, | ||
} from './headless-redirection-trigger'; | ||
import {buildMockSearchAppEngine, MockEngine} from '../../test/mock-engine'; | ||
import {SearchAppState} from '../../state/search-app-state'; | ||
import {redirection, configuration} from '../../app/reducers'; | ||
import {logTriggerRedirect} from '../../features/redirection/redirection-analytics-actions'; | ||
|
||
describe('RedirectionTrigger', () => { | ||
let engine: MockEngine<SearchAppState>; | ||
let redirectionTrigger: RedirectionTrigger; | ||
|
||
function initRedirectTrigger() { | ||
redirectionTrigger = buildRedirectionTrigger(engine); | ||
} | ||
|
||
function getLogTriggerRedirectAction() { | ||
return engine.actions.find( | ||
(a) => a.type === logTriggerRedirect.pending.type | ||
); | ||
} | ||
|
||
function registeredListeners() { | ||
return (engine.subscribe as jest.Mock).mock.calls.map((args) => args[0]); | ||
} | ||
|
||
beforeEach(() => { | ||
engine = buildMockSearchAppEngine(); | ||
initRedirectTrigger(); | ||
}); | ||
|
||
it('initializes', () => { | ||
expect(redirectionTrigger).toBeTruthy(); | ||
}); | ||
|
||
it('it adds the correct reducers to the engine', () => { | ||
expect(engine.addReducers).toHaveBeenCalledWith({ | ||
redirection, | ||
configuration, | ||
}); | ||
}); | ||
|
||
it('exposes a #subscribe method', () => { | ||
expect(redirectionTrigger.subscribe).toBeTruthy(); | ||
}); | ||
|
||
it('when the #engine.state.redirection.redirectTo is already initialized, it calls #onRedirect and dispatches #logTriggerRedirect', () => { | ||
const listener = jest.fn(); | ||
engine.state.redirection.redirectTo = 'https://www.google.com'; | ||
redirectionTrigger.subscribe(listener); | ||
|
||
expect(listener).toHaveBeenCalledTimes(1); | ||
expect(getLogTriggerRedirectAction()).toBeTruthy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be possible to avoid logging an analytics event immediately on subscribe if this line was initialized to the value in state instead of null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I get this, we should log and call the listener on subscribe if the redirection state did contain a trigger (not null, or equivalent would be not an empty string) Use cases could be SSR or creating a trigger controller later, after the latest search There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking it should log on state change so headless does not log the same event over and over if someone instantiates multiple controllers. We've said controllers should be instantiated before the first search. Allowing late registration feels inconsistent. UX-wise too, a user would see the results of their search and then suddenly a redirection when the late controller registers? I don't find that very nice, I think it's better for redirection not to work in this situation, a) encouraging devs to adjust their code and b) avoiding questions around why instantiating one controller late "works", whereas others need to be instantiated before the search. |
||
}); | ||
|
||
it('when the #engine.state.redirection.redirectTo is not updated, it does not call #onRedirect and does not dispatch #logTriggerRedirect', () => { | ||
const listener = jest.fn(); | ||
redirectionTrigger.subscribe(listener); | ||
|
||
expect(listener).not.toHaveBeenCalled(); | ||
expect(getLogTriggerRedirectAction()).toBeFalsy(); | ||
}); | ||
|
||
it('when the #engine.state.redirection.redirectTo is updated, it calls #onRedirect and dispatches #logTriggerRedirect', () => { | ||
const listener = jest.fn(); | ||
engine.state.redirection.redirectTo = 'https://www.google.com'; | ||
redirectionTrigger.subscribe(listener); | ||
engine.state.redirection.redirectTo = 'https://www.coveo.com'; | ||
const [firstListener] = registeredListeners(); | ||
firstListener(); | ||
|
||
expect(listener).toHaveBeenCalledTimes(2); | ||
expect(getLogTriggerRedirectAction()).toBeTruthy(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import {Engine} from '../../app/headless-engine'; | ||
import { | ||
ConfigurationSection, | ||
RedirectionSection, | ||
} from '../../state/state-sections'; | ||
import {configuration, redirection} from '../../app/reducers'; | ||
import {buildController, Controller} from '../controller/headless-controller'; | ||
import {loadReducerError} from '../../utils/errors'; | ||
import {logTriggerRedirect} from '../../features/redirection/redirection-analytics-actions'; | ||
|
||
/** | ||
* The `RedirectionTrigger` controller handles redirection actions. | ||
*/ | ||
export interface RedirectionTrigger extends Controller { | ||
/** | ||
* the state of the `RedirectionTrigger` controller. | ||
*/ | ||
state: RedirectionTriggerState; | ||
} | ||
|
||
/** | ||
* A scoped and simplified part of the headless state that is relevant to the `RedirectionTrigger` controller. | ||
*/ | ||
export interface RedirectionTriggerState { | ||
/** | ||
* The url used for the redirection. | ||
*/ | ||
redirectTo: string | null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be possible to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a hard argument from my part but: Its initialized as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initialized it to null for the case where a redirection trigger is received and saved to the engine's state but a redirection-trigger controller has not been initialized yet. Upon its initialization, it would need to immediately deal with that trigger and log it. If we immediately initialize it to the value in state then we would lose that functionality. |
||
} | ||
|
||
/** | ||
* Creates a `RedirectionTrigger` controller instance. | ||
* | ||
* @param engine - The headless engine. | ||
* @returns A `RedirectionTrigger` controller instance. | ||
* */ | ||
export function buildRedirectionTrigger( | ||
engine: Engine<object> | ||
): RedirectionTrigger { | ||
if (!loadRedirectionReducers(engine)) { | ||
throw loadReducerError; | ||
} | ||
|
||
const controller = buildController(engine); | ||
const {dispatch} = engine; | ||
|
||
const getState = () => engine.state; | ||
|
||
let previousRedirectTo: string | null = null; | ||
|
||
return { | ||
...controller, | ||
|
||
subscribe(listener: () => void) { | ||
const strictListener = () => { | ||
const hasChanged = previousRedirectTo !== this.state.redirectTo; | ||
previousRedirectTo = this.state.redirectTo; | ||
|
||
if (hasChanged && this.state.redirectTo) { | ||
listener(); | ||
dispatch(logTriggerRedirect()); | ||
} | ||
}; | ||
strictListener(); | ||
return engine.subscribe(strictListener); | ||
ThibodeauJF marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
|
||
get state() { | ||
return { | ||
redirectTo: getState().redirection.redirectTo, | ||
}; | ||
}, | ||
}; | ||
} | ||
|
||
function loadRedirectionReducers( | ||
engine: Engine<object> | ||
): engine is Engine<RedirectionSection & ConfigurationSection> { | ||
engine.addReducers({configuration, redirection}); | ||
return true; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,26 @@ | ||
import {createReducer} from '@reduxjs/toolkit'; | ||
import {executeSearch} from '../search/search-actions'; | ||
import {checkForRedirection} from './redirection-actions'; | ||
import {getRedirectionInitialState} from './redirection-state'; | ||
import { | ||
Trigger, | ||
TriggerRedirect, | ||
isTriggerRedirect, | ||
} from './../../api/search/trigger'; | ||
|
||
export const redirectionReducer = createReducer( | ||
getRedirectionInitialState(), | ||
(builder) => | ||
builder.addCase(checkForRedirection.fulfilled, (state, action) => { | ||
state.redirectTo = action.payload; | ||
}) | ||
builder | ||
.addCase(checkForRedirection.fulfilled, (state, action) => { | ||
state.redirectTo = action.payload; | ||
}) | ||
.addCase(executeSearch.fulfilled, (state, action) => { | ||
const redirectTriggers: Trigger[] = action.payload.response.triggers.filter( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you don't set the type to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought I had to specify the type, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the triggers feature live in it's own slice? It seems like a single state property now gets it's value from two different api calls ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you thinking of doing one slice for all the triggers or having one slice per type of trigger? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking one slice for all triggers, however it would be a good to spend some time documenting the design, comparing pros and cons of different approaches, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should create a separate slice for triggers. As is, if someone is using a standalone search box and a trigger, if the plan endpoint returns a url, then headless would log a redirection trigger event to UA which would be incorrect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Goind point, agreed, the said slice's state should be split into the triggers types. A con: would the redirection-slice become misleading then, if it's not affected by trigger on the /search call, but only on the /plan call ? |
||
(trigger) => isTriggerRedirect(trigger) | ||
); | ||
state.redirectTo = redirectTriggers.length | ||
? (redirectTriggers[0] as TriggerRedirect).content | ||
: ''; | ||
}) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import {TriggerNotify} from '../../src/api/search/trigger'; | ||
|
||
export function buildMockTriggerNotify( | ||
config: Partial<TriggerNotify> = {} | ||
): TriggerNotify { | ||
return { | ||
type: 'notify', | ||
content: '', | ||
...config, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import {TriggerQuery} from '../../src/api/search/trigger'; | ||
|
||
export function buildMockTriggerQuery( | ||
config: Partial<TriggerQuery> = {} | ||
): TriggerQuery { | ||
return { | ||
type: 'query', | ||
content: '', | ||
...config, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import {TriggerRedirect} from '../../src/api/search/trigger'; | ||
|
||
export function buildMockTriggerRedirect( | ||
config: Partial<TriggerRedirect> = {} | ||
): TriggerRedirect { | ||
return { | ||
type: 'redirect', | ||
content: '', | ||
...config, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!