feat: add cohorts for A/B testing in Specs List Banners #23735
Conversation
|
Thanks for taking the time to open a PR!
|
warrensplayer
changed the title
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
packages/frontend-shared/src/composables/examples/UseCohortsExample.vue
Outdated
Show resolved
Hide resolved
packages/frontend-shared/src/composables/examples/UseCohortsExample.vue
Outdated
Show resolved
Hide resolved
packages/frontend-shared/src/composables/examples/UseCohortsExample.cy.tsx
Outdated
Show resolved
Hide resolved
|
Another thought: we have a lot of complicated manual testing for most ACI PRs. I don't see any reason we cannot invest a bit of time into some better testing infra, just in the runner - we can add some endpoints to update the cache or app data if needed, and some to force the app to refetch the data. I don't see much other than "re-open the app" here we can't automate. Just an idea, it's possible there is technical blockers I'm not aware of. |
|
Reactor updates to fix race condition The refactor also pushed the logic to choose the cohort to the data-context layer. This is encapsulated in the |
| return useCohorts(cohortConfig) | ||
| } | ||
|
|
||
| const cohorts = reactive({} as Record<'login' | 'connectProject' | 'organization', any>) |
There was a problem hiding this comment.
I have some questions about this (sorry I only just noticed as part of the re-review...)
This type and usage felt a little off. I found out why - we have a kind of "double reactivity" here. We are using reactive to hold properties which all point to Ref (which is also part of the reactivity system).
It's similar to doing something like
const [foo1, setFoo1] = useState()
const [foo1, setFoo1] = useState()
const [foos, setFoos] = useState()
setFoos({ setFoo1, setFoo2 })I'm not sure if this really makes sense. I tried a few things, and landed on https://github.com/cypress-io/cypress/compare/stokes/23083_aci_cohorts...lmiller/stokes/23083_aci_cohorts-v1?expand=1.
type BannerType = 'login' | 'connectProject' | 'organization'
const cohorts: Partial<Record<BannerType, ReturnType<typeof getCohortForBanner>>> = {}Alternatively, just have 3 separate variables:
const cohortLogin = ref<ReturnType<typeof getCohortForBanner> | undefined>()
// ...This might be more simple - avoid thinking about the reactivity problem all together.
I don't know if cohorts actually needs to be reactive. Either way, this change revealed a bug - getCohortForBanner which returns useCohorts which returns ref<CohortOption | undefined>(). I got a TS error, which I fixed by doing an additional null check (see above link).
The properties (cohorts.login etc) are nullable - but we are passing them to <ConnectProjectBanner /> etc, which has a non-nullable prop type. I think we need this null check.
We missed this because of the as Record<any> cast. Can a user be in no cohort? If they are, I don't know if we'd want to display the modals.
Not sure if this actual manifested in a functional bug, but definitely worth clarifying + improving the types (ideally avoiding any).
There was a problem hiding this comment.
I do not believe there is a double reactivity problem because Vue unwraps refs in side reactive. See: https://vuejs.org/guide/essentials/reactivity-fundamentals.html#ref-unwrapping-in-reactive-objects
Also, I do still need the cohorts to be reactive because of the mutation it uses is async. I was following this pattern for my composable: https://www.vuemastery.com/blog/coding-better-composables-5-of-5/
The problem with using reactive and trying to strongly type the value part of the Record is the unwrapping the reactive does. You can assign either a ref or the unwrapped value any place inside a reactive object, but would always access it as unwrapped.
I liked using reactive because of the unwrapping so you would not have to do all the .value calls in the template. I thought about the separate variables as well, but they would have to be double nested refs or just be defined with let and set after the getCohortForBanner returns.
I am checking in an update that looks pretty ugly, but does seem to work that defines the cohorts variable like:
type BannerType = 'login' | 'connectProject' | 'organization'
const cohorts: Partial<Record<BannerType, CohortOption | Ref<CohortOption | undefined>>> = reactive({})
Let me know what you think.
There was a problem hiding this comment.
Nevermind.... Went back to the any for the moment since that did introduce another type check issue.
There was a problem hiding this comment.
Right I see why cohorts needs to be reactive now - the refs are initially undefined and (possibly) assigned later on.
I think the original problem still stands doesn't it? By using any are we missing out on some value typescript static checking?
I won't hold this one up too much more - I could be missing something, I thought this would be straight forward to type, but maybe that's not the case.
There was a problem hiding this comment.
Ended up going with your answer but just left off the last part of the null check since the CohortOption, if present, will always have a cohort
packages/frontend-shared/src/composables/examples/UseCohortsExample.cy.tsx
Outdated
Show resolved
Hide resolved
| const cumulativeWeights = _.reduce(weights, (result: number[], value: number) => { | ||
| if (result.length === 0) { | ||
| result.push(value) | ||
| } else { | ||
| // @ts-ignores | ||
| result.push(value + result[result.length - 1]) | ||
| } | ||
|
|
||
| return result | ||
| }, []) | ||
|
|
||
| // @ts-ignores | ||
| const randomNumber = Math.random() * cumulativeWeights[cumulativeWeights.length - 1] | ||
|
|
||
| const choice = _.transform(cumulativeWeights, (result, value, index) => { | ||
| if (value >= randomNumber) { | ||
| result.chosenIndex = index | ||
| } | ||
|
|
||
| return result.chosenIndex === -1 | ||
| }, { chosenIndex: -1 }) | ||
|
|
||
| return values[choice.chosenIndex] | ||
| } |
There was a problem hiding this comment.
Any ideas for not needing the two @ts-ignores I added for referencing the last value in an array?
There was a problem hiding this comment.
Hmm maybe this:
Only line I am unsure about
const randomNumber = Math.random() * (cumulativeWeights[cumulativeWeights.length - 1] ?? 1)Should we error if cumulativeWeights[cumulativeWeights.length - 1] is null? Should be impossible?
import _ from 'lodash'
export type WeightedAlgorithm = {
pick: (values: string[]) => string
}
/**
* Randomly choose an index from an array based on weights
*
* Based on algorithm found here: https://dev.to/trekhleb/weighted-random-algorithm-in-javascript-1pdc
*
* @param weights array of numbered weights that correspond to the indexed values
* @param values array of values to choose from
*/
const weightedChoice = (weights: number[], values: any[]) => {
if (weights.length === 0 || values.length === 0 || weights.length !== values.length) {
throw new Error('The length of the weights and values must be the same and greater than zero')
}
const cumulativeWeights = weights.reduce<number[]>((acc, curr) => {
if (acc.length === 0) {
return [curr]
}
const next = acc[acc.length - 1]
if (!next) {
return acc
}
return [...acc, next]
}, [])
const randomNumber = Math.random() * (cumulativeWeights[cumulativeWeights.length - 1] ?? 1)
const choice = _.transform(cumulativeWeights, (result, value, index) => {
if (value >= randomNumber) {
result.chosenIndex = index
}
return result.chosenIndex === -1
}, { chosenIndex: -1 })
return values[choice.chosenIndex]
}
export const WEIGHTED = (weights: number[]): WeightedAlgorithm => {
return {
pick: (values: any[]): string => {
return weightedChoice(weights, values)
},
}
}
export const WEIGHTED_EVEN = (values: any[]): WeightedAlgorithm => {
return WEIGHTED(_.fill(Array(values.length), 1))
}
| * | ||
| * @returns object with getCohort function for returning the cohort | ||
| */ | ||
| export const useCohorts = () => { |
There was a problem hiding this comment.
After the refactoring, I feel like the better pattern here would have been to use pinia, where the cohortOptionSelected would have been stored as state and the getCohort would be an action. It seems strange that a composable is returning a function and not reactive state.
There was a problem hiding this comment.
Can a store make a GraphQL mutation? That is what I was trying to encapsulate in useCohorts?
There was a problem hiding this comment.
You should be able to use useMutation inside an action, see https://pinia.vuejs.org/core-concepts/actions.html#accessing-other-stores-actions where they utilize composables inside pinia actions.
warrensplayer
changed the title
User facing changelog
n/a
Additional details
In order to be able to A/B test different concepts in the app, an ability to assign a user to a particular cohort is needed. A cohort can be assigned per feature or functionality that needs to be A/B tested.
In this first use case, A/B testing is limited to the ACI Smart Prompt banners being shown on the Specs list page of the app. See the details in #23118 that describes the specific changes.
The cohort functionality is encapsulated in a composable called
useCohortsin thefrontend-sharedpackage.Steps to test
Prerequisites
The cohort functionality is used within the Smart Prompt banners used on the Specs list page of the app. In order to fully test the different banners, the project used for testing will need to be in a certain state.
state.jsonfile under the "App Data" directory and then navigating to "projects" and then the directory with the project name and hash. ThefirstOpenedvalue should be a timestamp older than 4 days agobannersentry from the same file. This will reset the apps tracking of when banners were viewed or dismissed.cypress.config.jsfile for the project as appropriateTesting cohorts
After first starting the app with the launchpad loaded, select the "Developer tools > View App Data" menu item
Within the directory that opens, open the
cachefile in a text editorConfirm that there is not a "COHORTS" entry in the json data
Make sure you are logged out from the dashboard
Login Banner
cachefile from above and confirm there is now a COHORTS entry with a key ofaci_082022_loginthat matches the bannerId inLoginBanner.vue. The key should have an object with the name matching the bannerId and a cohort value of either 'A' or 'B'. Confirm the body copy of the Login Banner matches the appropriate copy for either A or B shown in the document herecachefile should not have changedcachefile to remove the key foraci_082022_logincachefile has a new entry foraci_082022_loginand that the appropriate copy is showingcachefile and remove the same entry until the algorithm chooses the other cohortcachefile to manually edit the cohort to the other value (AorB)Remaining banners
Test the other Smart Prompt banners as described here: feat: ACI Notification Banners #23256
For each banner, confirm the same "cohort" behavior and compare the results with the expected A/B testing values here
The keys in the "COHORTS" section should match the bannerIds for each banner
aci_082022_createOrganizationaci_082022_connectProjectaci_082022_recordUTM Parameters
How has the user experience changed?
Once fully implemented, the only user experience change will be that different copy may be used in the banners depending on which cohort the user is assigned to.
PR Tasks
cypress-documentation?type definitions?