From 4c1276efe03058e6acafc57580af6b744ba66931 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 13:54:31 -0400 Subject: [PATCH 01/10] Add QuickstartTag model --- cmd/migrate/migrate.go | 2 +- pkg/database/db.go | 6 ++++++ pkg/database/db_seed.go | 9 +++++++++ pkg/models/tag.go | 5 +++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/cmd/migrate/migrate.go b/cmd/migrate/migrate.go index ea3ea68f..5edb417b 100644 --- a/cmd/migrate/migrate.go +++ b/cmd/migrate/migrate.go @@ -12,7 +12,7 @@ func main() { godotenv.Load() config.Init() database.Init() - err := database.DB.AutoMigrate(&models.Quickstart{}, &models.QuickstartProgress{}, &models.Tag{}, &models.HelpTopic{}, &models.FavoriteQuickstart{}) + err := database.DB.AutoMigrate(&models.Quickstart{}, &models.QuickstartProgress{}, &models.Tag{}, &models.HelpTopic{}, &models.FavoriteQuickstart{}, &models.QuickstartTag{}) if err != nil { panic(err) } diff --git a/pkg/database/db.go b/pkg/database/db.go index a8cda813..7b596b0b 100644 --- a/pkg/database/db.go +++ b/pkg/database/db.go @@ -45,6 +45,12 @@ func Init() { if !DB.Migrator().HasTable(&models.FavoriteQuickstart{}) { DB.Migrator().CreateTable(&models.FavoriteQuickstart{}) } + if (!DB.Migrator().HasTable(&models.QuickstartTag{})) { + DB.Migrator().CreateTable(&models.QuickstartTag{}) + } + + DB.SetupJoinTable(&models.Quickstart{}, "Tags", &models.QuickstartTag{}) + DB.SetupJoinTable(&models.Tag{}, "Quickstarts", &models.QuickstartTag{}) if err != nil { panic(fmt.Sprintf("failed to connect database: %s", err.Error())) diff --git a/pkg/database/db_seed.go b/pkg/database/db_seed.go index b9728f4d..2c679d8d 100644 --- a/pkg/database/db_seed.go +++ b/pkg/database/db_seed.go @@ -249,6 +249,15 @@ func clearOldContent() []models.FavoriteQuickstart { DB.Unscoped().Delete(&h) } + // Remove any left-over links between quickstarts and their tags. + + var staleQuickStartLinks []models.QuickstartTag + DB.Model(&models.QuickstartTag{}).Find(&staleQuickStartLinks) + + for _, link := range staleQuickStartLinks { + DB.Unscoped().Delete(&link) + } + return favorites } diff --git a/pkg/models/tag.go b/pkg/models/tag.go index 39cd247e..67844fc9 100644 --- a/pkg/models/tag.go +++ b/pkg/models/tag.go @@ -52,3 +52,8 @@ type Tag struct { Quickstarts []Quickstart `gorm:"many2many:quickstart_tags;"` HelpTopics []HelpTopic `gorm:"many2many:help_topic_tags;"` } + +type QuickstartTag struct { + QuickstartID uint `gorm:"primaryKey"` + TagID uint `gorm:"primaryKey"` +} From 3766af22af7f979f6a8bbebbf183bdb19b6b16b4 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 13:59:22 -0400 Subject: [PATCH 02/10] Add priority to QuickstartTag --- pkg/database/db_seed.go | 16 ++++++++++++---- pkg/models/tag.go | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/database/db_seed.go b/pkg/database/db_seed.go index 2c679d8d..47259e54 100644 --- a/pkg/database/db_seed.go +++ b/pkg/database/db_seed.go @@ -16,8 +16,9 @@ import ( ) type TagTemplate struct { - Kind string - Value string + Kind string + Value string + Priority *int } type MetadataTemplate struct { @@ -315,12 +316,19 @@ func SeedTags() { originalTag = newTag } - // Create tags quickstarts associations - err := DB.Model(&originalTag).Association("Quickstarts").Append(&quickstart) + newLink := models.QuickstartTag{QuickstartID: quickstart.ID, TagID: originalTag.ID} + + if newTag.Type == models.BundleTag { + newLink.Priority = tag.Priority + } + + err := DB.Create(&newLink).Error + if err != nil { fmt.Println("Failed creating tags associations", err.Error()) } + originalTag.Quickstarts = append(originalTag.Quickstarts, quickstart) quickstart.Tags = append(quickstart.Tags, originalTag) DB.Save(&quickstart) diff --git a/pkg/models/tag.go b/pkg/models/tag.go index 67844fc9..7e404e26 100644 --- a/pkg/models/tag.go +++ b/pkg/models/tag.go @@ -56,4 +56,5 @@ type Tag struct { type QuickstartTag struct { QuickstartID uint `gorm:"primaryKey"` TagID uint `gorm:"primaryKey"` + Priority *int `gorm:"default:null"` } From c8e4e6ad293da2b560e73112fe65be79fd2843f6 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 13:59:37 -0400 Subject: [PATCH 03/10] Return quickstarts from a single bundle in sorted order --- pkg/routes/quickstarts.go | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/pkg/routes/quickstarts.go b/pkg/routes/quickstarts.go index 67b8e516..ae189220 100644 --- a/pkg/routes/quickstarts.go +++ b/pkg/routes/quickstarts.go @@ -9,6 +9,8 @@ import ( "github.com/RedHatInsights/quickstarts/pkg/database" "github.com/RedHatInsights/quickstarts/pkg/models" "github.com/go-chi/chi/v5" + + "gorm.io/gorm/clause" ) func FindQuickstartById(id int) (models.Quickstart, error) { @@ -27,7 +29,56 @@ func findQuickstartsByName(name string, pagination Pagination) ([]models.Quickst return quickStarts, nil } +func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quickstart, error) { + var quickstarts []models.Quickstart + var foundTags []models.Tag + var err error + + database.DB.Model(&models.Tag{}).Where("type = ? AND value = ?", models.BundleTag, bundle).Limit(1).Find(&foundTags) + err = database.DB.Error + + if err != nil { + return quickstarts, err + } + + if len(foundTags) == 0 { + return quickstarts, nil + } + + tag := foundTags[0] + + quickstartIdsQuery := database.DB.Model(&models.QuickstartTag{}).Select("quickstart_id").Where("tag_id = ?", tag.ID) + + database. + DB. + Limit(pagination.Limit). + Offset(pagination.Offset). + Select("id, name, content"). + Where("id IN (?)", quickstartIdsQuery). + Clauses(clause.OrderBy{ + Expression: clause.Expr{ + SQL: "COALESCE((SELECT MAX(priority) FROM quickstart_tags WHERE quickstart_tags.tag_id = ? AND quickstart_tags.quickstart_id = quickstarts.id), 1000)", + Vars: []interface{}{tag.ID}, + }, + }). + Find(&quickstarts) + + err = database.DB.Error + + if err != nil { + return quickstarts, err + } + + return quickstarts, nil +} + func findQuickstartsByTags(tagTypes []models.TagType, tagValues []string, pagination Pagination) ([]models.Quickstart, error) { + // Special case of requesting exactly a single bundle: we will return results sorted by the quickstarts' priorities + // within that bundle + if len(tagTypes) == 1 && tagTypes[0] == models.BundleTag && len(tagValues) == 1 { + return findBundleQuickstarts(tagValues[0], pagination) + } + var quickstarts []models.Quickstart var tagsArray []models.Tag database.DB.Where("type IN ? AND value IN ?", tagTypes, tagValues).Find(&tagsArray) From e03ecece85801d63bbf9ea6d603254fcc4193d58 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 17:04:24 -0400 Subject: [PATCH 04/10] Include priorities in quickstart metadata --- pkg/database/db_seed.go | 88 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 8 deletions(-) diff --git a/pkg/database/db_seed.go b/pkg/database/db_seed.go index 47259e54..531af55c 100644 --- a/pkg/database/db_seed.go +++ b/pkg/database/db_seed.go @@ -79,22 +79,94 @@ func findTags() []MetadataTemplate { return MetadataTemplates } -func seedQuickstart(t MetadataTemplate, defaultTag models.Tag) (models.Quickstart, error) { - yamlfile, err := ioutil.ReadFile(t.ContentPath) +func makeQuickstartPrioritiesMap(tags []TagTemplate) (out map[string]int) { + out = make(map[string]int) + + for _, tag := range tags { + if tag.Kind == string(models.BundleTag) && tag.Priority != nil { + out[tag.Value] = *tag.Priority + } + } + + return out +} + +func quickstartMetadata(quickstartData map[string]interface{}) (map[string]interface{}, error) { + rawMetadata, ok := quickstartData["metadata"] + + if !ok { + return nil, fmt.Errorf("expected quickstart to contain metadata") + } + + metadata, ok := rawMetadata.(map[string]interface{}) + + if !ok { + return nil, fmt.Errorf("expected quickstart metadata to be an object, got %v", metadata) + } + + return metadata, nil +} + +func quickstartName(metadata map[string]interface{}) (string, error) { + rawName, ok := metadata["name"] + + if !ok { + return "", fmt.Errorf("expected quickstart metadata to contain a name") + } + + name, ok := rawName.(string) + + if !ok { + return "", fmt.Errorf("expected quickstart metadata.name to be a string got %v", name) + } + + return name, nil +} + +func seedQuickstart(t MetadataTemplate, defaultTag models.Tag, priorities map[string]int) (models.Quickstart, error) { var newQuickstart models.Quickstart var originalQuickstart models.Quickstart + + yamlfile, err := ioutil.ReadFile(t.ContentPath) + + if err != nil { + return newQuickstart, err + } + + var quickstartData map[string]interface{} + err = yaml.Unmarshal(yamlfile, &quickstartData) + + if err != nil { + return newQuickstart, err + } + + metadata, err := quickstartMetadata(quickstartData) + + if err != nil { + return newQuickstart, err + } + + name, err := quickstartName(metadata) + + if err != nil { + return newQuickstart, err + } + + if len(priorities) > 0 { + metadata["bundle_priority"] = priorities + } + + jsonContent, err := json.Marshal(quickstartData) + if err != nil { return newQuickstart, err } - jsonContent, err := yaml.YAMLToJSON(yamlfile) - var data map[string]map[string]string - json.Unmarshal(jsonContent, &data) - name := data["metadata"]["name"] r := DB.Where("name = ?", name).Find(&originalQuickstart) + if r.Error != nil { // check for DB error - return newQuickstart, err + return newQuickstart, r.Error } else if r.RowsAffected == 0 { // Create new quickstart newQuickstart.Content = jsonContent @@ -294,7 +366,7 @@ func SeedTags() { var quickstart models.Quickstart var quickstartErr error var tags []models.Tag - quickstart, quickstartErr = seedQuickstart(template, defaultTags["quickstart"]) + quickstart, quickstartErr = seedQuickstart(template, defaultTags["quickstart"], makeQuickstartPrioritiesMap(template.Tags)) if quickstartErr != nil { fmt.Println("Unable to seed quickstart: ", quickstartErr.Error(), template.ContentPath) } From a535691fc353572cec9eab131333913c5f655e8a Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 17:07:58 -0400 Subject: [PATCH 05/10] Add warning comment for default priority --- pkg/routes/quickstarts.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/routes/quickstarts.go b/pkg/routes/quickstarts.go index ae189220..ab1f9dd1 100644 --- a/pkg/routes/quickstarts.go +++ b/pkg/routes/quickstarts.go @@ -49,6 +49,9 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick quickstartIdsQuery := database.DB.Model(&models.QuickstartTag{}).Select("quickstart_id").Where("tag_id = ?", tag.ID) + // The hard-coded 1000 here is the default priority of a quickstart within a bundle. + // This must remain in sync with the learning-resources frontent. + database. DB. Limit(pagination.Limit). From 2943098347e9d01d85c6cdb623f086a857256893 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 17:17:31 -0400 Subject: [PATCH 06/10] Remove unnecessary MAX --- pkg/routes/quickstarts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/routes/quickstarts.go b/pkg/routes/quickstarts.go index ab1f9dd1..1d06b850 100644 --- a/pkg/routes/quickstarts.go +++ b/pkg/routes/quickstarts.go @@ -60,7 +60,7 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick Where("id IN (?)", quickstartIdsQuery). Clauses(clause.OrderBy{ Expression: clause.Expr{ - SQL: "COALESCE((SELECT MAX(priority) FROM quickstart_tags WHERE quickstart_tags.tag_id = ? AND quickstart_tags.quickstart_id = quickstarts.id), 1000)", + SQL: "COALESCE((SELECT priority FROM quickstart_tags WHERE quickstart_tags.tag_id = ? AND quickstart_tags.quickstart_id = quickstarts.id), 1000)", Vars: []interface{}{tag.ID}, }, }). From e09af3b77a6f657db532eaf2e9712cd813039f65 Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 17:24:15 -0400 Subject: [PATCH 07/10] Add warning for ignored priority --- pkg/database/db_seed.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/database/db_seed.go b/pkg/database/db_seed.go index 531af55c..2b51c37d 100644 --- a/pkg/database/db_seed.go +++ b/pkg/database/db_seed.go @@ -392,6 +392,8 @@ func SeedTags() { if newTag.Type == models.BundleTag { newLink.Priority = tag.Priority + } else if tag.Priority != nil { + logrus.Warningln("Unexpected priority for non-bundle tag in file", template.ContentPath) } err := DB.Create(&newLink).Error From 44365f02e9a6bf95fc281799b67167800383239b Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Wed, 7 Aug 2024 17:32:01 -0400 Subject: [PATCH 08/10] Fix typos --- pkg/routes/quickstarts.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/routes/quickstarts.go b/pkg/routes/quickstarts.go index 1d06b850..fbd36201 100644 --- a/pkg/routes/quickstarts.go +++ b/pkg/routes/quickstarts.go @@ -50,7 +50,7 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick quickstartIdsQuery := database.DB.Model(&models.QuickstartTag{}).Select("quickstart_id").Where("tag_id = ?", tag.ID) // The hard-coded 1000 here is the default priority of a quickstart within a bundle. - // This must remain in sync with the learning-resources frontent. + // This must remain in sync with the learning-resources frontend. database. DB. @@ -77,7 +77,7 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick func findQuickstartsByTags(tagTypes []models.TagType, tagValues []string, pagination Pagination) ([]models.Quickstart, error) { // Special case of requesting exactly a single bundle: we will return results sorted by the quickstarts' priorities - // within that bundle + // within that bundle. if len(tagTypes) == 1 && tagTypes[0] == models.BundleTag && len(tagValues) == 1 { return findBundleQuickstarts(tagValues[0], pagination) } From 2b685a96d1d6536fca5ceb80bce48c6c93c9f27a Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Thu, 8 Aug 2024 11:46:46 -0400 Subject: [PATCH 09/10] Handle multiple matching tags in findBundleQuickstarts --- pkg/routes/quickstarts.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/pkg/routes/quickstarts.go b/pkg/routes/quickstarts.go index fbd36201..3309aa74 100644 --- a/pkg/routes/quickstarts.go +++ b/pkg/routes/quickstarts.go @@ -34,7 +34,12 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick var foundTags []models.Tag var err error - database.DB.Model(&models.Tag{}).Where("type = ? AND value = ?", models.BundleTag, bundle).Limit(1).Find(&foundTags) + // We have to handle the case of multiple tags here, despite it seeming like there can only be one. + // There is no database constraint that actually enforces uniqueness, so of course non-unique tags + // do happen. For instance, two tests create tags with type=bundle and value=rhel, and if we only + // look for one tag, then we may not find what we are actually looking for. + + database.DB.Model(&models.Tag{}).Where("type = ? AND value = ?", models.BundleTag, bundle).Find(&foundTags) err = database.DB.Error if err != nil { @@ -45,9 +50,13 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick return quickstarts, nil } - tag := foundTags[0] + var tagIDs []uint + + for _, tag := range foundTags { + tagIDs = append(tagIDs, tag.ID) + } - quickstartIdsQuery := database.DB.Model(&models.QuickstartTag{}).Select("quickstart_id").Where("tag_id = ?", tag.ID) + quickstartIdsQuery := database.DB.Model(&models.QuickstartTag{}).Select("quickstart_id").Where("tag_id IN ?", tagIDs) // The hard-coded 1000 here is the default priority of a quickstart within a bundle. // This must remain in sync with the learning-resources frontend. @@ -60,8 +69,8 @@ func findBundleQuickstarts(bundle string, pagination Pagination) ([]models.Quick Where("id IN (?)", quickstartIdsQuery). Clauses(clause.OrderBy{ Expression: clause.Expr{ - SQL: "COALESCE((SELECT priority FROM quickstart_tags WHERE quickstart_tags.tag_id = ? AND quickstart_tags.quickstart_id = quickstarts.id), 1000)", - Vars: []interface{}{tag.ID}, + SQL: "COALESCE((SELECT MIN(priority) FROM quickstart_tags WHERE quickstart_tags.tag_id IN ? AND quickstart_tags.quickstart_id = quickstarts.id AND quickstart_tags.priority IS NOT NULL), 1000)", + Vars: []interface{}{tagIDs}, }, }). Find(&quickstarts) From 0e98d7586ba0842c11292cae9866f27b9415215c Mon Sep 17 00:00:00 2001 From: Janet Cobb Date: Thu, 8 Aug 2024 14:12:29 -0400 Subject: [PATCH 10/10] Test quickstart sorting --- pkg/routes/quickstarts_test.go | 39 +++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/pkg/routes/quickstarts_test.go b/pkg/routes/quickstarts_test.go index d00035b3..14a160db 100644 --- a/pkg/routes/quickstarts_test.go +++ b/pkg/routes/quickstarts_test.go @@ -16,7 +16,8 @@ import ( var quickstart models.Quickstart var taggedQuickstart models.Quickstart var settingsQuickstart models.Quickstart -var rhelQuickstart models.Quickstart +var rhelQuickstartA models.Quickstart +var rhelQuickstartB models.Quickstart var rbacQuickstart models.Quickstart var rhelBudleTag models.Tag var settingsBundleTag models.Tag @@ -77,6 +78,10 @@ func setupTags() { database.DB.Create(&unusedTag) } +func linkWithPriority(quickstart *models.Quickstart, tag *models.Tag, priority int) { + database.DB.Create(&models.QuickstartTag{QuickstartID: quickstart.ID, TagID: tag.ID, Priority: &priority}) +} + func setupTaggedQuickstarts() { taggedQuickstart.Name = "tagged-quickstart" taggedQuickstart.Content = []byte(`{"tags": "all-tags"}`) @@ -92,12 +97,17 @@ func setupTaggedQuickstarts() { database.DB.Model(&settingsQuickstart).Association("Tags").Append(&settingsBundleTag) database.DB.Save(&settingsQuickstart) - rhelQuickstart.Name = "rhel-quickstart" - rhelQuickstart.Content = []byte(`{"tags": "rhel"}`) + rhelQuickstartA.Name = "rhel-quickstart-a" + rhelQuickstartA.Content = []byte(`{"tags": "rhel"}`) + + database.DB.Create(&rhelQuickstartA) + linkWithPriority(&rhelQuickstartA, &rhelBudleTag, 1100) - database.DB.Create(&rhelQuickstart) - database.DB.Model(&rhelQuickstart).Association("Tags").Append(&rhelBudleTag) - database.DB.Save(&rhelQuickstart) + rhelQuickstartB.Name = "rhel-quickstart-b" + rhelQuickstartB.Content = []byte(`{"tags": "rhel"}`) + + database.DB.Create(&rhelQuickstartB) + linkWithPriority(&rhelQuickstartB, &rhelBudleTag, 900) rbacQuickstart.Name = "rbac-quickstart" rbacQuickstart.Content = []byte(`{"tags": "rbac"}`) @@ -121,7 +131,7 @@ func TestGetAll(t *testing.T) { var payload *responsePayload json.NewDecoder(response.Body).Decode(&payload) assert.Equal(t, 200, response.Code) - assert.Equal(t, 3, len(payload.Data)) + assert.Equal(t, 4, len(payload.Data)) }) t.Run("should get all quickstarts with 'rhel' bundle tag", func(t *testing.T) { @@ -132,7 +142,12 @@ func TestGetAll(t *testing.T) { var payload *responsePayload json.NewDecoder(response.Body).Decode(&payload) assert.Equal(t, 200, response.Code) - assert.Equal(t, 2, len(payload.Data)) + assert.Equal(t, 3, len(payload.Data)) + + // This is a request for a single bundle, so the quickstarts should be sorted in priority order. + assert.Equal(t, "rhel-quickstart-b", payload.Data[0].Name) // Priority 900 + assert.Equal(t, "tagged-quickstart", payload.Data[1].Name) // Default priority (1000) + assert.Equal(t, "rhel-quickstart-a", payload.Data[2].Name) // Priority 1100 }) t.Run("should get all quickstarts with 'settings' bundle tag", func(t *testing.T) { @@ -169,7 +184,7 @@ func TestGetAll(t *testing.T) { var payload *responsePayload json.NewDecoder(response.Body).Decode(&payload) assert.Equal(t, 200, response.Code) - assert.Equal(t, 5, len(payload.Data)) + assert.Equal(t, 6, len(payload.Data)) }) t.Run("should get quikctart by ID", func(t *testing.T) { @@ -203,10 +218,10 @@ func TestGetAll(t *testing.T) { var payload *responsePayload json.NewDecoder(response.Body).Decode(&payload) assert.Equal(t, 200, response.Code) - assert.Equal(t, 5, len(payload.Data)) + assert.Equal(t, 6, len(payload.Data)) }) - t.Run("should offset response by 2 and recover 3 records", func(t *testing.T) { + t.Run("should offset response by 2 and recover 4 records", func(t *testing.T) { request, _ := http.NewRequest(http.MethodGet, "/?offset=2", nil) response := httptest.NewRecorder() router.ServeHTTP(response, request) @@ -214,7 +229,7 @@ func TestGetAll(t *testing.T) { var payload *responsePayload json.NewDecoder(response.Body).Decode(&payload) assert.Equal(t, 200, response.Code) - assert.Equal(t, 3, len(payload.Data)) + assert.Equal(t, 4, len(payload.Data)) }) t.Run("should limit response by 2 offset response by 2 and recover 2 records", func(t *testing.T) {