Skip to content

Commit

Permalink
Test methodologies editor (simpler) (#641)
Browse files Browse the repository at this point in the history
* Test methodologies editor

* comment

* min: 0.1

* Default to ABTest

* only allow one method
  • Loading branch information
tomrf1 authored Oct 31, 2024
1 parent ef2f652 commit c92d867
Show file tree
Hide file tree
Showing 14 changed files with 172 additions and 62 deletions.
3 changes: 2 additions & 1 deletion app/models/BannerTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import io.circe.generic.extras.Configuration
import io.circe.generic.extras.auto._
import io.circe.generic.extras.semiauto._
import io.circe.{Decoder, Encoder, Json}
import models.Methodology.defaultMethodologies

case class BannerUI(designName: String)

Expand Down Expand Up @@ -44,9 +45,9 @@ case class BannerTest(
deviceType: Option[DeviceType] = None,
campaignName: Option[String] = Some("NOT_IN_CAMPAIGN"),
signedInStatus: Option[SignedInStatus] = Some(SignedInStatus.All),
isBanditTest: Option[Boolean] = None,
consentStatus: Option[ConsentStatus] = Some(ConsentStatus.All),
deploySchedule: Option[BannerTestDeploySchedule] = None,
methodologies: List[Methodology] = defaultMethodologies
) extends ChannelTest[BannerTest] {

override def withChannel(channel: Channel): BannerTest =
Expand Down
1 change: 1 addition & 0 deletions app/models/ChannelTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ trait ChannelTest[T] {
val lockStatus: Option[LockStatus]
val priority: Option[Int] // 0 is top priority
val campaignName: Option[String]
val methodologies: List[Methodology]

def withChannel(channel: Channel): T
def withPriority(priority: Int): T
Expand Down
3 changes: 2 additions & 1 deletion app/models/EpicTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import io.circe.generic.extras.Configuration
import io.circe.generic.extras.auto._
import io.circe.generic.extras.semiauto._
import io.circe.{Decoder, Encoder}
import models.Methodology.defaultMethodologies

import scala.collection.immutable.IndexedSeq

Expand Down Expand Up @@ -61,8 +62,8 @@ case class EpicTest(
deviceType: Option[DeviceType] = None,
campaignName: Option[String] = Some("NOT_IN_CAMPAIGN"),
signedInStatus: Option[SignedInStatus] = Some(SignedInStatus.All),
isBanditTest: Option[Boolean] = None,
consentStatus: Option[ConsentStatus] = Some(ConsentStatus.All),
methodologies: List[Methodology] = defaultMethodologies
) extends ChannelTest[EpicTest] {

override def withChannel(channel: Channel): EpicTest = this.copy(channel = Some(channel))
Expand Down
2 changes: 2 additions & 0 deletions app/models/HeaderTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import io.circe.generic.extras.Configuration
import io.circe.generic.extras.auto._
import io.circe.generic.extras.semiauto._
import io.circe.{Decoder, Encoder}
import models.Methodology.defaultMethodologies

case class HeaderContent(
heading: String,
Expand Down Expand Up @@ -33,6 +34,7 @@ case class HeaderTest(
campaignName: Option[String] = Some("NOT_IN_CAMPAIGN"),
signedInStatus: Option[SignedInStatus] = Some(SignedInStatus.All),
consentStatus: Option[ConsentStatus] = Some(ConsentStatus.All),
methodologies: List[Methodology] = defaultMethodologies
) extends ChannelTest[HeaderTest] {

override def withChannel(channel: Channel): HeaderTest = this.copy(channel = Some(channel))
Expand Down
19 changes: 19 additions & 0 deletions app/models/Methodology.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package models

import io.circe.generic.extras.Configuration
import io.circe.generic.extras.auto._
import io.circe.{Decoder, Encoder}

sealed trait Methodology

case class ABTest(name: String = "ABTest") extends Methodology
case class EpsilonGreedyBandit(name: String = "EpsilonGreedyBandit", epsilon: Double) extends Methodology

case object Methodology {
val defaultMethodologies = List(ABTest())

implicit val customConfig: Configuration = Configuration.default.withDiscriminator("name")

implicit val secondaryCtaDecoder = Decoder[Methodology]
implicit val secondaryCtaEncoder = Encoder[Methodology]
}
110 changes: 110 additions & 0 deletions public/src/components/channelManagement/TestMethodologyEditor.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import React from 'react';
import { Methodology } from './helpers/shared';
import { makeStyles } from '@mui/styles';
import { MenuItem, Select, SelectChangeEvent, TextField, Theme } from '@mui/material';

const useStyles = makeStyles(({ spacing, palette }: Theme) => ({
container: {
'& > * + *': {
marginTop: spacing(1),
},
},
methodologyContainer: {
display: 'flex',
flexDirection: 'row',
border: `1px solid ${palette.grey[800]}`,
borderRadius: '4px',
padding: spacing(1),
'& > * + *': {
marginLeft: spacing(1),
},
},
}));

const defaultEpsilonGreedyBandit: Methodology = {
name: 'EpsilonGreedyBandit',
epsilon: 0.1,
};

interface TestMethodologyProps {
methodology: Methodology;
isDisabled: boolean;
onChange: (methodology: Methodology) => void;
}

const TestMethodology: React.FC<TestMethodologyProps> = ({
methodology,
isDisabled,
onChange,
}: TestMethodologyProps) => {
const classes = useStyles();

const onSelectChange = (event: SelectChangeEvent<Methodology['name']>) => {
const value = event.target.value as Methodology['name'];
if (value === 'EpsilonGreedyBandit') {
onChange(defaultEpsilonGreedyBandit);
} else {
onChange({ name: 'ABTest' });
}
};
return (
<div className={classes.methodologyContainer}>
<div>
<Select value={methodology.name} onChange={onSelectChange}>
<MenuItem value={'ABTest'} key={'ABTest'}>
AB test
</MenuItem>
<MenuItem value={'EpsilonGreedyBandit'} key={'EpsilonGreedyBandit'}>
Epsilon-greedy bandit
</MenuItem>
</Select>
</div>
<div>
{methodology.name === 'EpsilonGreedyBandit' && (
<div>
<TextField
type={'number'}
InputProps={{ inputProps: { min: 0.1, max: 1, step: 0.1 } }}
value={methodology.epsilon}
label={'Epsilon'}
disabled={isDisabled}
onChange={event => {
const epsilon = parseFloat(event.target.value);
onChange({ ...methodology, epsilon });
}}
/>
</div>
)}
</div>
</div>
);
};

interface TestMethodologyEditorProps {
methodologies: Methodology[];
onChange: (methodologies: Methodology[]) => void;
isDisabled: boolean;
}

export const TestMethodologyEditor: React.FC<TestMethodologyEditorProps> = ({
methodologies,
onChange,
isDisabled,
}: TestMethodologyEditorProps) => {
const classes = useStyles();

// For now we only support one methodology
const methodology = methodologies[0] ?? { name: 'ABTest' };

return (
<div className={classes.container}>
<TestMethodology
methodology={methodology}
isDisabled={isDisabled}
onChange={updatedMethodology => {
onChange([updatedMethodology]);
}}
/>
</div>
);
};
43 changes: 0 additions & 43 deletions public/src/components/channelManagement/banditEditor.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
ArticlesViewedSettings,
ConsentStatus,
DeviceType,
Methodology,
SignedInStatus,
UserCohort,
} from '../helpers/shared';
Expand Down Expand Up @@ -37,8 +38,8 @@ import {
} from '../../../utils/requests';
import TestEditorContextTargeting from '../testEditorContextTargeting';
import { getDesignForVariant } from '../../../utils/bannerDesigns';
import { BanditEditor } from '../banditEditor';
import { DeployScheduleEditor } from './deployScheduleEditor';
import { TestMethodologyEditor } from '../TestMethodologyEditor';

const copyHasTemplate = (content: BannerContent, template: string): boolean =>
(content.heading && content.heading.includes(template)) ||
Expand Down Expand Up @@ -99,8 +100,9 @@ const BannerTestEditor: React.FC<ValidatedTestEditorProps<BannerTest>> = ({
});
};

const onExperimentMethodologyChange = (isBanditTest?: boolean): void => {
updateTest({ ...test, isBanditTest });
const onMethodologyChange = (methodologies: Methodology[]): void => {
setValidationStatusForField('methodologies', methodologies.length > 0);
updateTest({ ...test, methodologies });
};

const onArticlesViewedSettingsValidationChanged = (isValid: boolean): void =>
Expand Down Expand Up @@ -236,17 +238,17 @@ const BannerTestEditor: React.FC<ValidatedTestEditorProps<BannerTest>> = ({
<Typography variant={'h3'} className={classes.sectionHeader}>
Experiment Methodology
</Typography>
<BanditEditor
test={test}
<TestMethodologyEditor
methodologies={test.methodologies}
isDisabled={!userHasTestLocked}
onExperimentMethodologyChange={onExperimentMethodologyChange}
onChange={onMethodologyChange}
/>
</div>

{test.variants.length > 1 && (
<div className={classes.sectionContainer}>
<Typography variant={'h3'} className={classes.sectionHeader}>
Variants split
Variants split (applies to AB tests only)
</Typography>
<div>
<TestVariantsSplitEditor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const DEV_AND_CODE_DEFAULT_BANNER_TEST: BannerTest = {
variants: [DEV_AND_CODE_DEFAULT_VARIANT],
articlesViewedSettings: undefined,
contextTargeting: { tagIds: [], sectionIds: [], excludedTagIds: [], excludedSectionIds: [] },
methodologies: [{ name: 'ABTest' }],
};

const PROD_DEFAULT_BANNER: BannerTest = {
Expand All @@ -64,6 +65,7 @@ const PROD_DEFAULT_BANNER: BannerTest = {
variants: [],
articlesViewedSettings: undefined,
contextTargeting: { tagIds: [], sectionIds: [], excludedTagIds: [], excludedSectionIds: [] },
methodologies: [{ name: 'ABTest' }],
};

export const getDefaultTest = (): BannerTest => {
Expand Down
18 changes: 10 additions & 8 deletions public/src/components/channelManagement/epicTests/testEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
SignedInStatus,
PageContextTargeting,
ConsentStatus,
Methodology,
} from '../helpers/shared';
import { FormControlLabel, Switch, Typography } from '@mui/material';
import CampaignSelector from '../CampaignSelector';
Expand All @@ -34,8 +35,8 @@ import { useStyles } from '../helpers/testEditorStyles';
import { EpicTestPreviewButton } from './testPreview';
import { ValidatedTestEditor, ValidatedTestEditorProps } from '../validatedTestEditor';
import { TestEditorProps } from '../testsForm';
import { BanditEditor } from '../banditEditor';
import { AnalyticsButton } from '../AnalyticsButton';
import { TestMethodologyEditor } from '../TestMethodologyEditor';

const copyHasTemplate = (test: EpicTest, template: string): boolean =>
test.variants.some(
Expand Down Expand Up @@ -90,8 +91,9 @@ export const getEpicTestEditor = (
});
};

const onExperimentMethodologyChange = (isBanditTest?: boolean): void => {
updateTest({ ...test, isBanditTest });
const onMethodologyChange = (methodologies: Methodology[]): void => {
setValidationStatusForField('methodologies', methodologies.length > 0);
updateTest({ ...test, methodologies });
};

const onVariantsChange = (updatedVariantList: EpicVariant[]): void => {
Expand Down Expand Up @@ -261,25 +263,25 @@ export const getEpicTestEditor = (
<Typography variant={'h3'} className={classes.sectionHeader}>
Experiment Methodology
</Typography>
<BanditEditor
test={test}
<TestMethodologyEditor
methodologies={test.methodologies}
isDisabled={!userHasTestLocked}
onExperimentMethodologyChange={onExperimentMethodologyChange}
onChange={onMethodologyChange}
/>
</div>

{epicEditorConfig.allowCustomVariantSplit && canHaveCustomVariantSplit(test.variants) && (
<div className={classes.sectionContainer}>
<Typography variant={'h3'} className={classes.sectionHeader}>
Variants split
Variants split (applies to AB tests only)
</Typography>
<div>
<TestVariantsSplitEditor
variants={test.variants}
controlProportionSettings={test.controlProportionSettings}
onControlProportionSettingsChange={onControlProportionSettingsChange}
onValidationChange={onVariantsSplitSettingsValidationChanged}
isDisabled={!userHasTestLocked || !!test.isBanditTest}
isDisabled={!userHasTestLocked}
/>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const DEV_AND_CODE_DEFAULT_TEST: EpicTest = {
variants: [DEV_AND_CODE_DEFAULT_VARIANT],
highPriority: false, // has been removed from form, but might be used in future
useLocalViewLog: false,
methodologies: [{ name: 'ABTest' }],
};

const PROD_DEFAULT_TEST: EpicTest = {
Expand All @@ -90,6 +91,7 @@ const PROD_DEFAULT_TEST: EpicTest = {
variants: [],
highPriority: false, // has been removed from form, but might be used in future
useLocalViewLog: false,
methodologies: [{ name: 'ABTest' }],
};

export const getDefaultTest = (): EpicTest => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ const HeaderTestEditor: React.FC<ValidatedTestEditorProps<HeaderTest>> = ({
{test.variants.length > 1 && (
<div className={classes.sectionContainer}>
<Typography variant={'h3'} className={classes.sectionHeader}>
Variants split
Variants split (applies to AB tests only)
</Typography>
<div>
<TestVariantsSplitEditor
Expand Down
Loading

0 comments on commit c92d867

Please sign in to comment.