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

fix(headless): stop sending invalid actionCause for event protocol #4698

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexprudhomme
Copy link
Contributor

https://coveord.atlassian.net/browse/KIT-3748

Stop sending to EP events that are not in that list as they are invalid. This ended up taking 20 minutes so I just did it now.

@@ -23,10 +23,6 @@ export enum SearchPageEvents {
* Identifies the search event that gets logged when a submit button is selected on a search box.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I just removed a bunch of actions that were not used.

Comment on lines -53 to -56
export const nullActionCause = (): SearchAction => ({
actionCause: null as unknown as string,
});

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 realized that instead of sending null we can simply not send any actionCause by doing executeSearch({legacy:...}) only and not executeSearch({legacy:..., next:...})

Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 241.3 241.3 0
commerce 346 346 0
search 412.9 412.4 -0.1
insight 403.6 403.5 0
recommendation 253.5 253.5 0
ssr 406.4 405.9 -0.1
ssr-commerce 358.2 358.2 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

@lvu285
Copy link
Contributor

lvu285 commented Nov 21, 2024

Great job! I'm bit curious, after this, how we could validate if these events are valid or invalid ?

@alexprudhomme
Copy link
Contributor Author

Great job! I'm bit curious, after this, how we could validate if these events are valid or invalid ?

If they are not in that list, they will be flagged as invalid. https://github.com/coveo-platform/analytics_schema/blob/de502f0877eeaae8e523c78a08e92222f8b26eee/schemas/common/types.json#L267-L292

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Nice job!

@louis-bompart
Copy link
Collaborator

Asking for @erocheleau or @mmitiche review for "insight stuff" review.

@lvu285
Copy link
Contributor

lvu285 commented Nov 22, 2024

Ok kk. So I assume we are not able to know until it goes to Data-platform.
(it's not urgent, you don't have to do it now, maybe next quarter) Can you please help to make sure after the fix is deployed, there is no more invalid flagged by data-platform? Or at least identify if there is anything else, maybe a jira to follow up later 🤔

Great job! I'm bit curious, after this, how we could validate if these events are valid or invalid ?

If they are not in that list, they will be flagged as invalid. https://github.com/coveo-platform/analytics_schema/blob/de502f0877eeaae8e523c78a08e92222f8b26eee/schemas/common/types.json#L267-L292

@mmitiche
Copy link
Contributor

mmitiche commented Nov 22, 2024

We need to update the test files under src/integration-tests/** accordingly, they are not filling becuse they are being excluded here: https://github.com/coveo/ui-kit/blob/4ccf316335e28c784e52b18d3f0c29eeddf124ba/packages/headless/package.json#L132 but they are using the functions that were deleted in this PR.

out of curiosity, what's the reason behind excluding these tests under src/integration-tests/**?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants