Merge pull request #90 from uc-cdis/prerelease_040

Improvements to cohort-middleware docs and tests following extra code review

Author: pieterlukasse

docs/cohort-middleware-overview.png

@@ -75,7 +75,7 @@ func (u TeamProjectAuthz) TeamProjectValidationForCohort(ctx *gin.Context, cohor
 
 func (u TeamProjectAuthz) TeamProjectValidation(ctx *gin.Context, cohortDefinitionIds []int, filterCohortPairs []utils.CustomDichotomousVariableDef) bool {
 
-	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsListFromRequest(cohortDefinitionIds, filterCohortPairs)
+	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsList(cohortDefinitionIds, filterCohortPairs)
 	return u.TeamProjectValidationForCohortIdsList(ctx, uniqueCohortDefinitionIdsList)
 }
 

middlewares/teamprojectauthz.go

@@ -566,39 +566,68 @@ func TestRetrieveBreakdownStatsBySourceIdAndCohortIdWithResultsWithOnePersonTwoH
 
 func TestGetTeamProjectsThatMatchAllCohortDefinitionIdsOnlyDefaultMatch(t *testing.T) {
 	setUp(t)
-	cohortDefinitionId := 2
+	cohortDefinitionId := 2 // 'Medium cohort' in test_data_atlas.sql
 	filterCohortPairs := []utils.CustomDichotomousVariableDef{
 		{
 			CohortDefinitionId1: smallestCohort.Id,
 			CohortDefinitionId2: largestCohort.Id,
 			ProvidedName:        "test"},
 	}
-	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsListFromRequest([]int{cohortDefinitionId}, filterCohortPairs)
+	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsList([]int{cohortDefinitionId}, filterCohortPairs)
+	if len(uniqueCohortDefinitionIdsList) != 3 {
+		t.Errorf("Expected uniqueCohortDefinitionIdsList length to be 3")
+	}
 	teamProjects, _ := cohortDefinitionModel.GetTeamProjectsThatMatchAllCohortDefinitionIds(uniqueCohortDefinitionIdsList)
 	if len(teamProjects) != 1 || teamProjects[0] != "defaultteamproject" {
 		t.Errorf("Expected to find only defaultteamproject")
 	}
+
+	// Should also hold true if the uniqueCohortDefinitionIdsList is length 2 (which matches teamprojectX's cohort
+	// list length but not in contents):
+	filterCohortPairs = []utils.CustomDichotomousVariableDef{
+		{
+			CohortDefinitionId1: 2,
+			CohortDefinitionId2: largestCohort.Id,
+			ProvidedName:        "test"},
+	}
+	uniqueCohortDefinitionIdsList = utils.GetUniqueCohortDefinitionIdsList([]int{cohortDefinitionId}, filterCohortPairs)
+	if len(uniqueCohortDefinitionIdsList) != 2 {
+		t.Errorf("Expected uniqueCohortDefinitionIdsList length to be 2")
+	}
+	teamProjects, _ = cohortDefinitionModel.GetTeamProjectsThatMatchAllCohortDefinitionIds(uniqueCohortDefinitionIdsList)
+	if len(teamProjects) != 1 || teamProjects[0] != "defaultteamproject" {
+		t.Errorf("Expected to find only defaultteamproject")
+	}
 }
 
 func TestGetTeamProjectsThatMatchAllCohortDefinitionIds(t *testing.T) {
 	setUp(t)
-	cohortDefinitionId := 2
+	cohortDefinitionId := 2 // 'Medium cohort' in test_data_atlas.sql
 	filterCohortPairs := []utils.CustomDichotomousVariableDef{
 		{
 			CohortDefinitionId1: 2,
-			CohortDefinitionId2: 2,
+			CohortDefinitionId2: 32,
 			ProvidedName:        "test"},
 	}
-	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsListFromRequest([]int{cohortDefinitionId}, filterCohortPairs)
+	uniqueCohortDefinitionIdsList := utils.GetUniqueCohortDefinitionIdsList([]int{cohortDefinitionId}, filterCohortPairs)
+	if len(uniqueCohortDefinitionIdsList) != 2 {
+		t.Errorf("Expected uniqueCohortDefinitionIdsList length to be 2")
+	}
 	teamProjects, _ := cohortDefinitionModel.GetTeamProjectsThatMatchAllCohortDefinitionIds(uniqueCohortDefinitionIdsList)
 	if len(teamProjects) != 2 {
 		t.Errorf("Expected to find two 'team projects' matching the cohort list, found %s", teamProjects)
 	}
+	if !utils.ContainsString(teamProjects, "defaultteamproject") {
+		t.Errorf("Expected to find 'defaultteamproject' in the results, found %s", teamProjects)
+	}
+	if !utils.ContainsString(teamProjects, "teamprojectX") {
+		t.Errorf("Expected to find 'teamprojectX' in the results, found %s", teamProjects)
+	}
 }
 
 func TestGetCohortDefinitionIdsForTeamProject(t *testing.T) {
 	setUp(t)
-	testTeamProject := "teamprojectX"
+	testTeamProject := "teamprojectY"
 	allowedCohortDefinitionIds, _ := cohortDefinitionModel.GetCohortDefinitionIdsForTeamProject(testTeamProject)
 	if len(allowedCohortDefinitionIds) != 1 {
 		t.Errorf("Expected teamProject '%s' to have one cohort, but found %d",
@@ -633,7 +662,7 @@ func TestGetAllCohortDefinitionsAndStatsOrderBySizeDesc(t *testing.T) {
 	}
 
 	// some extra tests to cover also the teamProject option for this method:
-	testTeamProject := "teamprojectX"
+	testTeamProject := "teamprojectY"
 	allowedCohortDefinitions, _ := cohortDefinitionModel.GetAllCohortDefinitionsAndStatsOrderBySizeDesc(testSourceId, testTeamProject)
 	if len(allowedCohortDefinitions) != 1 {
 		t.Errorf("Expected teamProject '%s' to have one cohort, but found %d",

tests/models_tests/models_test.go

@@ -69,28 +69,29 @@ values
     (1458, 1005, 1185),
     (1459, 1005, 1186),
     (1460, 1005, 1187),
-    (1461, 1009, 1188),
-    (1462, 1009, 1189),
-    (1463, 1009, 1190),
-    (1464, 1009, 1191),
-    (1465, 1009, 1192),
-    (1466, 1009, 1193),
-    (1467, 1009, 1194),
-    (2454, 4000, 1181),
-    (2455, 4000, 1182),
-    (2456, 4000, 1183),
-    (2457, 4000, 1184),
-    (2458, 4000, 1185),
-    (2459, 4000, 1186),
-    (2460, 4000, 1187),
-    (2461, 4000, 1188),
-    (2462, 4000, 1189),
-    (2463, 4000, 1190),
-    (2464, 4000, 1191),
-    (2465, 4000, 1192),
-    (2466, 4000, 1193),
-    (2467, 4000, 1194),
-    (2468, 4000, 2193),
-    (2469, 4000, 3193),
-    (2470, 4000, 4193)
+    (1461, 1005, 4193), -- 1005 teamprojectX has access to cohorts 2 and 32
+    (2461, 1009, 1188),
+    (2462, 1009, 1189),
+    (2463, 1009, 1190),
+    (2464, 1009, 1191),
+    (2465, 1009, 1192),
+    (2466, 1009, 1193),
+    (2467, 1009, 1194), -- 1009 teamprojectY has access to cohort 4
+    (4454, 4000, 1181),
+    (4455, 4000, 1182),
+    (4456, 4000, 1183),
+    (4457, 4000, 1184),
+    (4458, 4000, 1185),
+    (4459, 4000, 1186),
+    (4460, 4000, 1187),
+    (4461, 4000, 1188),
+    (4462, 4000, 1189),
+    (4463, 4000, 1190),
+    (4464, 4000, 1191),
+    (4465, 4000, 1192),
+    (4466, 4000, 1193),
+    (4467, 4000, 1194),
+    (4468, 4000, 2193),
+    (4469, 4000, 3193),
+    (4470, 4000, 4193) -- 4000 defaultteamproject has access to all cohorts
 ;

tests/setup_local_db/test_data_atlas.sql

@@ -43,6 +43,15 @@ func Pos(value int64, list []int64) int {
 	return -1
 }
 
+func ContainsString(list []string, value string) bool {
+	for _, item := range list {
+		if item == value {
+			return true
+		}
+	}
+	return false
+}
+
 func Contains(list []int, value int) bool {
 	for _, item := range list {
 		if item == value {
@@ -289,7 +298,10 @@ func MakeUnique(input []int) []int {
 	return uniqueList
 }
 
-func GetUniqueCohortDefinitionIdsListFromRequest(cohortDefinitionIds []int, filterCohortPairs []CustomDichotomousVariableDef) []int {
+// Utility function to parse out the cohort definition ids from a specific structure in which
+// they can be found (in this case deep inside CustomDichotomousVariableDef items) and concatenate them
+// with another given list of ids, removing duplicate ids (if any). 
+func GetUniqueCohortDefinitionIdsList(cohortDefinitionIds []int, filterCohortPairs []CustomDichotomousVariableDef) []int {
 	var idsList []int
 	idsList = append(idsList, cohortDefinitionIds...)
 	if len(filterCohortPairs) > 0 {