Skip to content
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

Merged
merged 43 commits into from
Jun 21, 2021
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
1467908
added triggers to search-response interface
nbuissoncoveo Jun 8, 2021
6c6615d
added executeSeearch action to redirection slice
nbuissoncoveo Jun 8, 2021
036665f
initial version of controller
nbuissoncoveo Jun 9, 2021
17846c7
refactored and removed initial state
nbuissoncoveo Jun 9, 2021
f544823
extracted state from engine
nbuissoncoveo Jun 9, 2021
a5ff2ce
added check to make sure response contains redirect triggers
nbuissoncoveo Jun 9, 2021
af6bd09
called onRedirect when response contains a redirect trigger
nbuissoncoveo Jun 9, 2021
04ae168
log trigger to analytics
nbuissoncoveo Jun 9, 2021
71e9dd6
fixed documentation
nbuissoncoveo Jun 10, 2021
5b50a9a
checking length of array
nbuissoncoveo Jun 10, 2021
ea483c2
added subscribe
nbuissoncoveo Jun 10, 2021
6c8680b
fixed build
nbuissoncoveo Jun 10, 2021
2c078b7
added lodash.memoize package to run local headless test
nbuissoncoveo Jun 10, 2021
351735a
added tests for redirection-slice
nbuissoncoveo Jun 10, 2021
6c209b6
added lodash.memoize to package-lock.json
nbuissoncoveo Jun 10, 2021
ffde6d1
Delete package-lock.json
nbuissoncoveo Jun 10, 2021
2a578d5
Delete package.json
nbuissoncoveo Jun 10, 2021
bcd9cb8
Revert "Delete package.json"
nbuissoncoveo Jun 10, 2021
ec4174a
Revert "Delete package-lock.json"
nbuissoncoveo Jun 10, 2021
10ca7af
Revert "Revert "Delete package-lock.json""
nbuissoncoveo Jun 10, 2021
bcb1d36
Revert "Revert "Revert "Delete package-lock.json"""
nbuissoncoveo Jun 10, 2021
9bb98b7
removed lodash.memoize from package.json files
nbuissoncoveo Jun 10, 2021
40e68d0
removed options and props + initial version of controller test
nbuissoncoveo Jun 14, 2021
6291827
controller state not updating correctly
nbuissoncoveo Jun 14, 2021
23a5c7d
fixed action dispatch
nbuissoncoveo Jun 14, 2021
ee88fdd
still state issue
nbuissoncoveo Jun 14, 2021
2b056de
subscribe doesn't get called when controller state changes?
nbuissoncoveo Jun 15, 2021
52ac917
same issue
nbuissoncoveo Jun 15, 2021
700a724
unit test controller
nbuissoncoveo Jun 16, 2021
792bde7
previousRedirectTo instantiated to null
nbuissoncoveo Jun 16, 2021
7417d85
added controller export to index file
nbuissoncoveo Jun 16, 2021
94e0453
initial versions of react samples
nbuissoncoveo Jun 16, 2021
85d6b15
added instruction to README
nbuissoncoveo Jun 16, 2021
99aded8
added react samples
nbuissoncoveo Jun 16, 2021
ab8db94
Merge branch 'master' into KIT-548
nbuissoncoveo Jun 16, 2021
dd2e378
fixed typo index file
nbuissoncoveo Jun 16, 2021
f72d205
ts narrowing getter bug
nbuissoncoveo Jun 17, 2021
064035c
migrated to trigger slice
nbuissoncoveo Jun 18, 2021
0eed201
created trigger section for engine
nbuissoncoveo Jun 18, 2021
d00a8ac
fixed react samples
nbuissoncoveo Jun 18, 2021
0a33602
adjustements
nbuissoncoveo Jun 21, 2021
eb5d528
fixed test
nbuissoncoveo Jun 21, 2021
c77e399
merge master
nbuissoncoveo Jun 21, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/headless/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ To run the unit tests and watch, run:
npm run test:watch
```

To use @coveo/headless locally, you have to build the package by running:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!


```bash
npm run build
```

### Redux

The headless project intensively uses ["Redux"](https://redux.js.org) along with ["Redux Toolkit"](https://redux-toolkit.js.org), so prior knowledge is necessary.
Expand Down
2 changes: 2 additions & 0 deletions packages/headless/src/api/search/search/search-response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {AnyFacetResponse} from '../../../features/facets/generic/interfaces/gene
import {QueryRankingExpression} from './query-ranking-expression';
import {SecurityIdentity} from './security-identity';
import {ExecutionReport} from './execution-report';
import {Trigger} from './../trigger';
import {QuestionsAnswers} from './question-answering';

export interface SearchResponseSuccess {
Expand All @@ -16,6 +17,7 @@ export interface SearchResponseSuccess {
totalCountFiltered: number;
facets: AnyFacetResponse[];
queryCorrections: QueryCorrection[];
triggers: Trigger[];
questionAnswer: QuestionsAnswers;
}

Expand Down
6 changes: 6 additions & 0 deletions packages/headless/src/controllers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,12 @@ export {
buildFoldedResultList,
} from './folded-result-list/headless-folded-result-list';

export {
RedirectionTrigger,
RedirectionTriggerState,
buildRedirectionTrigger,
} from './triggers/headless-redirection-trigger';

export {
SmartSnippet,
SmartSnippetState,
Expand Down
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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@ThibodeauJF ThibodeauJF Jun 18, 2021

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be possible to remove null from the type, and always initialize as a string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a hard argument from my part but: Its initialized as null in the slice/state as well, and checking against undefined/null is easier than for empty strings

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
},

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
Expand Up @@ -15,6 +15,13 @@ import {SearchAppState} from '../../state/search-app-state';
import pino from 'pino';
import {validatePayloadAndThrow} from '../../utils/validate-payload';
import {buildMockSearchAPIClient} from '../../test/mock-search-api-client';
import {buildMockSearchResponse} from '../../test/mock-search-response';
import {buildMockSearch} from '../../test/mock-search';
import {buildMockTriggerRedirect} from '../../test/mock-trigger-redirect';
import {buildMockTriggerNotify} from '../../test/mock-trigger-notify';
import {buildMockTriggerQuery} from '../../test/mock-trigger-query';
import {executeSearch} from '../search/search-actions';
import {logSearchboxSubmit} from '../query/query-analytics-actions';

describe('redirection slice', () => {
it('should have initial state', () => {
Expand Down Expand Up @@ -117,4 +124,66 @@ describe('redirection slice', () => {
expect(getLogTriggerRedirectAction()).toBeTruthy();
done();
});

it('when a executeSearch fulfilled is received and the payload does not contain any Trigger objects, it does not update `state.redirectTo`', () => {
const state = getRedirectionInitialState();
const response = buildMockSearchResponse();
const searchState = buildMockSearch({
response,
});

const action = executeSearch.fulfilled(
searchState,
'',
logSearchboxSubmit()
);
const finalState = redirectionReducer(state, action);

expect(finalState.redirectTo).toEqual('');
});

it('when a executeSearch fulfilled is received and the payload does not contain any TriggerRedirect objects, it does not update `state.redirectTo`', () => {
const state = getRedirectionInitialState();
const triggers = [
buildMockTriggerNotify({content: 'notification'}),
buildMockTriggerQuery({content: 'query'}),
];
const response = buildMockSearchResponse({
triggers,
});
const searchState = buildMockSearch({
response,
});

const action = executeSearch.fulfilled(
searchState,
'',
logSearchboxSubmit()
);
const finalState = redirectionReducer(state, action);

expect(finalState.redirectTo).toEqual('');
});

it('when a executeSearch fulfilled is received and the payload contains TriggerRedirect objects, it updates `state.redirectTo`', () => {
const state = getRedirectionInitialState();
const triggers = [
buildMockTriggerRedirect({content: 'https://www.coveo.com'}),
];
const response = buildMockSearchResponse({
triggers,
});
const searchState = buildMockSearch({
response,
});

const action = executeSearch.fulfilled(
searchState,
'',
logSearchboxSubmit()
);
const finalState = redirectionReducer(state, action);

expect(finalState.redirectTo).toEqual('https://www.coveo.com');
});
});
21 changes: 18 additions & 3 deletions packages/headless/src/features/redirection/redirection-slice.ts
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you don't set the type to Trigger, will it take on the TriggerRedirect type?

Copy link
Contributor Author

@nbuissoncoveo nbuissoncoveo Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had to specify the type, any does not seem to work. isTriggerRedirect only accepts params of the type Trigger so I don't see how I would be able to filter for only TriggerRedirect objects.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 (/plan and /search). I think we should separate the two concerns,

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

@samisayegh samisayegh Jun 10, 2021

Choose a reason for hiding this comment

The 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,

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
: '';
})
);
1 change: 1 addition & 0 deletions packages/headless/src/features/search/search-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export function getSearchInitialState(): SearchState {
totalCountFiltered: 0,
facets: [],
queryCorrections: [],
triggers: [],
questionAnswer: emptyQuestionAnswer(),
},
duration: 0,
Expand Down
1 change: 1 addition & 0 deletions packages/headless/src/test/mock-search-response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export function buildMockSearchResponse(
totalCountFiltered: 0,
facets: [],
queryCorrections: [emptyCorrection()],
triggers: [],
questionAnswer: emptyQuestionAnswer(),
...config,
};
Expand Down
11 changes: 11 additions & 0 deletions packages/headless/src/test/mock-trigger-notify.ts
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,
};
}
11 changes: 11 additions & 0 deletions packages/headless/src/test/mock-trigger-query.ts
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,
};
}
11 changes: 11 additions & 0 deletions packages/headless/src/test/mock-trigger-redirect.ts
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,
};
}
Loading