WIP Feature/handle users in multiple experiments #1

ThomasAdams2 · 2019-01-29T17:02:17Z

No description provided.

Gibe.AbTest/AbTest.cs

aheaford · 2019-02-05T09:57:34Z

Gibe.AbTest/DefaultExperimentService.cs

+			_experimentCookieValueFactory = experimentCookieValueFactory;
+		}
+
+		//TODO: Not sure we need this, being in no experiments isn't a special case


Could we replace this with "are there any active experiments?"? It feels like this should never be allowed to return false in any other scenario

lol i ended up with the same question again two and a half years later :v

what is this for?

Gibe.AbTest/DefaultExperimentService.cs

jdurcan

Looks good, Tommo. Few little bits.

Gibe.AbTest/ExperimentCookieValue.cs

Gibe.AbTest/AbTest.cs

Gibe.AbTest/DefaultExperimentService.cs

tristanjthompson · 2021-05-21T14:22:20Z

Gibe.AbTest/AbTest.cs

 		{
 			return _abTestingService.GetVariation(experimentId, variationNumber);
 		}

+		private IEnumerable<Experiment> FilterExperiments(IEnumerable<Experiment> experments, string userAgent)


This method (particularly line 56) is fairly difficult to read and understand what it's doing - could we use better naming? I know it's filtering, but what's it filtering for and/or why? A better name here would help immensely

tristanjthompson · 2021-05-21T15:44:09Z

Gibe.AbTest/AbTest.cs

 		{
 			return _abTestingService.GetVariation(experimentId, variationNumber);
 		}

+		private IEnumerable<Experiment> FilterExperiments(IEnumerable<Experiment> experments, string userAgent)
+		{
+			var filtered = experments.Where(e => e.Variations.Any(v => v.DesktopOnly) && !userAgent.Contains("Mobi") || !e.Variations.All(v => v.DesktopOnly));


I really don't fully understand what the intent is here - especially given the lack of brackets to make it super-clear what order the && and || are being applied in (I've figured out they would be around the two sides of the &&)

I think what it's saying is:
"If I'm not on mobile, get me experiments that have at least 1 desktop-only variation"
Otherwise
"Get me experiments that aren't all desktop-only experiments"

Is that correct? Is that the logic we want?

Taking that into account Looking at the FilterVariations method, this might read better as the below to mirror what's going on in FilterVariations()

Suggested change

var filtered = experments.Where(e => e.Variations.Any(v => v.DesktopOnly) && !userAgent.Contains("Mobi") || !e.Variations.All(v => v.DesktopOnly));

var filtered = experments.Where(e =>

(e.Variations.Any(v => v.DesktopOnly) && !userAgent.Contains("Mobi"))

||

(e.Variations.Any(v => !v.DesktopOnly) && userAgent.Contains("Mobi"))

);

Given the multiple use of userAgent.Contains("Mobi"), would it read better, and centralise code using the DRY principle) if you created an IsMobile() method?

i.e.

private bool IsMobile(string userAgent) => userAgent.Contains("Mobi")

tristanjthompson · 2021-05-21T15:44:42Z

Gibe.AbTest/AbTest.cs

+			var filtered = experments.Where(e => e.Variations.Any(v => v.DesktopOnly) && !userAgent.Contains("Mobi") || !e.Variations.All(v => v.DesktopOnly));
+			if (!filtered.Any())
+			{
+				return experments.Take(1); //TODO: We should not return anything if there are no correct matches, this requires a refactor to use IEnumerables everywhere


Should we do this refactor then? As this method returns IEnumerable, I'm not sure what needs refactoring?

tristanjthompson · 2021-05-21T15:46:10Z

Gibe.AbTest/AbTest.cs

+			}
+			return filtered;
+		}
+
 		private IEnumerable<Variation> FilterVariations(IEnumerable<Variation> variations, string userAgent)


Again, not sure this name helps understand what's being filtered/why

tristanjthompson · 2021-05-21T15:53:02Z

