From 52cf817c8e28c675eeb129ee2c7030faf9d98f93 Mon Sep 17 00:00:00 2001 From: Rodrigo Villablanca Date: Tue, 2 Jun 2020 13:17:52 -0400 Subject: [PATCH] SqlTermsOfService migrates to plain errors (#14675) * Advances migragint TermsOfService * Advances... * Replaced message key * Replaced message key * Replaced message key * Replaced message key * Refactor name of errors * Fix mixed type of error * Fix-imports * i18n-extract * Rollback changes * Rollback changes * Rollback changes Co-authored-by: mattermod --- api4/user_test.go | 2 +- app/config.go | 2 +- app/terms_of_service.go | 48 +++++++++++++++++-- i18n/en.json | 32 ++++++------- .../localcachelayer/terms_of_service_layer.go | 6 +-- store/opentracing_layer.go | 6 +-- store/sqlstore/terms_of_service_store.go | 21 ++++---- store/store.go | 6 +-- store/storetest/mocks/TermsOfServiceStore.go | 30 +++++------- store/storetest/terms_of_service_store.go | 16 +++---- store/timer_layer.go | 6 +-- 11 files changed, 104 insertions(+), 71 deletions(-) diff --git a/api4/user_test.go b/api4/user_test.go index f060dc8dbd..8bfee3b95f 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -4285,7 +4285,7 @@ func TestRegisterTermsOfServiceAction(t *testing.T) { defer th.TearDown() success, resp := th.Client.RegisterTermsOfServiceAction(th.BasicUser.Id, "st_1", true) - CheckErrorMessage(t, resp, "store.sql_terms_of_service_store.get.no_rows.app_error") + CheckErrorMessage(t, resp, "app.terms_of_service.get.no_rows.app_error") assert.Nil(t, success) termsOfService, err := th.App.CreateTermsOfService("terms of service", th.BasicUser.Id) diff --git a/app/config.go b/app/config.go index 43be07ff0a..1d4c9edc01 100644 --- a/app/config.go +++ b/app/config.go @@ -27,7 +27,7 @@ import ( ) const ( - ERROR_TERMS_OF_SERVICE_NO_ROWS_FOUND = "store.sql_terms_of_service_store.get.no_rows.app_error" + ERROR_TERMS_OF_SERVICE_NO_ROWS_FOUND = "app.terms_of_service.get.no_rows.app_error" ) func (s *Server) Config() *model.Config { diff --git a/app/terms_of_service.go b/app/terms_of_service.go index 529dd03c43..d52086a02a 100644 --- a/app/terms_of_service.go +++ b/app/terms_of_service.go @@ -4,7 +4,11 @@ package app import ( + "errors" + "net/http" + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store" ) func (a *App) CreateTermsOfService(text, userId string) (*model.TermsOfService, *model.AppError) { @@ -13,17 +17,51 @@ func (a *App) CreateTermsOfService(text, userId string) (*model.TermsOfService, UserId: userId, } - if _, err := a.GetUser(userId); err != nil { - return nil, err + if _, appErr := a.GetUser(userId); appErr != nil { + return nil, appErr } - return a.Srv().Store.TermsOfService().Save(termsOfService) + var err error + if termsOfService, err = a.Srv().Store.TermsOfService().Save(termsOfService); err != nil { + var iErr *store.ErrInvalidInput + var appErr *model.AppError + switch { + case errors.As(err, &iErr): + return nil, model.NewAppError("CreateTermsOfService", "app.terms_of_service.create.existing.app_error", nil, "id="+termsOfService.Id, http.StatusBadRequest) + case errors.As(err, &appErr): + return nil, appErr + default: + return nil, model.NewAppError("CreateTermsOfService", "app.terms_of_service.create.app_error", nil, "terms_of_service_id="+termsOfService.Id+",err="+err.Error(), http.StatusInternalServerError) + } + } + + return termsOfService, nil } func (a *App) GetLatestTermsOfService() (*model.TermsOfService, *model.AppError) { - return a.Srv().Store.TermsOfService().GetLatest(true) + termsOfService, err := a.Srv().Store.TermsOfService().GetLatest(true) + if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + return nil, model.NewAppError("GetLatestTermsOfService", "app.terms_of_service.get.no_rows.app_error", nil, "err="+err.Error(), http.StatusNotFound) + default: + return nil, model.NewAppError("GetLatestTermsOfService", "app.terms_of_service.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) + } + } + return termsOfService, nil } func (a *App) GetTermsOfService(id string) (*model.TermsOfService, *model.AppError) { - return a.Srv().Store.TermsOfService().Get(id, true) + termsOfService, err := a.Srv().Store.TermsOfService().Get(id, true) + if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + return nil, model.NewAppError("GetTermsOfService", "app.terms_of_service.get.no_rows.app_error", nil, "", http.StatusNotFound) + default: + return nil, model.NewAppError("GetTermsOfService", "app.terms_of_service.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) + } + } + return termsOfService, nil } diff --git a/i18n/en.json b/i18n/en.json index cdc44676d6..8c73abfe18 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3846,6 +3846,22 @@ "id": "app.team.rename_team.name_occupied", "translation": "Unable to rename the team, the name is already in use." }, + { + "id": "app.terms_of_service.create.app_error", + "translation": "Unable to save terms of service." + }, + { + "id": "app.terms_of_service.create.existing.app_error", + "translation": "Must not call save for existing terms of service." + }, + { + "id": "app.terms_of_service.get.app_error", + "translation": "Unable to fetch terms of service." + }, + { + "id": "app.terms_of_service.get.no_rows.app_error", + "translation": "No terms of service found." + }, { "id": "app.user.complete_switch_with_oauth.blank_email.app_error", "translation": "Unable to complete SAML login with an empty email address." @@ -7294,22 +7310,6 @@ "id": "store.sql_team.user_belongs_to_teams.app_error", "translation": "Unable to determine if the user belongs to a list of teams." }, - { - "id": "store.sql_terms_of_service.save.app_error", - "translation": "Unable to save terms of service." - }, - { - "id": "store.sql_terms_of_service_store.get.app_error", - "translation": "Unable to fetch terms of service." - }, - { - "id": "store.sql_terms_of_service_store.get.no_rows.app_error", - "translation": "No terms of service found." - }, - { - "id": "store.sql_terms_of_service_store.save.existing.app_error", - "translation": "Must not call save for existing terms of service." - }, { "id": "store.sql_user.analytics_daily_active_users.app_error", "translation": "Unable to get the active users during the requested period." diff --git a/store/localcachelayer/terms_of_service_layer.go b/store/localcachelayer/terms_of_service_layer.go index 41a798b48c..2a25a93303 100644 --- a/store/localcachelayer/terms_of_service_layer.go +++ b/store/localcachelayer/terms_of_service_layer.go @@ -33,7 +33,7 @@ func (s LocalCacheTermsOfServiceStore) ClearCaches() { } } -func (s LocalCacheTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { +func (s LocalCacheTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) { tos, err := s.TermsOfServiceStore.Save(termsOfService) if err == nil { @@ -43,7 +43,7 @@ func (s LocalCacheTermsOfServiceStore) Save(termsOfService *model.TermsOfService return tos, err } -func (s LocalCacheTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s LocalCacheTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { if allowFromCache { if s.rootStore.termsOfServiceCache.Len() != 0 { if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, LATEST_KEY); cacheItem != nil { @@ -62,7 +62,7 @@ func (s LocalCacheTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.Te return termsOfService, err } -func (s LocalCacheTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s LocalCacheTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, error) { if allowFromCache { if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, id); cacheItem != nil { return cacheItem.(*model.TermsOfService), nil diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 3d7f6e0667..0e9c8a01fb 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -7095,7 +7095,7 @@ func (s *OpenTracingLayerTeamStore) UserBelongsToTeams(userId string, teamIds [] return resultVar0, resultVar1 } -func (s *OpenTracingLayerTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s *OpenTracingLayerTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TermsOfServiceStore.Get") s.Root.Store.SetContext(newCtx) @@ -7113,7 +7113,7 @@ func (s *OpenTracingLayerTermsOfServiceStore) Get(id string, allowFromCache bool return resultVar0, resultVar1 } -func (s *OpenTracingLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s *OpenTracingLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TermsOfServiceStore.GetLatest") s.Root.Store.SetContext(newCtx) @@ -7131,7 +7131,7 @@ func (s *OpenTracingLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*m return resultVar0, resultVar1 } -func (s *OpenTracingLayerTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { +func (s *OpenTracingLayerTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TermsOfServiceStore.Save") s.Root.Store.SetContext(newCtx) diff --git a/store/sqlstore/terms_of_service_store.go b/store/sqlstore/terms_of_service_store.go index 69a7bf7410..49645996e0 100644 --- a/store/sqlstore/terms_of_service_store.go +++ b/store/sqlstore/terms_of_service_store.go @@ -5,11 +5,12 @@ package sqlstore import ( "database/sql" - "net/http" "github.com/mattermost/mattermost-server/v5/einterfaces" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" + + "github.com/pkg/errors" ) type SqlTermsOfServiceStore struct { @@ -33,9 +34,9 @@ func newSqlTermsOfServiceStore(sqlStore SqlStore, metrics einterfaces.MetricsInt func (s SqlTermsOfServiceStore) createIndexesIfNotExists() { } -func (s SqlTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { +func (s SqlTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) { if len(termsOfService.Id) > 0 { - return nil, model.NewAppError("SqlTermsOfServiceStore.Save", "store.sql_terms_of_service_store.save.existing.app_error", nil, "id="+termsOfService.Id, http.StatusBadRequest) + return nil, store.NewErrInvalidInput("TermsOfService", "Id", termsOfService.Id) } termsOfService.PreSave() @@ -45,33 +46,33 @@ func (s SqlTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*mod } if err := s.GetMaster().Insert(termsOfService); err != nil { - return nil, model.NewAppError("SqlTermsOfServiceStore.Save", "store.sql_terms_of_service.save.app_error", nil, "terms_of_service_id="+termsOfService.Id+",err="+err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "could not save a new TermsOfService") } return termsOfService, nil } -func (s SqlTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s SqlTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { var termsOfService *model.TermsOfService err := s.GetReplica().SelectOne(&termsOfService, "SELECT * FROM TermsOfService ORDER BY CreateAt DESC LIMIT 1") if err != nil { if err == sql.ErrNoRows { - return nil, model.NewAppError("SqlTermsOfServiceStore.GetLatest", "store.sql_terms_of_service_store.get.no_rows.app_error", nil, "err="+err.Error(), http.StatusNotFound) + return nil, store.NewErrNotFound("TermsOfService", "CreateAt=latest") } - return nil, model.NewAppError("SqlTermsOfServiceStore.GetLatest", "store.sql_terms_of_service_store.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "could not find latest TermsOfService") } return termsOfService, nil } -func (s SqlTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s SqlTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, error) { obj, err := s.GetReplica().Get(model.TermsOfService{}, id) if err != nil { - return nil, model.NewAppError("SqlTermsOfServiceStore.Get", "store.sql_terms_of_service_store.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "could not find TermsOfService with id=%s", id) } if obj == nil { - return nil, model.NewAppError("SqlTermsOfServiceStore.GetLatest", "store.sql_terms_of_service_store.get.no_rows.app_error", nil, "", http.StatusNotFound) + return nil, store.NewErrNotFound("TermsOfService", id) } return obj.(*model.TermsOfService), nil } diff --git a/store/store.go b/store/store.go index 54eb2e72b0..f06405d329 100644 --- a/store/store.go +++ b/store/store.go @@ -613,9 +613,9 @@ type SchemeStore interface { } type TermsOfServiceStore interface { - Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) - GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) - Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) + Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) + GetLatest(allowFromCache bool) (*model.TermsOfService, error) + Get(id string, allowFromCache bool) (*model.TermsOfService, error) } type UserTermsOfServiceStore interface { diff --git a/store/storetest/mocks/TermsOfServiceStore.go b/store/storetest/mocks/TermsOfServiceStore.go index c839559028..ffa02b5700 100644 --- a/store/storetest/mocks/TermsOfServiceStore.go +++ b/store/storetest/mocks/TermsOfServiceStore.go @@ -15,7 +15,7 @@ type TermsOfServiceStore struct { } // Get provides a mock function with given fields: id, allowFromCache -func (_m *TermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (_m *TermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, error) { ret := _m.Called(id, allowFromCache) var r0 *model.TermsOfService @@ -27,20 +27,18 @@ func (_m *TermsOfServiceStore) Get(id string, allowFromCache bool) (*model.Terms } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, bool) error); ok { r1 = rf(id, allowFromCache) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // GetLatest provides a mock function with given fields: allowFromCache -func (_m *TermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (_m *TermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { ret := _m.Called(allowFromCache) var r0 *model.TermsOfService @@ -52,20 +50,18 @@ func (_m *TermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfSer } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(bool) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(bool) error); ok { r1 = rf(allowFromCache) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // Save provides a mock function with given fields: termsOfService -func (_m *TermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { +func (_m *TermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) { ret := _m.Called(termsOfService) var r0 *model.TermsOfService @@ -77,13 +73,11 @@ func (_m *TermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*mode } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.TermsOfService) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.TermsOfService) error); ok { r1 = rf(termsOfService) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 diff --git a/store/storetest/terms_of_service_store.go b/store/storetest/terms_of_service_store.go index 22ce9d561a..1d6fb04f30 100644 --- a/store/storetest/terms_of_service_store.go +++ b/store/storetest/terms_of_service_store.go @@ -23,8 +23,8 @@ func testSaveTermsOfService(t *testing.T, ss store.Store) { u1.Username = model.NewId() u1.Email = MakeEmail() u1.Nickname = model.NewId() - _, err := ss.User().Save(&u1) - require.Nil(t, err) + _, appErr := ss.User().Save(&u1) + require.Nil(t, appErr) termsOfService := &model.TermsOfService{Text: "terms of service", UserId: u1.Id} savedTermsOfService, err := ss.TermsOfService().Save(termsOfService) @@ -40,11 +40,11 @@ func testGetLatestTermsOfService(t *testing.T, ss store.Store) { u1.Username = model.NewId() u1.Email = MakeEmail() u1.Nickname = model.NewId() - _, err := ss.User().Save(&u1) - require.Nil(t, err) + _, appErr := ss.User().Save(&u1) + require.Nil(t, appErr) termsOfService := &model.TermsOfService{Text: "terms of service", UserId: u1.Id} - _, err = ss.TermsOfService().Save(termsOfService) + _, err := ss.TermsOfService().Save(termsOfService) require.Nil(t, err) fetchedTermsOfService, err := ss.TermsOfService().GetLatest(true) @@ -58,11 +58,11 @@ func testGetTermsOfService(t *testing.T, ss store.Store) { u1.Username = model.NewId() u1.Email = MakeEmail() u1.Nickname = model.NewId() - _, err := ss.User().Save(&u1) - require.Nil(t, err) + _, appErr := ss.User().Save(&u1) + require.Nil(t, appErr) termsOfService := &model.TermsOfService{Text: "terms of service", UserId: u1.Id} - _, err = ss.TermsOfService().Save(termsOfService) + _, err := ss.TermsOfService().Save(termsOfService) require.Nil(t, err) r1, err := ss.TermsOfService().Get("an_invalid_id", true) diff --git a/store/timer_layer.go b/store/timer_layer.go index 2232bd5a35..486e1ae33e 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -6409,7 +6409,7 @@ func (s *TimerLayerTeamStore) UserBelongsToTeams(userId string, teamIds []string return resultVar0, resultVar1 } -func (s *TimerLayerTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s *TimerLayerTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, error) { start := timemodule.Now() resultVar0, resultVar1 := s.TermsOfServiceStore.Get(id, allowFromCache) @@ -6425,7 +6425,7 @@ func (s *TimerLayerTermsOfServiceStore) Get(id string, allowFromCache bool) (*mo return resultVar0, resultVar1 } -func (s *TimerLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { +func (s *TimerLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { start := timemodule.Now() resultVar0, resultVar1 := s.TermsOfServiceStore.GetLatest(allowFromCache) @@ -6441,7 +6441,7 @@ func (s *TimerLayerTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.T return resultVar0, resultVar1 } -func (s *TimerLayerTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { +func (s *TimerLayerTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, error) { start := timemodule.Now() resultVar0, resultVar1 := s.TermsOfServiceStore.Save(termsOfService)