-
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): expose logRecommendationOpen action #902
Changes from 8 commits
57bf12f
9bd9667
bc5a919
dd1fdfb
305842c
68fd176
6becfe2
6ec6765
766ede7
40be695
aa005d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,47 +1,95 @@ | ||
import {createMockState} from '../../test'; | ||
import {buildMockResult} from '../../test'; | ||
import {partialDocumentInformation} from './analytics-utils'; | ||
import {createMockRecommendationState} from '../../test/mock-recommendation-state'; | ||
import { | ||
partialDocumentInformation, | ||
partialRecommendationInformation, | ||
} from './analytics-utils'; | ||
|
||
describe('analytics-utils', () => { | ||
it('should extract documentation information with a single author', () => { | ||
const result = buildMockResult(); | ||
result.raw['author'] = 'john'; | ||
describe('#partialDocumentInformation', () => { | ||
it('should extract documentation information with a single author', () => { | ||
const result = buildMockResult(); | ||
result.raw['author'] = 'john'; | ||
|
||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('john'); | ||
}); | ||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('john'); | ||
}); | ||
|
||
it('should extract documentation information with multiple author', () => { | ||
const result = buildMockResult(); | ||
result.raw['author'] = ['john', 'doe']; | ||
it('should extract documentation information with multiple author', () => { | ||
const result = buildMockResult(); | ||
result.raw['author'] = ['john', 'doe']; | ||
|
||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('john;doe'); | ||
}); | ||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('john;doe'); | ||
}); | ||
|
||
it('should extract document information when there is no author', () => { | ||
const result = buildMockResult(); | ||
delete result.raw['author']; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('unknown'); | ||
}); | ||
it('should extract document information when there is no author', () => { | ||
const result = buildMockResult(); | ||
delete result.raw['author']; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).documentAuthor | ||
).toBe('unknown'); | ||
}); | ||
|
||
it('should extract sourceName information from source field', () => { | ||
const result = buildMockResult(); | ||
result.raw.source = 'mysource'; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).sourceName | ||
).toBe('mysource'); | ||
}); | ||
|
||
it('should extract sourceName information when there is no source field', () => { | ||
const result = buildMockResult(); | ||
delete result.raw['source']; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).sourceName | ||
).toBe('unknown'); | ||
}); | ||
|
||
it('when the result is not found in state, the documentPosition is 0', () => { | ||
const result = buildMockResult({uniqueId: '1'}); | ||
const state = createMockState(); | ||
|
||
it('should extract sourceName information from source field', () => { | ||
const result = buildMockResult(); | ||
result.raw.source = 'mysource'; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).sourceName | ||
).toBe('mysource'); | ||
const {documentPosition} = partialDocumentInformation(result, state); | ||
expect(documentPosition).toBe(0); | ||
}); | ||
|
||
it('when the result is found in state, the documentPosition is the index + 1', () => { | ||
const result = buildMockResult({uniqueId: '1'}); | ||
const state = createMockState(); | ||
state.search.response.results = [result]; | ||
|
||
const {documentPosition} = partialDocumentInformation(result, state); | ||
expect(documentPosition).toBe(1); | ||
}); | ||
}); | ||
|
||
it('should extract sourceName information when there is no source field', () => { | ||
const result = buildMockResult(); | ||
delete result.raw['source']; | ||
expect( | ||
partialDocumentInformation(result, createMockState()).sourceName | ||
).toBe('unknown'); | ||
describe('#partialRecommendationInformation', () => { | ||
it('when the recommendation is not found in state, the documentPosition is 0', () => { | ||
const recommendation = buildMockResult({uniqueId: '1'}); | ||
const state = createMockRecommendationState(); | ||
|
||
const {documentPosition} = partialRecommendationInformation( | ||
recommendation, | ||
state | ||
); | ||
expect(documentPosition).toBe(0); | ||
}); | ||
|
||
it('when the recommendation is found in state, the documentPosition is the index + 1', () => { | ||
const recommendation = buildMockResult({uniqueId: '1'}); | ||
const state = createMockRecommendationState(); | ||
state.recommendation.recommendations = [recommendation]; | ||
|
||
const {documentPosition} = partialRecommendationInformation( | ||
recommendation, | ||
state | ||
); | ||
expect(documentPosition).toBe(1); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import {SearchEventResponse} from 'coveo.analytics/dist/definitions/events'; | |
import {AsyncThunkAction, createAsyncThunk} from '@reduxjs/toolkit'; | ||
import {requiredNonEmptyString} from '../../utils/validate-payload'; | ||
import {ThunkExtraArguments} from '../../app/thunk-extra-arguments'; | ||
import {PipelineSection} from '../../state/state-sections'; | ||
import {RecommendationAppState} from '../../state/recommendation-app-state'; | ||
|
||
export enum AnalyticsType { | ||
Search, | ||
|
@@ -117,6 +119,26 @@ export const partialDocumentInformation = ( | |
({uniqueId}) => result.uniqueId === uniqueId | ||
) || 0; | ||
|
||
return buildPartialDocumentInformation(result, resultIndex, state); | ||
}; | ||
|
||
export const partialRecommendationInformation = ( | ||
result: Result, | ||
state: Partial<RecommendationAppState> | ||
): PartialDocumentInformation => { | ||
const resultIndex = | ||
state.recommendation?.recommendations.findIndex( | ||
({uniqueId}) => result.uniqueId === uniqueId | ||
) || 0; | ||
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. Question on this ternary, similar to the one on line 120:
I feel it's weird to send two different values for the same condition of "result not found". 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.
In any case, I guess just keep in mind that no matter what, if we are unable to find the given result unique ID in the list of results, there's not much we can do to make the UA payload "correct", it's most likely going to fail in one manner or the other. And it being 0, or -1, is a small detail versus the fact that there's some code upstream that is completely wrong when calling this function. 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. Alright, I don't feel this is important enough to solve right now. It's definitely niche, but I still think it would be good to addressed, whether through a value, or some kind of feedback to the devs. I've created a jira to revisit. |
||
|
||
return buildPartialDocumentInformation(result, resultIndex, state); | ||
}; | ||
|
||
function buildPartialDocumentInformation( | ||
result: Result, | ||
resultIndex: number, | ||
state: Partial<PipelineSection> | ||
): PartialDocumentInformation { | ||
const collection = result.raw.collection; | ||
const collectionName = | ||
typeof collection === 'string' ? collection : 'default'; | ||
|
@@ -133,7 +155,7 @@ export const partialDocumentInformation = ( | |
sourceName: getSourceName(result), | ||
queryPipeline: state.pipeline || getPipelineInitialState(), | ||
}; | ||
}; | ||
} | ||
|
||
export const documentIdentifier = (result: Result): DocumentIdentifier => { | ||
return { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import {AsyncThunkAction} from '@reduxjs/toolkit'; | ||
import {StateNeededByAnalyticsProvider} from '../../api/analytics/analytics'; | ||
import {Result} from '../../api/search/search/result'; | ||
import {Engine} from '../../app/headless-engine'; | ||
import { | ||
AnalyticsType, | ||
AsyncThunkAnalyticsOptions, | ||
} from '../analytics/analytics-utils'; | ||
import {logRecommendationOpen} from './recommendation-analytics-actions'; | ||
|
||
/** | ||
* The click analytics action creators. | ||
*/ | ||
export interface ClickAnalyticsActionCreators { | ||
/** | ||
* The event to log when a recommendation is selected. | ||
* | ||
* @param result - The selected recommendation. | ||
* @returns A dispatchable action. | ||
*/ | ||
logRecommendationOpen( | ||
recommendation: Result | ||
): AsyncThunkAction< | ||
{ | ||
analyticsType: AnalyticsType.Click; | ||
}, | ||
void, | ||
AsyncThunkAnalyticsOptions<StateNeededByAnalyticsProvider> | ||
>; | ||
} | ||
|
||
/** | ||
* Returns possible click analytics action creators. | ||
* | ||
* @param engine - The headless engine. | ||
* @returns An object holding the action creators. | ||
*/ | ||
export function loadClickAnalyticsActions( | ||
engine: Engine<object> | ||
): ClickAnalyticsActionCreators { | ||
engine.addReducers({}); | ||
|
||
return { | ||
logRecommendationOpen, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,15 @@ | ||
import {Component, ContextType} from 'react'; | ||
import {ResultLink} from '../result-list/result-link'; | ||
import { | ||
buildRecommendationList, | ||
RecommendationList as HeadlessRecommendationList, | ||
RecommendationListOptions, | ||
RecommendationListState, | ||
Result, | ||
Unsubscribe, | ||
} from '@coveo/headless'; | ||
import {loadClickAnalyticsActions} from '@coveo/headless/recommendation'; | ||
import {AppContext} from '../../context/engine'; | ||
import {filterProtocol} from '../../utils/filter-protocol'; | ||
|
||
export class RecommendationList extends Component< | ||
RecommendationListOptions, | ||
|
@@ -37,6 +39,17 @@ export class RecommendationList extends Component< | |
this.setState(this.controller.state); | ||
} | ||
|
||
private logClick(recommendation: Result) { | ||
const engine = this.context.recommendationEngine; | ||
|
||
if (!engine) { | ||
return; | ||
} | ||
|
||
const {logRecommendationOpen} = loadClickAnalyticsActions(engine); | ||
engine.dispatch(logRecommendationOpen(recommendation)); | ||
} | ||
|
||
render() { | ||
if (!this.state) { | ||
return null; | ||
|
@@ -65,9 +78,15 @@ export class RecommendationList extends Component< | |
<article> | ||
<h2> | ||
{/* Make sure to log analytics when the result link is clicked. */} | ||
<ResultLink result={recommendation}> | ||
<a | ||
href={filterProtocol(recommendation.clickUri)} // Filters out dangerous URIs that can create XSS attacks such as `javascript:`. | ||
onClick={() => this.logClick(recommendation)} | ||
onContextMenu={() => this.logClick(recommendation)} | ||
onMouseDown={() => this.logClick(recommendation)} | ||
onMouseUp={() => this.logClick(recommendation)} | ||
> | ||
{recommendation.title} | ||
</ResultLink> | ||
</a> | ||
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. These samples will eventually leverage a |
||
</h2> | ||
<p>{recommendation.excerpt}</p> | ||
</article> | ||
|
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.
Not sure what happened to the package-lock. If you try installing the latest analytics package, is the diff large?
I am using node
16.2.0
, npm7.13.0
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.
There's definitely something wonky with the package locks, I think they need to be updated more often