Skip to content

Commit db2239b

Browse files
committed
Validate work package updates up front
Resolve actions, patch values, and attachment paths before the first mutation. This prevents local validation errors from leaving earlier changes applied while preserving lock-version refreshes. Attachment validation also requires the addAttachment link and rejects fog storages before any mutation, so a missing upload capability surfaces as an error instead of a panic after the action and patch have already run.
1 parent 0056b19 commit db2239b

6 files changed

Lines changed: 522 additions & 22 deletions

File tree

cmd/workpackage/options_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
package workpackage
22

33
import (
4+
"errors"
5+
"strings"
46
"testing"
57

68
"github.com/spf13/cobra"
79

10+
openerrors "github.com/opf/openproject-cli/components/errors"
11+
"github.com/opf/openproject-cli/components/printer"
812
"github.com/opf/openproject-cli/components/resources/work_packages"
913
)
1014

@@ -65,3 +69,31 @@ func TestUpdateOptions_DescriptionProvided(t *testing.T) {
6569
t.Error("expected UpdateDescription to be present when flag explicitly provided with empty string")
6670
}
6771
}
72+
73+
func TestUpdateWithoutOptionsStopsBeforeRequest(t *testing.T) {
74+
updateActionFlag = ""
75+
updateAssigneeFlag = 0
76+
updateAttachFlag = ""
77+
updateDescriptionFlag = ""
78+
updateSubjectFlag = ""
79+
updateTypeFlag = ""
80+
81+
cmd := newCmdWithDescriptionFlag(&updateDescriptionFlag)
82+
if err := cmd.Flags().Parse(nil); err != nil {
83+
t.Fatal(err)
84+
}
85+
86+
testingPrinter := &printer.TestingPrinter{}
87+
printer.Init(testingPrinter)
88+
89+
err := updateWorkPackage(cmd, []string{"42"})
90+
if !errors.Is(err, openerrors.ErrHandled) {
91+
t.Fatalf("updateWorkPackage error = %v, want ErrHandled", err)
92+
}
93+
if !strings.Contains(testingPrinter.ErrResult, "No update options provided") {
94+
t.Errorf("stderr = %q, want no-options diagnostic", testingPrinter.ErrResult)
95+
}
96+
if strings.Contains(testingPrinter.ErrResult, "Cannot execute requests") {
97+
t.Errorf("command attempted a request: %q", testingPrinter.ErrResult)
98+
}
99+
}

cmd/workpackage/update.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package workpackage
22

