Skip to content

Fix schema filter name collision bug #1036

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Fix schema filter name collision bug #1036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77448ad
Check filters by creation name instead of current
danieljackins Feb 8, 2024
413f97c
changeset
danieljackins Feb 8, 2024
0d4e6a7
add test
danieljackins Feb 12, 2024
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
5 changes: 5 additions & 0 deletions .changeset/spicy-fireants-reflect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': patch
---

Fix schema-filter bug
45 changes: 43 additions & 2 deletions packages/browser/src/plugins/schema-filter/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const settings: LegacySettings = {
'Braze Web Mode (Actions)': {},
// note that Fullstory's name here doesn't contain 'Actions'
Fullstory: {},
'Google Analytics': {},
'Google Analytics 4 Web': {},
'Segment.io': {},
},
remotePlugins: [
Expand All @@ -35,7 +37,7 @@ const settings: LegacySettings = {
{
// note that Fullstory name contains 'Actions'
name: 'Fullstory (Actions)',
creationName: 'Fullstory (Actions)',
creationName: 'Fullstory',
libraryName: 'fullstoryDestination',
url: 'https://cdn.segment.com/next-integrations/actions/fullstory/35ea1d304f85f3306f48.js',
settings: {
Expand All @@ -49,6 +51,19 @@ const settings: LegacySettings = {
],
},
},
{
name: 'Google Analytics 4 Web',
creationName: 'Google Analytics 4 Web',
libraryName: 'google-analytics-4-webDestination',
url: 'https://cdn.segment.com/next-integrations/actions/google-analytics-4-web/bfab87631cbcb7d70964.js',
settings: {
subscriptions: [
{
partnerAction: 'Custom Event',
},
],
},
},
],
}

Expand Down Expand Up @@ -92,6 +107,11 @@ const fullstory: Plugin = {
name: 'Fullstory (Actions) trackEvent',
}

const ga4: Plugin = {
...trackEvent,
name: 'Google Analytics 4 Web Custom Event',
}

describe('schema filter', () => {
let options: SegmentioSettings
let filterXt: Plugin
Expand All @@ -113,6 +133,7 @@ describe('schema filter', () => {
jest.spyOn(updateUserProfile, 'track')
jest.spyOn(amplitude, 'track')
jest.spyOn(fullstory, 'track')
jest.spyOn(ga4, 'track')
})

describe('plugins and destinations', () => {
Expand Down Expand Up @@ -472,7 +493,7 @@ describe('schema filter', () => {
expect(updateUserProfile.track).toHaveBeenCalled()
})

it('covers different names between remote plugins and integrations', async () => {
it('works when current name differs from creation name', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test that would have failed without this fix? 2 integrations where the 2nd one's name starts with the entirety of the 1st one?

const filterXt = schemaFilter(
{
hi: {
Expand Down Expand Up @@ -516,5 +537,25 @@ describe('schema filter', () => {
expect(updateUserProfile.track).toHaveBeenCalled()
expect(fullstory.track).toHaveBeenCalled()
})

it('doesnt block destinations with similar names', async () => {
const filterXt = schemaFilter(
{
hi: {
enabled: true,
integrations: {
'Google Analytics': false,
},
},
},
settings
)

await ajs.register(segment, ga4, filterXt)

await ajs.track('hi')

expect(ga4.track).toHaveBeenCalled()
})
})
})
2 changes: 1 addition & 1 deletion packages/browser/src/plugins/schema-filter/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function disabledActionDestinations(
const disabledRemotePlugins: string[] = []
;(settings.remotePlugins ?? []).forEach((p: RemotePlugin) => {
disabledIntegrations.forEach((int) => {
if (p.name.includes(int) || int.includes(p.name)) {
if (p.creationName == int) {
disabledRemotePlugins.push(p.name)
}
})
Expand Down