-
Notifications
You must be signed in to change notification settings - Fork 663
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
MG-2216 - Rename delete policy function #2218
base: main
Are you sure you want to change the base?
MG-2216 - Rename delete policy function #2218
Conversation
2848916
to
158f23d
Compare
72da806
to
7bc5809
Compare
7bc5809
to
edf248a
Compare
@@ -1394,7 +1394,7 @@ func TestDeletePolicies(t *testing.T) { | |||
} | |||
|
|||
for _, tc := range cases { | |||
repocall := prepo.On("DeletePolicies", mock.Anything, mock.Anything).Return(tc.err) | |||
repocall := prepo.On("DeletePolicies", context.Background(), mock.Anything).Return(tc.err) | |||
err := svc.DeletePolicies(context.Background(), tc.pr) |
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.
@nyagamunene ,Why we are changing from mock.Anything
to context.Background()
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.
When calling page, err := svc.ListAllObjects(context.Background(), tc.pr)
context.Background is passed to the repo call. This is a mistake that was not noted during the mock test
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.
if we have mock.Anything
does the test fails
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.
No they don't fail
auth/api/grpc/endpoint_test.go
Outdated
deletePolicyReq *magistrala.DeletePolicyReq | ||
deletePolicyRes *magistrala.DeletePolicyRes | ||
deletePolicyReq *magistrala.DeletePolicyFilterReq | ||
deletePolicyRes *magistrala.DeletePolicyFilterRes | ||
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.
err error | |
deletePolicyFilterReq *magistrala.DeletePolicyFilterReq | |
deletePolicyFilterRes *magistrala.DeletePolicyFilterRes |
users/service_test.go
Outdated
@@ -1050,7 +1050,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
desc: "update client role to user role with failed to delete policy", | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
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.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
users/service_test.go
Outdated
@@ -1059,7 +1059,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
desc: "update client role to user role with failed to delete policy with error", | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
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.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
users/service_test.go
Outdated
@@ -1071,7 +1071,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
addPolicyResponse: &magistrala.AddPolicyRes{Added: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: true}, | |||
updateRoleResponse: mgclients.Client{}, |
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.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: true}, |
users/service_test.go
Outdated
@@ -1083,7 +1083,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
addPolicyResponse: &magistrala.AddPolicyRes{Added: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
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.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
edf248a
to
271320b
Compare
f0e8c67
to
39482cf
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.
LGTM
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.
@nyagamunene I will approve this, however upon testing I found out that UnshareThing
does not check whether the provided userID is actually a user in the platform. Additionally, unshare thing with a correct thing ID and a random user ID passes successfully, and it can be done multiple times successfully.
Please test this on your end and create an issue for it so that we do not forget it.
c8962d2
2f7c8cd
to
0ca9479
Compare
0ca9479
to
ab2eccc
Compare
auth/api/grpc/endpoint_test.go
Outdated
repoCall := svc.On("DeletePolicy", mock.Anything, mock.Anything).Return(tc.err) | ||
dpr, err := client.DeletePolicy(context.Background(), tc.deletePolicyReq) | ||
assert.Equal(t, tc.deletePolicyRes.GetDeleted(), dpr.GetDeleted(), fmt.Sprintf("%s: expected %v got %v", tc.desc, tc.deletePolicyRes.GetDeleted(), dpr.GetDeleted())) | ||
repoCall := svc.On("DeletePolicyFilter", mock.Anything, mock.Anything).Return(tc.err) |
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.
repoCall := svc.On("DeletePolicyFilter", mock.Anything, mock.Anything).Return(tc.err) | |
svcCall := svc.On("DeletePolicyFilter", mock.Anything, mock.Anything).Return(tc.err) |
@@ -1448,7 +1448,7 @@ func TestListObjects(t *testing.T) { | |||
}, | |||
} | |||
for _, tc := range cases { | |||
repocall2 := prepo.On("RetrieveObjects", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) | |||
repocall2 := prepo.On("RetrieveObjects", context.Background(), mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) |
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.
repocall2 := prepo.On("RetrieveObjects", context.Background(), mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) | |
repocall := prepo.On("RetrieveObjects", context.Background(), mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) |
@@ -1516,7 +1516,7 @@ func TestCountObjects(t *testing.T) { | |||
|
|||
pageLen := uint64(15) | |||
|
|||
repocall2 := prepo.On("RetrieveAllObjectsCount", mock.Anything, mock.Anything, mock.Anything).Return(pageLen, nil) | |||
repocall2 := prepo.On("RetrieveAllObjectsCount", context.Background(), mock.Anything, mock.Anything).Return(pageLen, nil) |
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.
repocall2 := prepo.On("RetrieveAllObjectsCount", context.Background(), mock.Anything, mock.Anything).Return(pageLen, nil) | |
repocall := prepo.On("RetrieveAllObjectsCount", context.Background(), mock.Anything, mock.Anything).Return(pageLen, nil) |
auth/service_test.go
Outdated
@@ -1566,7 +1566,7 @@ func TestListSubjects(t *testing.T) { | |||
}, | |||
} | |||
for _, tc := range cases { | |||
repocall2 := prepo.On("RetrieveSubjects", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) | |||
repocall2 := prepo.On("RetrieveSubjects", context.Background(), mock.Anything, mock.Anything, mock.Anything).Return(expectedPolicies, mock.Anything, tc.err) |
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.
same
auth/service_test.go
Outdated
@@ -1619,7 +1619,7 @@ func TestListAllSubjects(t *testing.T) { | |||
}, | |||
} | |||
for _, tc := range cases { | |||
repocall2 := prepo.On("RetrieveAllSubjects", mock.Anything, mock.Anything).Return(expectedPolicies, tc.err) | |||
repocall2 := prepo.On("RetrieveAllSubjects", context.Background(), mock.Anything).Return(expectedPolicies, tc.err) |
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.
same
auth/service_test.go
Outdated
@@ -1653,7 +1653,7 @@ func TestListPermissions(t *testing.T) { | |||
} | |||
filterPermisions := []string{auth.ViewPermission, auth.AdminPermission} | |||
|
|||
repoCall1 := prepo.On("RetrievePermissions", mock.Anything, pr, filterPermisions).Return(auth.Permissions{}, nil) | |||
repoCall1 := prepo.On("RetrievePermissions", context.Background(), pr, filterPermisions).Return(auth.Permissions{}, nil) |
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.
repoCall1 := prepo.On("RetrievePermissions", context.Background(), pr, filterPermisions).Return(auth.Permissions{}, nil) | |
repoCall := prepo.On("RetrievePermissions", context.Background(), pr, filterPermisions).Return(auth.Permissions{}, nil) |
authzResp: &magistrala.AuthorizeRes{ | ||
Authorized: true, | ||
}, | ||
deleteChildPoliciesRes: &magistrala.DeletePolicyRes{ | ||
deleteChildPoliciesRes: &magistrala.DeletePolicyFilterRes{ | ||
Deleted: true, | ||
}, | ||
deleteThingsPoliciesRes: &magistrala.DeletePolicyRes{ | ||
deleteThingsPoliciesRes: &magistrala.DeletePolicyFilterRes{ | ||
Deleted: true, | ||
}, | ||
deleteDomainsPoliciesRes: &magistrala.DeletePolicyRes{ | ||
deleteDomainsPoliciesRes: &magistrala.DeletePolicyFilterRes{ | ||
Deleted: true, | ||
}, | ||
deleteUsersPoliciesRes: &magistrala.DeletePolicyRes{ | ||
deleteUsersPoliciesRes: &magistrala.DeletePolicyFilterRes{ | ||
Deleted: true, | ||
}, |
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.
authzResp: &magistrala.AuthorizeRes{ | |
Authorized: true, | |
}, | |
deleteChildPoliciesRes: &magistrala.DeletePolicyRes{ | |
deleteChildPoliciesRes: &magistrala.DeletePolicyFilterRes{ | |
Deleted: true, | |
}, | |
deleteThingsPoliciesRes: &magistrala.DeletePolicyRes{ | |
deleteThingsPoliciesRes: &magistrala.DeletePolicyFilterRes{ | |
Deleted: true, | |
}, | |
deleteDomainsPoliciesRes: &magistrala.DeletePolicyRes{ | |
deleteDomainsPoliciesRes: &magistrala.DeletePolicyFilterRes{ | |
Deleted: true, | |
}, | |
deleteUsersPoliciesRes: &magistrala.DeletePolicyRes{ | |
deleteUsersPoliciesRes: &magistrala.DeletePolicyFilterRes{ | |
Deleted: true, | |
}, | |
authzResp: &magistrala.AuthorizeRes{Authorized: true}, | |
deleteChildPoliciesRes: &magistrala.DeletePolicyFilterRes{Deleted: true}, | |
deleteThingsPoliciesRes: &magistrala.DeletePolicyFilterRes{Deleted: true}, | |
deleteDomainsPoliciesRes: &magistrala.DeletePolicyFilterRes{Deleted: true}, | |
deleteUsersPoliciesRes: &magistrala.DeletePolicyFilterRes{Deleted: true}, |
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.
same as all other cases
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 think the format we currently have is much better.
internal/groups/service_test.go
Outdated
@@ -2626,23 +2626,23 @@ func TestDeleteGroup(t *testing.T) { | |||
Object: tc.groupID, | |||
ObjectType: auth.GroupType, | |||
}).Return(tc.authzResp, tc.authzErr) | |||
repocall2 := authsvc.On("DeletePolicy", context.Background(), &magistrala.DeletePolicyReq{ | |||
repocall2 := authsvc.On("DeletePolicyFilter", context.Background(), &magistrala.DeletePolicyFilterReq{ |
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.
repocall2 := authsvc.On("DeletePolicyFilter", context.Background(), &magistrala.DeletePolicyFilterReq{ | |
authCallll2 := authsvc.On("DeletePolicyFilter", context.Background(), &magistrala.DeletePolicyFilterReq{ |
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.
same as other cases
pkg/sdk/go/channels_test.go
Outdated
@@ -769,7 +769,7 @@ func TestDeleteChannel(t *testing.T) { | |||
repoCall1.Unset() | |||
repoCall2.Unset() | |||
|
|||
repoCall = auth.On("DeletePolicy", mock.Anything, mock.Anything, mock.Anything).Return(&magistrala.DeletePolicyRes{Deleted: true}, nil) | |||
repoCall = auth.On("DeletePolicyFilter", mock.Anything, mock.Anything, mock.Anything).Return(&magistrala.DeletePolicyFilterRes{Deleted: true}, nil) |
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.
same
pkg/sdk/go/groups_test.go
Outdated
@@ -909,7 +909,7 @@ func TestDeleteGroup(t *testing.T) { | |||
repoCall1.Unset() | |||
repoCall2.Unset() | |||
|
|||
repoCall = auth.On("DeletePolicy", mock.Anything, mock.Anything, mock.Anything).Return(&magistrala.DeletePolicyRes{Deleted: true}, nil) | |||
repoCall = auth.On("DeletePolicyFilter", mock.Anything, mock.Anything, mock.Anything).Return(&magistrala.DeletePolicyFilterRes{Deleted: true}, nil) |
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.
same
ab2eccc
to
0808a0b
Compare
0808a0b
to
f2d1aa1
Compare
eac3733
to
820a077
Compare
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
820a077
to
b8d562d
Compare
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>
What type of PR is this?
This is a feature that renames the
DeletePolicy
gRPC function toDeletePolicyFilter
policy with relationship filter.What does this do?
It rename gRPC, gRPC request & SpiceDB function
DeletePolicy
toDeletePolicyFilter
Which issue(s) does this PR fix/relate to?
DeletePolicy
gRPC & SpiceDB function toDeletePolicyFilter
and #2216DeletePolicy
gRPC & SpiceDB function toDeletePolicyFilter
and #2216Have you included tests for your changes?
Yes.
Did you document any new/modified feature?
No.
Notes