NOISSUE - Fix ViewGroup and UpdateGroup (#1269)

* NOISSUE - Fix ViewGroup and UpdateGroup Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Add ID check and fix naming Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix id in groupUpdateReq Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix review Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Add tests Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
absmach · Oct 27, 2020 · 926e979 · 926e979
1 parent 1c298d8
commit 926e979
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 28 deletions.
diff --git a/users/api/endpoint.go b/users/api/endpoint.go
@@ -259,23 +259,21 @@ func updateGroupEndpoint(svc users.Service) endpoint.Endpoint {
 	return func(ctx context.Context, request interface{}) (interface{}, error) {
 		req := request.(updateGroupReq)
 		if err := req.validate(); err != nil {
-			return createGroupRes{}, err
+			return updateGroupRes{}, err
 		}
+
 		group := users.Group{
+			ID:          req.id,
 			Name:        req.Name,
 			Description: req.Description,
 			Metadata:    req.Metadata,
 		}
+
 		if err := svc.UpdateGroup(ctx, req.token, group); err != nil {
-			return createGroupRes{}, err
+			return updateGroupRes{}, err
 		}
-		res := createGroupRes{
-			Name:        group.Name,
-			Description: group.Description,
-			Metadata:    group.Metadata,
-			created:     false,
-		}
-		return res, nil
+
+		return updateGroupRes{}, nil
 	}
 }
 
@@ -290,7 +288,10 @@ func viewGroupEndpoint(svc users.Service) endpoint.Endpoint {
 			return viewGroupRes{}, err
 		}
 		res := viewGroupRes{
+			ID:          group.ID,
 			Name:        group.Name,
+			ParentID:    group.ParentID,
+			OwnerID:     group.OwnerID,
 			Description: group.Description,
 			Metadata:    group.Metadata,
 		}

diff --git a/users/api/endpoint_test.go b/users/api/endpoint_test.go
@@ -475,7 +475,7 @@ func TestGroupCreate(t *testing.T) {
 		{"group create with existing name", createValidTokenRequest, contentType, http.StatusConflict, groupExists, token},
 		{"group create with invalid token", createInvalidTokenRequest, contentType, http.StatusForbidden, unauthRes, ""},
 		{"group create with empty JSON request", "{}", contentType, http.StatusBadRequest, malformedRes, token},
-		{"group create empty request", "", contentType, http.StatusBadRequest, failDecodeRes, token},
+		{"group create empty request", "", contentType, http.StatusBadRequest, malformedRes, token},
 		{"group create missing content type", createValidTokenRequest, "", http.StatusUnsupportedMediaType, unsupportedRes, token},
 	}
 

diff --git a/users/api/requests.go b/users/api/requests.go
@@ -129,7 +129,9 @@ func (req createGroupReq) validate() error {
 
 type updateGroupReq struct {
 	token       string
+	id          string
 	Name        string                 `json:"name,omitempty"`
+	ParentID    string                 `json:"parent_id,omitempty"`
 	Description string                 `json:"description,omitempty"`
 	Metadata    map[string]interface{} `json:"metadata,omitempty"`
 }
@@ -138,12 +140,13 @@ func (req updateGroupReq) validate() error {
 	if req.token == "" {
 		return users.ErrUnauthorizedAccess
 	}
-	if req.Name == "" {
+	if req.id == "" {
 		return users.ErrMalformedEntity
 	}
-	if len(req.Name) > maxNameSize {
+	if req.Name == "" || len(req.Name) > maxNameSize {
 		return users.ErrMalformedEntity
 	}
+
 	return nil
 }
 

diff --git a/users/api/responses.go b/users/api/responses.go
@@ -6,9 +6,9 @@ package api
 import (
 	"fmt"
 	"net/http"
-	"os/user"
 
 	"github.com/mainflux/mainflux"
+	"github.com/mainflux/mainflux/users"
 )
 
 var (
@@ -93,7 +93,7 @@ func (res updateUserRes) Empty() bool {
 type viewUserRes struct {
 	ID       string                 `json:"id"`
 	Email    string                 `json:"email"`
-	Groups   []user.Group           `json:"groups"`
+	Groups   []users.Group          `json:"groups"`
 	Metadata map[string]interface{} `json:"metadata,omitempty"`
 }
 
@@ -213,7 +213,7 @@ func (res passwChangeRes) Empty() bool {
 
 type groupPageRes struct {
 	pageRes
-	Groups []viewGroupRes `json:"groups"`
+	Groups []viewGroupRes
 }
 
 func (res groupPageRes) Code() int {

diff --git a/users/api/transport.go b/users/api/transport.go
@@ -160,7 +160,7 @@ func MakeHandler(svc users.Service, tracer opentracing.Tracer) http.Handler {
 
 	mux.Patch("/groups/:groupID", kithttp.NewServer(
 		kitot.TraceServer(tracer, "update_group")(updateGroupEndpoint(svc)),
-		decodeGroupCreate,
+		decodeGroupUpdate,
 		encodeResponse,
 		opts...,
 	))
@@ -312,19 +312,31 @@ func decodeGroupCreate(_ context.Context, r *http.Request) (interface{}, error)
 
 	var req createGroupReq
 	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
-		return nil, errors.Wrap(ErrFailedDecode, err)
+		return nil, errors.Wrap(users.ErrMalformedEntity, err)
 	}
 
 	req.token = r.Header.Get("Authorization")
 
 	return req, nil
 }
 
-func decodeGroupRequest(_ context.Context, r *http.Request) (interface{}, error) {
+func decodeGroupUpdate(_ context.Context, r *http.Request) (interface{}, error) {
 	if !strings.Contains(r.Header.Get("Content-Type"), contentType) {
 		return nil, ErrUnsupportedContentType
 	}
 
+	req := updateGroupReq{
+		token: r.Header.Get("Authorization"),
+		id:    bone.GetValue(r, "groupID"),
+	}
+	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+		return nil, errors.Wrap(users.ErrMalformedEntity, err)
+	}
+
+	return req, nil
+}
+
+func decodeGroupRequest(_ context.Context, r *http.Request) (interface{}, error) {
 	req := groupReq{
 		token:   r.Header.Get("Authorization"),
 		groupID: bone.GetValue(r, "groupID"),
@@ -371,10 +383,6 @@ func decodeListUserGroupsRequest(_ context.Context, r *http.Request) (interface{
 }
 
 func decodeUserGroupRequest(_ context.Context, r *http.Request) (interface{}, error) {
-	if !strings.Contains(r.Header.Get("Content-Type"), contentType) {
-		return nil, ErrUnsupportedContentType
-	}
-
 	req := userGroupReq{
 		token:   r.Header.Get("Authorization"),
 		groupID: bone.GetValue(r, "groupID"),

diff --git a/users/postgres/groups.go b/users/postgres/groups.go
@@ -75,15 +75,14 @@ func (gr groupRepository) Save(ctx context.Context, group users.Group) (users.Gr
 }
 
 func (gr groupRepository) Update(ctx context.Context, group users.Group) error {
-	q := `UPDATE groups SET(name, description, metadata) VALUES (:name, :description, :metadata) WHERE id = :id`
-
+	q := `UPDATE groups SET name = :name, metadata = :metadata, description = :description WHERE id = :id;`
 	dbu, err := toDBGroup(group)
 	if err != nil {
-		return errors.Wrap(errUpdateDB, err)
+		return errors.Wrap(users.ErrUpdateGroup, err)
 	}
 
 	if _, err := gr.db.NamedExecContext(ctx, q, dbu); err != nil {
-		return errors.Wrap(errUpdateDB, err)
+		return errors.Wrap(users.ErrUpdateGroup, err)
 	}
 
 	return nil

diff --git a/users/postgres/groups_test.go b/users/postgres/groups_test.go
@@ -6,6 +6,7 @@ package postgres_test
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/mainflux/mainflux/pkg/errors"
@@ -18,10 +19,15 @@ import (
 )
 
 const (
-	groupName = "Mainflux"
-	password  = "12345678"
+	maxNameSize = 254
+	maxDescSize = 1024
+	groupName   = "Mainflux"
+	password    = "12345678"
 )
 
+var invalidName = strings.Repeat("m", maxNameSize+1)
+var invalidDesc = strings.Repeat("m", maxDescSize+1)
+
 func TestGroupSave(t *testing.T) {
 	dbMiddleware := postgres.NewDatabase(db)
 	repo := postgres.NewGroupRepo(dbMiddleware)
@@ -142,6 +148,65 @@ func TestGroupRetrieveByID(t *testing.T) {
 	}
 }
 
+func TestGroupUpdate(t *testing.T) {
+	dbMiddleware := postgres.NewDatabase(db)
+	groupRepo := postgres.NewGroupRepo(dbMiddleware)
+
+	gid, err := uuid.New().ID()
+	require.Nil(t, err, fmt.Sprintf("group id unexpected error: %s", err))
+	group := users.Group{
+		ID:   gid,
+		Name: groupName,
+	}
+
+	_, err = groupRepo.Save(context.Background(), group)
+	require.Nil(t, err, fmt.Sprintf("group save got unexpected error: %s", err))
+
+	cases := []struct {
+		desc  string
+		group users.Group
+		err   error
+	}{
+		{
+			desc: "update group for existing id",
+			group: users.Group{
+				ID:   gid,
+				Name: groupName + "-1",
+			},
+			err: nil,
+		},
+		{
+			desc: "update group for non-existing id",
+			group: users.Group{
+				ID:   "wrong",
+				Name: groupName + "-2",
+			},
+			err: users.ErrUpdateGroup,
+		},
+		{
+			desc: "update group for invalid name",
+			group: users.Group{
+				ID:   gid,
+				Name: invalidName,
+			},
+			err: users.ErrUpdateGroup,
+		},
+		{
+			desc: "update group for invalid description",
+			group: users.Group{
+				ID:          gid,
+				Description: invalidDesc,
+			},
+			err: users.ErrUpdateGroup,
+		},
+	}
+
+	for _, tc := range cases {
+		err := groupRepo.Update(context.Background(), tc.group)
+		assert.True(t, errors.Contains(err, tc.err), fmt.Sprintf("%s: expected %s got %s\n", tc.desc, tc.err, err))
+	}
+}
+
 func TestGroupDelete(t *testing.T) {
 	dbMiddleware := postgres.NewDatabase(db)
 	repo := postgres.NewGroupRepo(dbMiddleware)

diff --git a/users/service.go b/users/service.go
@@ -59,6 +59,9 @@ var (
 	// ErrCreateGroup indicates error in creating group.
 	ErrCreateGroup = errors.New("failed to create group")
 
+	// ErrUpdateGroup indicates error in updating group.
+	ErrUpdateGroup = errors.New("failed to update group")
+
 	// ErrDeleteGroupMissing indicates in delete operation that group doesnt exist.
 	ErrDeleteGroupMissing = errors.New("group is not existing, already deleted")