Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Enum.ConvertFromModel should use Basetype.ConvertFromModel method #2224

Merged
merged 14 commits into from
Oct 9, 2018
Merged

Enum.ConvertFromModel should use Basetype.ConvertFromModel method #2224

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
733fd02
Enum.ConvertFromModel should use Basetype.ConvertFromModel method
jarifibrahim Aug 8, 2018
3699075
Merge branch 'master' into enum-type-fix
kwk Aug 8, 2018
da8831c
Merge branch 'master' into enum-type-fix
jarifibrahim Sep 24, 2018
92170e5
fixup ConvertFromModel function
jarifibrahim Sep 24, 2018
414395f
Add test for enumType.ConvertFromModel
jarifibrahim Sep 24, 2018
5d2b6b4
Merge branch 'enum-type-fix' of https://github.com/jarifibrahim/fabri…
jarifibrahim Sep 24, 2018
89e7036
fix failing tests
jarifibrahim Sep 24, 2018
e5965f9
fixup
jarifibrahim Sep 25, 2018
ec7ace0
Minor changes
jarifibrahim Sep 25, 2018
427afb0
Merge branch 'master' into enum-type-fix
jarifibrahim Oct 1, 2018
5512f13
Use errs.Errorf instead of fmt.Errorf
jarifibrahim Oct 8, 2018
377c80e
Merge branch 'enum-type-fix' of https://github.com/jarifibrahim/fabri…
jarifibrahim Oct 8, 2018
575bc70
Merge branch 'master' into enum-type-fix
jarifibrahim Oct 8, 2018
f47eed7
Merge branch 'master' into enum-type-fix
jarifibrahim Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion workitem/enum_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,17 @@ func containsAll(a []interface{}, v []interface{}) bool {
return result
}

// ConvertFromModel implements the FieldType interface
func (t EnumType) ConvertFromModel(value interface{}) (interface{}, error) {
converted, err := t.BaseType.ConvertToModel(value)
if value == nil {
return nil, nil
}
converted, err := t.BaseType.ConvertFromModel(value)
if err != nil {
return nil, fmt.Errorf("error converting enum value: %s", err.Error())
}
if !contains(t.Values, converted) {
return nil, fmt.Errorf("value: %+v (%[1]T) is not part of allowed enum values: %+v", value, t.Values)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jarifibrahim please return a traceable errs.Errorf error instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwk I've changed it in 5512f13

But could you please explain why should we use errs.Errorf instead of fmt.Errorf? Is it because errs.Errorf has a stacktrace?

}
return converted, nil
}
83 changes: 83 additions & 0 deletions workitem/enum_type_blackbox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,86 @@ func TestEnumType_EqualEnclosing(t *testing.T) {
})
})
}

func TestEnumType_ConvertFromModel(t *testing.T) {
t.Parallel()
resource.Require(t, resource.UnitTest)
type testCase struct {
subTestName string
input interface{} // contains valid and invalid values
expectedOutput interface{}
wantErr bool
}
tests := []struct {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum type created for the test uses only string, int and float as the base type. I have not added area/iteration/user for base type because it would not add much value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would add much value but currently the type-system treats all of them as strings which is simple wrong. But this is subject of another PR.

name string
enum w.EnumType
data []testCase
}{
{
"kind string",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need similar test data for ConvertToModel method but I am not sure how we could separate data from the test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be part of first cleaning up the typesystem. The test data needs to know no only input and output but also how a value is stored inside:

JSON -> Go -> DB -> Go -JSON

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using an enum of type string, why did you call this test "kind string" then? I suggest to have more tests for list, enums and simple types and test every one of them individually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a test for enum of type string. I can rename Kind String to type string. Would that be enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I didn't see the context for this subtest.

w.EnumType{
SimpleType: w.SimpleType{Kind: w.KindEnum},
BaseType: w.SimpleType{Kind: w.KindString},
Values: []interface{}{"first", "second", "third"},
},
[]testCase{
{"ok", "second", "second", false},
{"ok - nil", nil, nil, false},
{"fail - invalid string", "fourth", nil, true},
{"fail - int", 11, nil, true},
{"fail - float", 1.3, nil, true},
{"fail - empty string", "", nil, true},
{"fail - list", []string{"x", "y"}, nil, true},
},
},
{
"kind int",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why do you use an enum of ints and call the tests "kind int"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #2224 (comment)

w.EnumType{
SimpleType: w.SimpleType{Kind: w.KindEnum},
BaseType: w.SimpleType{Kind: w.KindInteger},
Values: []interface{}{4, 5, 6},
},
[]testCase{
{"ok", 4, 4, false},
{"ok - nil", nil, nil, false},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important. The method does not return error on nil value.

{"fail - invalid int", 2, nil, true},
{"fail - string", "11", nil, true},
{"fail - float", 1.3, nil, true},
{"fail - bool", true, nil, true},
{"fail - list", []string{"x", "y"}, nil, true},
},
},
{
"kind float",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #2224 (comment)

w.EnumType{
SimpleType: w.SimpleType{Kind: w.KindEnum},
BaseType: w.SimpleType{Kind: w.KindFloat},
Values: []interface{}{1.1, 2.2, 3.3},
},
[]testCase{
{"ok", 1.1, 1.1, false},
{"ok - nil", nil, nil, false},
{"fail - invalid float", 4.4, nil, true},
{"fail - int", 1, nil, true},
{"fail - string", "11", nil, true},
{"fail - bool", true, nil, true},
{"fail - list", []string{"x", "y"}, nil, true},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
for _, subtt := range test.data {
t.Run(subtt.subTestName, func(tt *testing.T) {
val, err := test.enum.ConvertFromModel(subtt.input)
if subtt.wantErr {
require.Error(tt, err)
} else {
require.NoError(tt, err)
}
require.Equal(tt, subtt.expectedOutput, val)
})
}
})
}
}
2 changes: 1 addition & 1 deletion workitem/simple_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (t SimpleType) ConvertToModel(value interface{}) (interface{}, error) {
}
}

// ConvertFromModel implements the t interface
// ConvertFromModel implements the FieldType interface
func (t SimpleType) ConvertFromModel(value interface{}) (interface{}, error) {
if value == nil {
return nil, nil
Expand Down
4 changes: 2 additions & 2 deletions workitem/workitem_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up
}
fieldValue := updatedWorkItem.Fields[fieldName]
var err error
if fieldName == SystemAssignees || fieldName == SystemLabels || fieldName == SystemBoardcolumns {
if fieldDef.Type.GetKind() == KindList {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change wasn't necessary for this PR but since this was a minor change I did it in this PR.

switch fieldValue.(type) {
case []string:
if len(fieldValue.([]string)) == 0 {
Expand Down Expand Up @@ -729,7 +729,7 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID,
if err != nil {
return nil, nil, errors.NewBadParameterError(fieldName, fieldValue) // TODO(kwk): Change errors pkg to consume the original error as well
}
if (fieldName == SystemAssignees || fieldName == SystemLabels || fieldName == SystemBoardcolumns) && fieldValue == nil {
if fieldDef.Type.GetKind() == KindList && fieldValue == nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/fabric8-services/fabric8-wit/pull/2224/files#r220177847

delete(wi.Fields, fieldName)
}
if fieldName == SystemDescription && wi.Fields[fieldName] != nil {
Expand Down