From b52e72b24436e477c0da745efd36ab8647903807 Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Thu, 23 Sep 2021 19:59:40 +0200 Subject: [PATCH] Move some album resolvers to actions, refactor album tests - Album resolvers has been refactored to make it easier to test, this is done by converting some of the resolvers to actions. - The album tests has been rewritten (and simplified) to accommodate the changes. --- .husky/lint-api.sh | 2 +- api/graphql/models/actions/album_actions.go | 124 +++++++++ .../models/actions/album_actions_test.go | 178 ++++++++++++ api/graphql/models/album.go | 28 ++ api/graphql/resolvers/album.go | 141 +--------- api/graphql/resolvers/album_test.go | 257 ------------------ api/graphql/resolvers/resolvers_test.go | 12 - 7 files changed, 339 insertions(+), 403 deletions(-) create mode 100644 api/graphql/models/actions/album_actions.go create mode 100644 api/graphql/models/actions/album_actions_test.go delete mode 100644 api/graphql/resolvers/album_test.go delete mode 100644 api/graphql/resolvers/resolvers_test.go diff --git a/.husky/lint-api.sh b/.husky/lint-api.sh index 4a9a8a81..a6812155 100644 --- a/.husky/lint-api.sh +++ b/.husky/lint-api.sh @@ -1,6 +1,6 @@ #!/bin/sh -gofiles=$(git diff --cached --name-only --diff-filter=ACM | grep '.go$') +gofiles=$(git diff --cached --name-only --diff-filter=ACM | grep --silent '.go$') [ -z "$gofiles" ] && exit 0 # Automatically format go code, exit on error diff --git a/api/graphql/models/actions/album_actions.go b/api/graphql/models/actions/album_actions.go new file mode 100644 index 00000000..6787249e --- /dev/null +++ b/api/graphql/models/actions/album_actions.go @@ -0,0 +1,124 @@ +package actions + +import ( + "github.com/photoview/photoview/api/graphql/models" + "github.com/pkg/errors" + "gorm.io/gorm" +) + +func MyAlbums(db *gorm.DB, user *models.User, order *models.Ordering, paginate *models.Pagination, onlyRoot *bool, showEmpty *bool, onlyWithFavorites *bool) ([]*models.Album, error) { + if err := user.FillAlbums(db); err != nil { + return nil, err + } + + if len(user.Albums) == 0 { + return nil, nil + } + + userAlbumIDs := make([]int, len(user.Albums)) + for i, album := range user.Albums { + userAlbumIDs[i] = album.ID + } + + query := db.Model(models.Album{}).Where("id IN (?)", userAlbumIDs) + + if onlyRoot != nil && *onlyRoot { + query = query.Where("parent_album_id IS NULL") + } + + if showEmpty == nil || !*showEmpty { + subQuery := db.Model(&models.Media{}).Where("album_id = albums.id") + + if onlyWithFavorites != nil && *onlyWithFavorites { + favoritesSubquery := db. + Model(&models.UserMediaData{UserID: user.ID}). + Where("user_media_data.media_id = media.id"). + Where("user_media_data.favorite = true") + + subQuery = subQuery.Where("EXISTS (?)", favoritesSubquery) + } + + query = query.Where("EXISTS (?)", subQuery) + } + + query = models.FormatSQL(query, order, paginate) + + var albums []*models.Album + if err := query.Find(&albums).Error; err != nil { + return nil, err + } + + return albums, nil +} + +func Album(db *gorm.DB, user *models.User, id int) (*models.Album, error) { + var album models.Album + if err := db.First(&album, id).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, errors.New("album not found") + } + return nil, err + } + + ownsAlbum, err := user.OwnsAlbum(db, &album) + if err != nil { + return nil, err + } + + if !ownsAlbum { + return nil, errors.New("forbidden") + } + + return &album, nil +} + +func SetAlbumCover(db *gorm.DB, user *models.User, mediaID int) (*models.Album, error) { + var media models.Media + + if err := db.Find(&media, mediaID).Error; err != nil { + return nil, err + } + + var album models.Album + + if err := db.Find(&album, &media.AlbumID).Error; err != nil { + return nil, err + } + + ownsAlbum, err := user.OwnsAlbum(db, &album) + if err != nil { + return nil, err + } + + if !ownsAlbum { + return nil, errors.New("forbidden") + } + + if err := db.Model(&album).Update("cover_id", mediaID).Error; err != nil { + return nil, err + } + + return &album, nil +} + +func ResetAlbumCover(db *gorm.DB, user *models.User, albumID int) (*models.Album, error) { + var album models.Album + if err := db.Find(&album, albumID).Error; err != nil { + return nil, err + } + + ownsAlbum, err := user.OwnsAlbum(db, &album) + if err != nil { + return nil, err + } + + if !ownsAlbum { + return nil, errors.New("forbidden") + } + + if err := db.Model(&album).Update("cover_id", nil).Error; err != nil { + return nil, err + } + + return &album, nil +} diff --git a/api/graphql/models/actions/album_actions_test.go b/api/graphql/models/actions/album_actions_test.go new file mode 100644 index 00000000..3cdc4e8d --- /dev/null +++ b/api/graphql/models/actions/album_actions_test.go @@ -0,0 +1,178 @@ +package actions_test + +import ( + "testing" + + "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/graphql/models/actions" + "github.com/photoview/photoview/api/test_utils" + "github.com/stretchr/testify/assert" +) + +func TestAlbumCover(t *testing.T) { + db := test_utils.DatabaseTest(t) + + rootAlbum := models.Album{ + Title: "root", + Path: "/photos", + } + + if !assert.NoError(t, db.Save(&rootAlbum).Error) { + return + } + + children := []models.Album{ + { + Title: "child1", + Path: "/photos/child1", + ParentAlbumID: &rootAlbum.ID, + }, + { + Title: "child2", + Path: "/photos/child2", + ParentAlbumID: &rootAlbum.ID, + }, + } + + if !assert.NoError(t, db.Save(&children).Error) { + return + } + + photos := []models.Media{ + { + Title: "pic1", + Path: "/photos/pic1", + AlbumID: rootAlbum.ID, + }, + { + Title: "pic2", + Path: "/photos/pic2", + AlbumID: rootAlbum.ID, + }, + { + Title: "pic3", + Path: "/photos/child1/pic3", + AlbumID: children[0].ID, + }, + { + Title: "pic4", + Path: "/photos/child1/pic4", + AlbumID: children[0].ID, + }, + { + Title: "pic5", + Path: "/photos/child2/pic5", + AlbumID: children[1].ID, + }, + { + Title: "pic6", + Path: "/photos/child2/pic6", + AlbumID: children[1].ID, + }, + } + + if !assert.NoError(t, db.Save(&photos).Error) { + return + } + + if !assert.NoError(t, db.Model(&children[0]).Update("cover_id", &photos[3].ID).Error) { + return + } + + photoUrls := []models.MediaURL{ + { + MediaID: photos[0].ID, + Media: &photos[0], + }, + { + MediaID: photos[1].ID, + Media: &photos[1], + }, + { + MediaID: photos[2].ID, + Media: &photos[2], + }, + { + MediaID: photos[3].ID, + Media: &photos[3], + }, + { + MediaID: photos[4].ID, + Media: &photos[4], + }, + { + MediaID: photos[5].ID, + Media: &photos[5], + }, + } + + if !assert.NoError(t, db.Save(&photoUrls).Error) { + return + } + + user_pass := "password" + regularUser, err := models.RegisterUser(db, "user1", &user_pass, false) + if !assert.NoError(t, err) { + return + } + + if !assert.NoError(t, db.Model(®ularUser).Association("Albums").Append(&rootAlbum)) { + return + } + + if !assert.NoError(t, db.Model(®ularUser).Association("Albums").Append(&children)) { + return + } + + // Single test since we cannot rely on the tests being performed sequentially + t.Run("Album get and reset cover photos", func(t *testing.T) { + { + album, err := actions.Album(db, regularUser, rootAlbum.ID) + assert.NoError(t, err) + + albumThumb, err := album.Thumbnail(db) + assert.NoError(t, err) + + // Should return pic1 since no coverID has been set + assert.EqualValues(t, "pic1", albumThumb.Title) + } + + { + album, err := actions.Album(db, regularUser, children[0].ID) + assert.NoError(t, err) + + albumThumb, err := album.Thumbnail(db) + assert.NoError(t, err) + + // coverID has already been set + assert.EqualValues(t, "pic4", albumThumb.Title) + } + + resetAlbum, err := actions.ResetAlbumCover(db, regularUser, children[0].ID) + assert.NoError(t, err) + + assert.Nil(t, resetAlbum.CoverID) + + resetThumb, err := resetAlbum.Thumbnail(db) + assert.NoError(t, err) + + assert.Equal(t, "pic3", resetThumb.Title) + }) + + t.Run("Album change cover photos", func(t *testing.T) { + assert.Nil(t, children[1].CoverID) + + album, err := actions.SetAlbumCover(db, regularUser, photos[4].ID) + assert.NoError(t, err) + + assert.Equal(t, children[1].ID, album.ID) + assert.NotNil(t, album.CoverID) + assert.Equal(t, photos[4].ID, *album.CoverID) + + albumThumb, err := album.Thumbnail(db) + assert.NoError(t, err) + + assert.Equal(t, photos[4].ID, albumThumb.ID) + }) + +} diff --git a/api/graphql/models/album.go b/api/graphql/models/album.go index 75625488..a58f622c 100644 --- a/api/graphql/models/album.go +++ b/api/graphql/models/album.go @@ -79,3 +79,31 @@ func GetParentsFromAlbums(db *gorm.DB, filter func(*gorm.DB) *gorm.DB, albumID i return parents, err } + +func (a *Album) Thumbnail(db *gorm.DB) (*Media, error) { + var media Media + + if a.CoverID == nil { + if err := db.Raw(` + WITH recursive sub_albums AS ( + SELECT * FROM albums AS root WHERE id = ? + UNION ALL + SELECT child.* FROM albums AS child JOIN sub_albums ON child.parent_album_id = sub_albums.id + ) + + SELECT * FROM media WHERE media.album_id IN ( + SELECT id FROM sub_albums + ) AND media.id IN ( + SELECT media_id FROM media_urls WHERE media_urls.media_id = media.id + ) LIMIT 1 + `, a.ID).Find(&media).Error; err != nil { + return nil, err + } + } else { + if err := db.Where("id = ?", a.CoverID).Find(&media).Error; err != nil { + return nil, err + } + } + + return &media, nil +} diff --git a/api/graphql/resolvers/album.go b/api/graphql/resolvers/album.go index 5b1c71ee..88970793 100644 --- a/api/graphql/resolvers/album.go +++ b/api/graphql/resolvers/album.go @@ -6,6 +6,7 @@ import ( api "github.com/photoview/photoview/api/graphql" "github.com/photoview/photoview/api/graphql/auth" "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/graphql/models/actions" "github.com/pkg/errors" "gorm.io/gorm" ) @@ -16,48 +17,7 @@ func (r *queryResolver) MyAlbums(ctx context.Context, order *models.Ordering, pa return nil, auth.ErrUnauthorized } - if err := user.FillAlbums(r.Database); err != nil { - return nil, err - } - - if len(user.Albums) == 0 { - return nil, nil - } - - userAlbumIDs := make([]int, len(user.Albums)) - for i, album := range user.Albums { - userAlbumIDs[i] = album.ID - } - - query := r.Database.Model(models.Album{}).Where("id IN (?)", userAlbumIDs) - - if onlyRoot != nil && *onlyRoot == true { - query = query.Where("parent_album_id IS NULL") - } - - if showEmpty == nil || *showEmpty == false { - subQuery := r.Database.Model(&models.Media{}).Where("album_id = albums.id") - - if onlyWithFavorites != nil && *onlyWithFavorites == true { - favoritesSubquery := r.Database. - Model(&models.UserMediaData{UserID: user.ID}). - Where("user_media_data.media_id = media.id"). - Where("user_media_data.favorite = true") - - subQuery = subQuery.Where("EXISTS (?)", favoritesSubquery) - } - - query = query.Where("EXISTS (?)", subQuery) - } - - query = models.FormatSQL(query, order, paginate) - - var albums []*models.Album - if err := query.Find(&albums).Error; err != nil { - return nil, err - } - - return albums, nil + return actions.MyAlbums(r.Database, user, order, paginate, onlyRoot, showEmpty, onlyWithFavorites) } func (r *queryResolver) Album(ctx context.Context, id int, tokenCredentials *models.ShareTokenCredentials) (*models.Album, error) { @@ -89,24 +49,7 @@ func (r *queryResolver) Album(ctx context.Context, id int, tokenCredentials *mod return nil, auth.ErrUnauthorized } - var album models.Album - if err := r.Database.First(&album, id).Error; err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - return nil, errors.New("album not found") - } - return nil, err - } - - ownsAlbum, err := user.OwnsAlbum(r.Database, &album) - if err != nil { - return nil, err - } - - if !ownsAlbum { - return nil, errors.New("forbidden") - } - - return &album, nil + return actions.Album(r.Database, user, id) } func (r *Resolver) Album() api.AlbumResolver { @@ -144,33 +87,8 @@ func (r *albumResolver) Media(ctx context.Context, album *models.Album, order *m return media, nil } -func (r *albumResolver) Thumbnail(ctx context.Context, obj *models.Album) (*models.Media, error) { - - var media models.Media - - if obj.CoverID == nil { - if err := r.Database.Raw(` - WITH recursive sub_albums AS ( - SELECT * FROM albums AS root WHERE id = ? - UNION ALL - SELECT child.* FROM albums AS child JOIN sub_albums ON child.parent_album_id = sub_albums.id - ) - - SELECT * FROM media WHERE media.album_id IN ( - SELECT id FROM sub_albums - ) AND media.id IN ( - SELECT media_id FROM media_urls WHERE media_urls.media_id = media.id - ) LIMIT 1 - `, obj.ID).Find(&media).Error; err != nil { - return nil, err - } - } else { - if err := r.Database.Where("id = ?", obj.CoverID).Find(&media).Error; err != nil { - return nil, err - } - } - - return &media, nil +func (r *albumResolver) Thumbnail(ctx context.Context, album *models.Album) (*models.Media, error) { + return album.Thumbnail(r.Database) } func (r *albumResolver) SubAlbums(ctx context.Context, parent *models.Album, order *models.Ordering, paginate *models.Pagination) ([]*models.Album, error) { @@ -254,58 +172,15 @@ func (r *mutationResolver) ResetAlbumCover(ctx context.Context, albumID int) (*m return nil, errors.New("unauthorized") } - var album models.Album - if err := r.Database.Find(&album, albumID).Error; err != nil { - return nil, err - } - - ownsAlbum, err := user.OwnsAlbum(r.Database, &album) - if err != nil { - return nil, err - } - - if !ownsAlbum { - return nil, errors.New("forbidden") - } - - if err := r.Database.Model(&album).Update("cover_id", nil).Error; err != nil { - return nil, err - } - - return &album, nil + return actions.ResetAlbumCover(r.Database, user, albumID) } // Takes media.id, finds parent album, sets album.cover_id to media.id (must be a more efficient way of doing this, but it works) -func (r *mutationResolver) SetAlbumCover(ctx context.Context, coverID int) (*models.Album, error) { +func (r *mutationResolver) SetAlbumCover(ctx context.Context, mediaID int) (*models.Album, error) { user := auth.UserFromContext(ctx) if user == nil { return nil, errors.New("unauthorized") } - var media models.Media - - if err := r.Database.Find(&media, coverID).Error; err != nil { - return nil, err - } - - var album models.Album - - if err := r.Database.Find(&album, &media.AlbumID).Error; err != nil { - return nil, err - } - - ownsAlbum, err := user.OwnsAlbum(r.Database, &album) - if err != nil { - return nil, err - } - - if !ownsAlbum { - return nil, errors.New("forbidden") - } - - if err := r.Database.Model(&album).Update("cover_id", coverID).Error; err != nil { - return nil, err - } - - return &album, nil + return actions.SetAlbumCover(r.Database, user, mediaID) } diff --git a/api/graphql/resolvers/album_test.go b/api/graphql/resolvers/album_test.go deleted file mode 100644 index 6fc4850f..00000000 --- a/api/graphql/resolvers/album_test.go +++ /dev/null @@ -1,257 +0,0 @@ -package resolvers_test - -import ( - "context" - "testing" - - api "github.com/photoview/photoview/api/graphql" - "github.com/photoview/photoview/api/graphql/models" - "github.com/photoview/photoview/api/graphql/resolvers" - - "github.com/99designs/gqlgen/client" - "github.com/99designs/gqlgen/graphql/handler" - "github.com/photoview/photoview/api/graphql/auth" - "github.com/photoview/photoview/api/test_utils" - "github.com/stretchr/testify/assert" -) - -func addContext(ctx context.Context) client.Option { - return func(bd *client.Request) { - bd.HTTP = bd.HTTP.WithContext(ctx) - } -} - -func TestAlbumCover(t *testing.T) { - db := test_utils.DatabaseTest(t) - - rootAlbum := models.Album{ - Title: "root", - Path: "/photos", - } - - if !assert.NoError(t, db.Save(&rootAlbum).Error) { - return - } - - children := []models.Album{ - { - Title: "child1", - Path: "/photos/child1", - ParentAlbumID: &rootAlbum.ID, - }, - { - Title: "child2", - Path: "/photos/child2", - ParentAlbumID: &rootAlbum.ID, - }, - } - - if !assert.NoError(t, db.Save(&children).Error) { - return - } - - photos := []models.Media{ - { - Title: "pic1", - Path: "/photos/pic1", - AlbumID: rootAlbum.ID, - }, - { - Title: "pic2", - Path: "/photos/pic2", - AlbumID: rootAlbum.ID, - }, - { - Title: "pic3", - Path: "/photos/child1/pic3", - AlbumID: children[0].ID, - }, - { - Title: "pic4", - Path: "/photos/child1/pic4", - AlbumID: children[0].ID, - }, - { - Title: "pic5", - Path: "/photos/child2/pic5", - AlbumID: children[1].ID, - }, - { - Title: "pic6", - Path: "/photos/child2/pic6", - AlbumID: children[1].ID, - }, - } - - if !assert.NoError(t, db.Save(&photos).Error) { - return - } - - if !assert.NoError(t, db.Model(&children[1]).Update("cover_id", &photos[5].ID).Error) { - return - } - - if !assert.NoError(t, db.Model(&children[0]).Update("cover_id", &photos[3].ID).Error) { - return - } - - photoUrls := []models.MediaURL{ - { - MediaID: photos[0].ID, - Media: &photos[0], - }, - { - MediaID: photos[1].ID, - Media: &photos[1], - }, - { - MediaID: photos[2].ID, - Media: &photos[2], - }, - { - MediaID: photos[3].ID, - Media: &photos[3], - }, - { - MediaID: photos[4].ID, - Media: &photos[4], - }, - { - MediaID: photos[5].ID, - Media: &photos[5], - }, - } - - if !assert.NoError(t, db.Save(&photoUrls).Error) { - return - } - - pass := "" - regularUser := models.User{ - Username: "user1", - Password: &pass, - Admin: false, - } - - if !assert.NoError(t, db.Save(®ularUser).Error) { - return - } - - if !assert.NoError(t, db.Model(®ularUser).Association("Albums").Append(&rootAlbum)) { - return - } - - if !assert.NoError(t, db.Model(®ularUser).Association("Albums").Append(&children)) { - return - } - - ctx := auth.AddUserToContext(context.TODO(), ®ularUser) - - c := client.New(handler.NewDefaultServer(api.NewExecutableSchema(api.Config{ - Resolvers: &resolvers.Resolver{ - Database: db, - }, - Directives: api.DirectiveRoot{ - IsAuthorized: api.IsAuthorized, - }, - }))) - - // Single test since we cannot rely on the tests being performed sequentially - t.Run("Album get and reset cover photos", func(t *testing.T) { - - var resp struct { - Album struct { - Thumbnail struct { - Title string - } - } - } - - q := `query ($albumID: ID!){ album (id: $albumID) { - thumbnail { - title - } - }} - ` - - postErr := c.Post( - q, - &resp, - client.Var("albumID", &rootAlbum.ID), - addContext(ctx), - ) - if !assert.NoError(t, postErr) { - return - } - - // Should return pic1 since no coverID has been set - assert.EqualValues(t, "pic1", resp.Album.Thumbnail.Title) - - qErr := c.Post( - q, - &resp, - client.Var("albumID", &children[0].ID), - addContext(ctx), - ) - if !assert.NoError(t, qErr) { - return - } - - // coverID has already been set - assert.EqualValues(t, "pic4", resp.Album.Thumbnail.Title) - - var resetResp struct { - ResetAlbumCover struct { - CoverID int - } - } - - m := `mutation resetCover($albumID: ID!) { - resetAlbumCover(albumID: $albumID) { - coverID - } - } - ` - mErr := c.Post( - m, - &resetResp, - client.Var("albumID", &children[0].ID), - addContext(ctx), - ) - if !assert.NoError(t, mErr) { - return - } - - assert.EqualValues(t, 0, resetResp.ResetAlbumCover.CoverID) - }) - - t.Run("Album change cover photos", func(t *testing.T) { - - var resp struct { - SetAlbumCover struct { - CoverID int - } - } - - q := `mutation changeCover($coverID: ID!) { - setAlbumCover(coverID: $coverID) { - coverID, - } - } - ` - - postErr := c.Post( - q, - &resp, - client.Var("coverID", &photos[4].ID), - addContext(ctx), - ) - if !assert.NoError(t, postErr) { - return - } - - assert.EqualValues(t, &photos[4].ID, &resp.SetAlbumCover.CoverID) - - }) - -} diff --git a/api/graphql/resolvers/resolvers_test.go b/api/graphql/resolvers/resolvers_test.go deleted file mode 100644 index 5cc39cee..00000000 --- a/api/graphql/resolvers/resolvers_test.go +++ /dev/null @@ -1,12 +0,0 @@ -package resolvers_test - -import ( - "os" - "testing" - - "github.com/photoview/photoview/api/test_utils" -) - -func TestMain(m *testing.M) { - os.Exit(test_utils.IntegrationTestRun(m)) -}