-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
11f3373
to
64dcdb1
Compare
Looks like #30 changes a lot of APIs so waiting for that to get merged before proceeding with it. |
@dpakach we merged the initial group support. only updating groups is not yet covered. but tha api has settled for now. |
f20d8d2
to
3711137
Compare
9010e62
to
0e6ca8c
Compare
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.
looks pretty good so far 🚀
some lines are pretty long, please make them shorter
please raise issues about the found bugs and mark the tests, so they point to the issues see https://github.com/owncloud/ocis-settings/pull/21/files#diff-477228b861253bdc0920482b86df104fR124
assertResponseContainsUser(t, resp, getAccount("user1")) | ||
assertResponseContainsUser(t, resp, getAccount("user2")) | ||
|
||
// time.Sleep(time.Second * 30) |
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.
debug line?
|
||
// time.Sleep(time.Second * 30) | ||
|
||
fmt.Println("new Users", newCreatedAccounts) |
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.
debug line? can it be deleted?
assert.IsType(t, &proto.Group{}, resp) | ||
assert.Empty(t, resp) | ||
assert.Error(t, err) | ||
assert.Equal(t, "{\"id\":\".\",\"code\":404,\"detail\":\"could not read group: open accounts-store/groups/42: no such file or directory\",\"status\":\"Not Found\"}", err.Error()) |
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.
can we unmarshal this string and compare the JSON object?
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.
The server returns this as a string so I don't think that will be really necessary
func TestDeleteGroup(t *testing.T) { | ||
grp1 := getTestGroups("grp1") | ||
grp2 := getTestGroups("grp2") | ||
// grp3 := getTestGroups("grp3") |
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.
what happened to grp3?
|
||
resp := listGroups(t) | ||
assertResponseContainsGroup(t, resp, updatedGroup) | ||
|
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.
need to check if member was added
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.
the updatedGroups contains the new member too. if the response contains the updated group then I think we can safely assume that them member was added.
// assertGroupsSame(t, grp1, res) | ||
|
||
resp := listGroups(t) | ||
assertResponseContainsGroup(t, resp, grp1) |
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.
need also to check that the member is not in the group anymore
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.
The original grp1 doesn't contain the member, in the beginning. If the response contains grp1, then we can assume that the member was removed from the group successfully.
func assertResponseNotContainsUser(t *testing.T, response *proto.ListAccountsResponse, account *proto.Account) { | ||
for _, a := range response.Accounts { | ||
if a.Id == account.Id || a.PreferredName == account.PreferredName { | ||
t.Fatal("Account not found in response") |
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.
account found or not found?
func assertResponseNotContainsGroup(t *testing.T, response *proto.ListGroupsResponse, group *proto.Group) { | ||
for _, g := range response.Groups { | ||
if g.Id == group.Id && g.DisplayName == group.DisplayName { | ||
t.Fatal("Group not found in response") |
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.
Group found or not found?
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
Add gRPC integration tests for accounts and group related actions for
fixes #29