-
Notifications
You must be signed in to change notification settings - Fork 2
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
Target by specific countries and/or country group #1271
Conversation
…eaning we can specifically target countries added in logic which will check for the country name and convert it into the country code as well as checking for a country group. hopefully they are an OR statement!
… DB. we convert it to a country code so it can check against the user payload which has the country code Added in a number of unit tests to cover the new logic including the country code converter
… DB. we convert it to a country code so it can check against the user payload which has the country code Added in a number of unit tests to cover the new logic including the country code converter
…cated) locations field as well as the string of arrays for countries
…nge refelct that including removing of mapping from country code to country name Updated unit tests minor renaming so it is clearer
Co-authored-by: Lakshmi R Pillai <[email protected]>
…s! The new function wasn't working so on reworking it is now working as expected
typo fixed in geolocation.ts
src/shared/lib/geolocation.ts
Outdated
return foundCountryGroupId || 'International'; | ||
}; | ||
|
||
//inCountryGroups is a bad name now that it accepts country names separately from country groups |
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.
Shall we rename it then?
Something like inTargetedCountry(...)
perhaps?
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.
Nice suggestion
src/shared/lib/geolocation.ts
Outdated
@@ -623,3 +636,8 @@ export const addRegionIdToSupportUrl = (originalUrl: string, countryCode?: strin | |||
|
|||
return originalUrl; | |||
}; | |||
|
|||
const countryNameToCodeMap: Record<string, string> = {}; |
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.
can this be removed?
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.
Yes it can!
@@ -58,6 +58,23 @@ function canShowAbandonedBasketBanner( | |||
return daysSince(new Date(abandonedBasketBannerLastClosedAt), now) > 0; | |||
} | |||
|
|||
export const isCountryTargetedForBanner = ( |
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.
These functions can be deleted once the migration is complete and locations
is gone, right?
We could avoid all the duplication if this logic happened inside inCountryGroups
, but if it's all going away soon then it's not really an issue!
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.
Yeah exactly. It's really not a very elegant solution throughout but once the locations field is removed we can remove this function and be able to pass inTargetedCountry straight into the selection.
inTargetedCountry(
targeting.countryCode,
test.regionTargeting?.targetedCountryGroups || [],
test.regionTargeting?.targetedCountryCodes || [],
)
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.
Although I think the Epic is slightly different because it's using Filter and is expecting a boolean and not a function so we may have to treat that different but for the other components the above should suffice!
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.
👍 we can do a little refactor later
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.
Sounds like a plan!
removing unused mapping function removing leftover console logs
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!
Co-authored with @LAKSHMIRPILLAI
What does this change?
TL;DR Adds in new logic to enable targeting by individual countries
Previously when we set up tests in the RRCP for epics or banners we can target audiences by region, but we have a new requirement to enable us to target by individual countries, like just Germany.
We have added in a new dropdown in the RRCP [SAC PR link] meaning that users can target by region, individual country or both! This is passed through to the SDC model via the targeted countries field. From here, we then check wether or not to display that epic. The targeted country comes through as country name (like Germany) but in our logic we convert that to the country code (DE) which then, depending on the users location will determine whether or not to show the epic.
We have test cases in geolocations which might help in understanding the newer logic.
For more information, as well as the testing see here
How to test
How can we measure success?
Have we considered potential risks?
Images
Accessibility