Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NOISSUE - Fix ViewGroup and UpdateGroup #1269

Merged
merged 5 commits into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
19 changes: 10 additions & 9 deletions users/api/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

mising ID

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
}
}

Expand All @@ -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,
}
Expand Down
2 changes: 1 addition & 1 deletion users/api/endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}

Expand Down
6 changes: 6 additions & 0 deletions users/api/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

we need parent id here also

Metadata map[string]interface{} `json:"metadata,omitempty"`
}
Expand All @@ -138,12 +140,16 @@ func (req updateGroupReq) validate() error {
if req.token == "" {
return users.ErrUnauthorizedAccess
}
if req.id == "" {
return users.ErrMalformedEntity
}
if req.Name == "" {
return users.ErrMalformedEntity
}
if len(req.Name) > maxNameSize {
return users.ErrMalformedEntity
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can or these conditions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that you can or all of these checks: if id == "" || name == "" || len(name) > maxNameLen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agreed to group this checks by type but for consistency with other services and readability I would keep it like that.
You can check here: https://github.com/mainflux/mainflux/blob/master/things/api/things/http/requests.go


return nil
}

Expand Down
6 changes: 3 additions & 3 deletions users/api/responses.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ package api
import (
"fmt"
"net/http"
"os/user"

"github.com/mainflux/mainflux"
"github.com/mainflux/mainflux/users"
)

var (
Expand Down Expand Up @@ -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"`
}

Expand Down Expand Up @@ -213,7 +213,7 @@ func (res passwChangeRes) Empty() bool {

type groupPageRes struct {
pageRes
Groups []viewGroupRes `json:"groups"`
Groups []viewGroupRes
}

func (res groupPageRes) Code() int {
Expand Down
22 changes: 15 additions & 7 deletions users/api/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...,
))
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down
3 changes: 1 addition & 2 deletions users/postgres/groups.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ 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)
Expand Down
36 changes: 36 additions & 0 deletions users/postgres/groups_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,42 @@ 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,
}
groupUp := users.Group{
ID: gid,
Name: groupName + "-TestGroupUpdate",
}

_, 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: groupUp,
err: nil,
},
}

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)
Expand Down