Gibe.AbTest/AbTestRepository.cs

 		public IEnumerable<VariationDto> GetVariations(string experimentId)
 		{
 			using (var db = _databaseProvider.GetDatabase())
 			{
-				return db.Query<VariationDto>("WHERE ExperimentId = @0", experimentId);
+				return db.Fetch<VariationDto>("WHERE ExperimentId = @0", experimentId);


Good update 👍

tristanjthompson · 2021-05-21T15:54:22Z

Gibe.AbTest/AbTestRepository.cs

 		{
 			using (var db = _databaseProvider.GetDatabase())
 			{
-				return db.Query<ExperimentDto>("FROM AbExperiment");
+				var now = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss");


Should this use an ITimeProvider?

tristanjthompson · 2021-05-21T16:00:01Z

Gibe.AbTest/AbTestRepository.cs

+					$"AND '{now}' >= [StartDate] OR StartDate IS NULL " +
+					$"AND '{now}' < [EndDate] OR StartDate IS NULL ");


Would some brackets help disambiguate this (I always prefer being specific with a chain of AND/OR statements)

Suggested change

$"AND '{now}' >= [StartDate] OR StartDate IS NULL " +

$"AND '{now}' < [EndDate] OR StartDate IS NULL ");

$"(AND '{now}' >= [StartDate] OR StartDate IS NULL) " +

$"(AND '{now}' < [EndDate] OR StartDate IS NULL)");

tristanjthompson · 2021-05-21T16:04:20Z

Gibe.AbTest/DefaultExperimentService.cs

+		{
+			var variationsCookie = _experimentCookieValueFactory.ExperimentCookieValue(variations);
+
+			_cookieService.Create(CookieKey, variationsCookie.RawValue, DateTime.Now.AddDays(120));


Should this use an ITimeProvider?

tristanjthompson · 2021-05-21T16:06:00Z

Gibe.AbTest/ExperimentCookieValue.cs

+		private const string Seperator = "~";
+		private const string ExperimentSeperator = "-";
+
+		public string RawValue;


Is this ever meant to be set externally? If not, would it be better as:

Suggested change

public string RawValue;

public string RawValue { get; private set; }

tristanjthompson · 2021-05-21T16:08:50Z

Gibe.AbTest/AbTest.cs

+			var currentWeight = 0;
+			foreach (var o in opts)
 			{
-				currrentWeight += t.Weight;
-				if (currrentWeight > selectedNumber)
-					return t;
+				currentWeight += o.Weight;
+				if (currentWeight > selectedNumber)
+					return o;


Spelling FTW!

jdurcan · 2021-05-25T13:08:53Z

Wow, this is a blast from the past.

ThomasAdams2 · 2021-05-26T09:09:35Z

This work has been returned to being on hold as these changes are not required for https://jira.gibedigital.net/browse/BR-1757.

Good night sweet prince

ThomasAdams2 added 3 commits January 25, 2019 12:54

[GCOM-1309] Add methods for users in multiple experiments

8bc8ba3

Add constructor to FakeAbTest

86a8864

version: 1.1.0

6766b80

ThomasAdams2 requested review from stevetemple, jdurcan and aheaford January 29, 2019 17:02

stevetemple and others added 3 commits January 30, 2019 11:25

Add more tests for repo

2e7f9fd

Fixing typos in variable names

e98c321

Move IExperiementService into project and unit tests for behaviour

32eebb0

aheaford reviewed Feb 5, 2019

View reviewed changes

jdurcan requested changes Jun 11, 2019

View reviewed changes

Gibe.AbTest/ExperimentCookieValue.cs Outdated Show resolved Hide resolved

Gibe.AbTest/AbTest.cs Outdated Show resolved Hide resolved

Gibe.AbTest/DefaultExperimentService.cs Outdated Show resolved Hide resolved

Gibe.AbTest/DefaultExperimentService.cs Outdated Show resolved Hide resolved

stevetemple changed the title ~~Feature/handle users in multiple experiements~~ WIP Feature/handle users in multiple experiements Jul 25, 2019

ThomasAdams2 added 5 commits September 17, 2019 09:41

Address PR comments

efa1320

Move enabled check into repository

9149eb9

Merge branch 'master' into feature/handle-users-in-multiple-experiements

602bcc7

Tidy up tests after merge

c69153f

Move date check into repository

9e616bf

stevetemple changed the title ~~WIP Feature/handle users in multiple experiements~~ WIP Feature/handle users in multiple experiments Jul 16, 2020

stevetemple marked this pull request as draft December 10, 2020 14:29

stevetemple removed their request for review December 16, 2020 14:17

Merge master

2f98352

ThomasAdams2 requested review from stevetemple and tristanjthompson May 20, 2021 14:43

Merge branch 'master' into feature/handle-users-in-multiple-experiements

5f50dec

tristanjthompson reviewed May 21, 2021

View reviewed changes

ThomasAdams2 marked this pull request as ready for review May 26, 2021 07:29

ThomasAdams2 marked this pull request as draft May 26, 2021 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Feature/handle users in multiple experiments #1

WIP Feature/handle users in multiple experiments #1

ThomasAdams2 commented Jan 29, 2019

aheaford Feb 5, 2019

aheaford May 25, 2021

jdurcan left a comment

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

tristanjthompson May 21, 2021

jdurcan commented May 25, 2021

ThomasAdams2 commented May 26, 2021

		$"AND '{now}' >= [StartDate] OR StartDate IS NULL " +
		$"AND '{now}' < [EndDate] OR StartDate IS NULL ");

	public string RawValue;
	public string RawValue { get; private set; }

WIP Feature/handle users in multiple experiments #1

Are you sure you want to change the base?

WIP Feature/handle users in multiple experiments #1

Conversation

ThomasAdams2 commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdurcan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdurcan commented May 25, 2021

ThomasAdams2 commented May 26, 2021