-
Notifications
You must be signed in to change notification settings - Fork 669
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
Conversation
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@@ -129,6 +129,7 @@ func (req createGroupReq) validate() error { | |||
|
|||
type updateGroupReq struct { | |||
token string | |||
id string | |||
Name string `json:"name,omitempty"` | |||
Description string `json:"description,omitempty"` |
There was a problem hiding this comment.
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
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
users/api/requests.go
Outdated
if req.id == "" { | ||
return users.ErrMalformedEntity | ||
} | ||
if req.Name == "" { | ||
return users.ErrMalformedEntity | ||
} | ||
if len(req.Name) > maxNameSize { | ||
return users.ErrMalformedEntity | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
} | ||
|
||
group := users.Group{ | ||
Name: req.Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mising ID
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me, just take into account @dusanb94 's remark and I'll approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com