33
import (
4+
stderrors "errors"
45
"fmt"
56
"strconv"
67

@@ -38,9 +39,17 @@ func updateWorkPackage(cmd *cobra.Command, args []string) error {
3839
return openerrors.ErrHandled
3940
}
4041

41-
workPackage, err := work_packages.Update(id, updateOptions(cmd))
42+
options := updateOptions(cmd)
43+
if len(options) == 0 {
44+
printer.ErrorText("No update options provided. Use --help to see available flags.")
45+
return openerrors.ErrHandled
46+
}
47+
48+
workPackage, err := work_packages.Update(id, options)
4249
if err != nil {
43-
printer.Error(err)
50+
if !stderrors.Is(err, openerrors.ErrHandled) {
51+
printer.Error(err)
52+
}
4453
return openerrors.ErrHandled
4554
}
4655

components/resources/work_packages/custom_actions.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,15 @@ import (
77
"strings"
88

99
"github.com/opf/openproject-cli/components/common"
10+
openerrors "github.com/opf/openproject-cli/components/errors"
1011
"github.com/opf/openproject-cli/components/parser"
1112
"github.com/opf/openproject-cli/components/printer"
1213
"github.com/opf/openproject-cli/components/requests"
1314
"github.com/opf/openproject-cli/dtos"
1415
"github.com/opf/openproject-cli/models"
1516
)
1617

17-
func action(workPackage *dtos.WorkPackageDto, action string) error {
18+
func resolveAction(workPackage *dtos.WorkPackageDto, action string) (*dtos.CustomActionDto, error) {
1819
foundAction := findAction(action, workPackage.Embedded.CustomActions)
1920
if foundAction == nil {
2021
printer.ErrorText("Failed to execute work package custom action.")
@@ -31,10 +32,10 @@ func action(workPackage *dtos.WorkPackageDto, action string) error {
3132
[]*models.CustomAction{},
3233
)
3334
printer.CustomActions(availableActions)
34-
return nil
35+
return nil, openerrors.ErrHandled
3536
}
3637

37-
return executeAction(workPackage, foundAction)
38+
return foundAction, nil
3839
}
3940

4041
func findAction(actionInput string, availableActions []*dtos.CustomActionDto) *dtos.CustomActionDto {

components/resources/work_packages/update.go

Lines changed: 60 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import (
44
"bytes"
55
"encoding/json"
66
"fmt"
7+
"os"
78
"strconv"
89

910
"github.com/opf/openproject-cli/components/common"
11+
openerrors "github.com/opf/openproject-cli/components/errors"
1012
"github.com/opf/openproject-cli/components/parser"
1113
"github.com/opf/openproject-cli/components/paths"
1214
"github.com/opf/openproject-cli/components/printer"
@@ -41,8 +43,27 @@ func Update(id string, options map[UpdateOption]string) (*models.WorkPackage, er
4143
return nil, err
4244
}
4345

44-
if customAction, ok := options[UpdateCustomAction]; ok {
45-
err = action(workPackage, customAction)
46+
var customAction *dtos.CustomActionDto
47+
if input, ok := options[UpdateCustomAction]; ok {
48+
customAction, err = resolveAction(workPackage, input)
49+
if err != nil {
50+
return nil, err
51+
}
52+
}
53+
54+
patchDto, updateString, patchNeeded, err := preparePatch(workPackage, options)
55+
if err != nil {
56+
return nil, err
57+
}
58+
59+
if file, ok := options[UpdateAttachment]; ok {
60+
if err := validateAttachment(workPackage, file); err != nil {
61+
return nil, err
62+
}
63+
}
64+
65+
if customAction != nil {
66+
err = executeAction(workPackage, customAction)
4667
if err != nil {
4768
return nil, err
4869
}
@@ -54,9 +75,11 @@ func Update(id string, options map[UpdateOption]string) (*models.WorkPackage, er
5475
}
5576
}
5677

57-
err = patch(workPackage, options)
58-
if err != nil {
59-
return nil, err
78+
if patchNeeded {
79+
patchDto.LockVersion = workPackage.LockVersion
80+
if err := executePatch(workPackage, patchDto, updateString); err != nil {
81+
return nil, err
82+
}
6083
}
6184

6285
if file, ok := options[UpdateAttachment]; ok {
@@ -74,9 +97,9 @@ func Update(id string, options map[UpdateOption]string) (*models.WorkPackage, er
7497
return workPackage.Convert(), nil
7598
}
7699

77-
func patch(workPackage *dtos.WorkPackageDto, options map[UpdateOption]string) error {
100+
func preparePatch(workPackage *dtos.WorkPackageDto, options map[UpdateOption]string) (*dtos.WorkPackageDto, string, bool, error) {
78101
var patchNeeded = false
79-
patchDto := dtos.WorkPackageDto{LockVersion: workPackage.LockVersion}
102+
patchDto := &dtos.WorkPackageDto{}
80103
var updateString string
81104

82105
for option, value := range options {
@@ -85,9 +108,9 @@ func patch(workPackage *dtos.WorkPackageDto, options map[UpdateOption]string) er
85108
}
86109

87110
patchNeeded = true
88-
updateStringLine, err := patchMap[option](&patchDto, workPackage, value)
111+
updateStringLine, err := patchMap[option](patchDto, workPackage, value)
89112
if err != nil {
90-
return err
113+
return nil, "", false, err
91114
}
92115

93116
if len(updateStringLine) > 0 {
@@ -98,10 +121,10 @@ func patch(workPackage *dtos.WorkPackageDto, options map[UpdateOption]string) er
98121
}
99122
}
100123

101-
if !patchNeeded {
102-
return nil
103-
}
124+
return patchDto, updateString, patchNeeded, nil
125+
}
104126

127+
func executePatch(workPackage, patchDto *dtos.WorkPackageDto, updateString string) error {
105128
printer.Info("Updating work package with patch ...")
106129

107130
marshal, err := json.Marshal(patchDto)
@@ -119,6 +142,30 @@ func patch(workPackage *dtos.WorkPackageDto, options map[UpdateOption]string) er
119142
return nil
120143
}
121144

145+
func validateAttachment(workPackage *dtos.WorkPackageDto, path string) error {
146+
if workPackage.Links.PrepareAttachment != nil {
147+
return fmt.Errorf("uploads to fog storages are currently not supported")
148+
}
149+
if workPackage.Links.AddAttachment == nil {
150+
return fmt.Errorf("this work package does not accept attachments (missing permission?)")
151+
}
152+
153+
file, err := os.Open(path)
154+
if err != nil {
155+
return err
156+
}
157+
defer func() { _ = file.Close() }()
158+
159+
info, err := file.Stat()
160+
if err != nil {
161+
return err
162+
}
163+
if info.IsDir() {
164+
return fmt.Errorf("attachment path %q is a directory", path)
165+
}
166+
return nil
167+
}
168+
122169
func typePatch(patch, workPackage *dtos.WorkPackageDto, input string) (string, error) {
123170
types, err := availableTypes(workPackage.Links.Project)
124171
if err != nil {
@@ -136,7 +183,7 @@ func typePatch(patch, workPackage *dtos.WorkPackageDto, input string) (string, e
136183

137184
printer.Types(types.Convert())
138185

139-
return "", nil
186+
return "", openerrors.ErrHandled
140187
}
141188

142189
if patch.Links == nil {

0 commit comments

Comments
 (0)