feat: make task not visible by default #226
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes the default visibility of newly created tasks from true (globally visible) to false (not visible). The change aligns the application behavior with the database model's default value (gorm:"default:false").
Key Changes:
- Updated task creation logic to use the provided
IsVisiblevalue instead of hardcoding it totrue - Added
isVisibleform parameter to the task upload API endpoint (defaults tofalsewhen not provided) - Enhanced test coverage with a dedicated test for default visibility behavior and refactored test setup using a helper function
- Updated Docker Compose configuration to use GitHub Container Registry images
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| package/service/task_service.go | Changed IsVisible assignment from hardcoded true to use the value from the input schema |
| internal/api/http/routes/tasks_management.go | Added parsing of optional isVisible form parameter with default value of false and updated Swagger documentation |
| package/service/task_service_test.go | Refactored test setup with helper function, added tests for default and custom visibility, and updated existing test cases with proper mock controller lifecycle |
| docker-compose.yaml | Removed pull_policy: never from worker service and updated frontend image to use GitHub Container Registry |
| CreatedBy: adminUser.ID, | ||
| IsVisible: true, | ||
| } | ||
| ur.EXPECT().Get(gomock.Any(), gomock.Any()).Return(&models.User{ID: 1, Role: types.UserRoleAdmin}, nil).Times(1) |
There was a problem hiding this comment.
The test name "Custom visibility" is misleading because it tests setting IsVisible to false, which is now the default behavior (same as the "Success" test). This test should either:
- Test setting IsVisible to true (which would be a true custom/non-default value), or
- Be renamed to better reflect what it's actually testing (e.g., "Explicitly set visibility to false")
| return int64(2), nil | ||
| }).Times(1) | ||
| acs.EXPECT().GrantOwnerAccess(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1) | ||
|
|
||
| taskID, err := ts.Create(db, adminUser, task) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(2), taskID) | ||
| }) | ||
|
|
||
| t.Run("Non unique title", func(t *testing.T) { |
There was a problem hiding this comment.
This test should verify that IsVisible is set to true to properly test custom (non-default) visibility. Currently, it's testing false, which is the same as the default behavior tested in the "Success" test case. Change line 163 to IsVisible: true and update line 168 to assert.True(t, model.IsVisible) to ensure this test actually validates custom visibility behavior.
|
@copilot please adjust according to comments if reasonable |
|
@TheRealSeber I've opened a new pull request, #227, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.