Test methodologies editor #640
Conversation
app/models/BannerTests.scala
Outdated
| consentStatus: Option[ConsentStatus] = Some(ConsentStatus.All), | ||
| deploySchedule: Option[BannerTestDeploySchedule] = None, | ||
| methodologies: List[Methodology] = Nil |
There was a problem hiding this comment.
although the new methodologies field is mandatory, it's defaulted to Nil (an empty list), meaning validation does not fail when fetching existing data from dynamodb
There was a problem hiding this comment.
Why is the field mandatory? Could we have it as an optional so we don't have to default it to a Nil
There was a problem hiding this comment.
In general I try to avoid Option[List[T]], because you have the following possible states:
None- empty list
- non-empty list
And 1 and 2 sort of mean the same thing!
If we make the field optional then the client has to handle that possibility as well.
| fontWeight: 500, | ||
| }, | ||
| error: { | ||
| color: 'red', |
There was a problem hiding this comment.
I don't I've noticed we can use the actual colour name before!
|
|
||
| const defaultEpsilonGreedyBandit: Methodology = { | ||
| name: 'EpsilonGreedyBandit', | ||
| epsilon: 0.1, |
There was a problem hiding this comment.
Do we need any documentation to help support setting the epsilon value? Or do marketing already have that knowledge?
There was a problem hiding this comment.
👍 we can document it in the RRCP user guide
| <div> | ||
| <TextField | ||
| type={'number'} | ||
| InputProps={{ inputProps: { min: 0, max: 1, step: 0.1 } }} |
There was a problem hiding this comment.
Out of interest what would happen if you set it to 0?
There was a problem hiding this comment.
Good question. It would pick the "best" variant 100% of the time, which is not useful! I'll set min to 0.1
tomrf1
force-pushed
the
tf-methodologies
branch
from
9cbcd9d
to
72483a1
Compare
| ))} | ||
| <Button | ||
| onClick={onAddClick} | ||
| disabled={isDisabled || methodologies.length >= 4} |
There was a problem hiding this comment.
Is 4 significant anywhere else? Seems like a sensible maximum though
There was a problem hiding this comment.
no it's fairly arbitrary!
This PR updates the methodologies selector to enable use to compare them, e.g. AB test vs epsilon-greedy.
See also the associated SDC PR: guardian/support-dotcom-components#1251
If more than one methodology is configured then the the audience is split evenly between the methodologies. Each methodology is tracked separately with a different test name. This is done using the new
testNamefield on theMethodologymodel.I've also added the constraint that you cannot change the methodologies once a test is live. This is because it could change the test names, which would break tracking as well as the bandit calculations!
Single methodology:
Two methodologies: