[MM-48560] LastAccessiblePostTime not removed on upgrade to Professional (#21708)

* Delete system value for LastAccessibleFileTime and LastAccessiblePostTime if the limits are 0 and a value is set

* Update tests

app/file.go

@@ -1380,6 +1380,28 @@ func (a *App) ComputeLastAccessibleFileTime() error {
 		return appErr
 	}
 
+	if limit == 0 {
+		// All files are accessible - we must check if a previous value was set so we can clear it
+		systemValue, err := a.Srv().Store().System().GetByName(model.SystemLastAccessibleFileTime)
+		if err != nil {
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(err, &nfErr):
+				// All files are already accessible
+				return nil
+			default:
+				return model.NewAppError("ComputeLastAccessibleFileTime", "app.system.get_by_name.app_error", nil, err.Error(), http.StatusInternalServerError)
+			}
+		}
+		if systemValue != nil {
+			// Previous value was set, so we must clear it
+			if _, err := a.Srv().Store().System().PermanentDeleteByName(model.SystemLastAccessibleFileTime); err != nil {
+				return model.NewAppError("ComputeLastAccessibleFileTime", "app.system.permanent_delete_by_name.app_error", nil, err.Error(), http.StatusInternalServerError)
+			}
+		}
+		return nil
+	}
+
 	createdAt, err := a.Srv().GetStore().FileInfo().GetUptoNSizeFileTime(limit)
 	if err != nil {
 		var nfErr *store.ErrNotFound

app/file_test.go

@@ -591,30 +591,61 @@ func TestGetLastAccessibleFileTime(t *testing.T) {
 }
 
 func TestComputeLastAccessibleFileTime(t *testing.T) {
-	th := SetupWithStoreMock(t)
-	defer th.TearDown()
+	t.Run("Updates the time, if cloud limit is applicable", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
 
-	th.App.Srv().SetLicense(model.NewTestLicense("cloud"))
+		th.App.Srv().SetLicense(model.NewTestLicense("cloud"))
 
-	cloud := &eMocks.CloudInterface{}
-	th.App.Srv().Cloud = cloud
+		cloud := &eMocks.CloudInterface{}
+		th.App.Srv().Cloud = cloud
 
-	cloud.Mock.On("GetCloudLimits", mock.Anything).Return(&model.ProductLimits{
-		Files: &model.FilesLimits{
-			TotalStorage: model.NewInt64(1),
-		},
-	}, nil)
+		cloud.Mock.On("GetCloudLimits", mock.Anything).Return(&model.ProductLimits{
+			Files: &model.FilesLimits{
+				TotalStorage: model.NewInt64(1),
+			},
+		}, nil)
 
-	mockStore := th.App.Srv().Store().(*storemocks.Store)
-	mockFileStore := storemocks.FileInfoStore{}
-	mockFileStore.On("GetUptoNSizeFileTime", mock.Anything).Return(int64(1), nil)
-	mockSystemStore := storemocks.SystemStore{}
-	mockSystemStore.On("SaveOrUpdate", mock.Anything).Return(nil)
-	mockStore.On("FileInfo").Return(&mockFileStore)
-	mockStore.On("System").Return(&mockSystemStore)
+		mockStore := th.App.Srv().Store().(*storemocks.Store)
+		mockFileStore := storemocks.FileInfoStore{}
+		mockFileStore.On("GetUptoNSizeFileTime", mock.Anything).Return(int64(1), nil)
+		mockSystemStore := storemocks.SystemStore{}
+		mockSystemStore.On("SaveOrUpdate", mock.Anything).Return(nil)
+		mockStore.On("FileInfo").Return(&mockFileStore)
+		mockStore.On("System").Return(&mockSystemStore)
 
-	err := th.App.ComputeLastAccessibleFileTime()
-	require.NoError(t, err)
+		err := th.App.ComputeLastAccessibleFileTime()
+		require.NoError(t, err)
 
-	mockSystemStore.AssertCalled(t, "SaveOrUpdate", mock.Anything)
+		mockSystemStore.AssertCalled(t, "SaveOrUpdate", mock.Anything)
+	})
+
+	t.Run("Removes the time, if cloud limit is not applicable", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		th.App.Srv().SetLicense(model.NewTestLicense("cloud"))
+
+		cloud := &eMocks.CloudInterface{}
+		th.App.Srv().Cloud = cloud
+
+		cloud.Mock.On("GetCloudLimits", mock.Anything).Return(nil, nil)
+
+		mockStore := th.App.Srv().Store().(*storemocks.Store)
+		mockFileStore := storemocks.FileInfoStore{}
+		mockFileStore.On("GetUptoNSizeFileTime", mock.Anything).Return(int64(1), nil)
+		mockSystemStore := storemocks.SystemStore{}
+		mockSystemStore.On("GetByName", mock.Anything).Return(&model.System{Name: model.SystemLastAccessibleFileTime, Value: "10"}, nil)
+		mockSystemStore.On("PermanentDeleteByName", mock.Anything).Return(nil, nil)
+		mockSystemStore.On("SaveOrUpdate", mock.Anything).Return(nil)
+		mockStore.On("FileInfo").Return(&mockFileStore)
+		mockStore.On("System").Return(&mockSystemStore)
+
+		err := th.App.ComputeLastAccessibleFileTime()
+		require.NoError(t, err)
+
+		mockSystemStore.AssertNotCalled(t, "SaveOrUpdate", mock.Anything)
+		mockSystemStore.AssertCalled(t, "PermanentDeleteByName", mock.Anything)
+
+	})
 }

app/post.go

@@ -1441,6 +1441,24 @@ func (a *App) ComputeLastAccessiblePostTime() error {
 	}
 
 	if limit == 0 {
+		// All posts are accessible - we must check if a previous value was set so we can clear it
+		systemValue, err := a.Srv().Store().System().GetByName(model.SystemLastAccessiblePostTime)
+		if err != nil {
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(err, &nfErr):
+				// There was no previous value, nothing to do
+				return nil
+			default:
+				return model.NewAppError("ComputeLastAccessiblePostTime", "app.system.get_by_name.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+			}
+		}
+		if systemValue != nil {
+			// Previous value was set, so we must clear it
+			if _, err = a.Srv().Store().System().PermanentDeleteByName(model.SystemLastAccessiblePostTime); err != nil {
+				return model.NewAppError("ComputeLastAccessiblePostTime", "app.system.permanent_delete_by_name.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+			}
+		}
 		// Cloud limit is not applicable
 		return nil
 	}

app/post_test.go

@@ -2887,7 +2887,7 @@ func TestComputeLastAccessiblePostTime(t *testing.T) {
 		mockSystemStore.AssertCalled(t, "SaveOrUpdate", mock.Anything)
 	})
 
-	t.Run("Do NOT update the time, if cloud limit is NOT applicable", func(t *testing.T) {
+	t.Run("Remove the time if cloud limit is NOT applicable", func(t *testing.T) {
 		th := SetupWithStoreMock(t)
 		defer th.TearDown()
 
@@ -2901,13 +2901,15 @@ func TestComputeLastAccessiblePostTime(t *testing.T) {
 
 		mockStore := th.App.Srv().Store().(*storemocks.Store)
 		mockSystemStore := storemocks.SystemStore{}
-		mockSystemStore.On("SaveOrUpdate", mock.Anything).Return(nil)
+		mockSystemStore.On("GetByName", mock.Anything).Return(&model.System{Name: model.SystemLastAccessiblePostTime, Value: "10"}, nil)
+		mockSystemStore.On("PermanentDeleteByName", mock.Anything).Return(nil, nil)
 		mockStore.On("System").Return(&mockSystemStore)
 
 		err := th.App.ComputeLastAccessiblePostTime()
 		assert.NoError(t, err)
 
 		mockSystemStore.AssertNotCalled(t, "SaveOrUpdate", mock.Anything)
+		mockSystemStore.AssertCalled(t, "PermanentDeleteByName", mock.Anything)
 	})
 }