From f3924ca8c2a1c615d6a916006ec861e6d8320c28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Franke?= Date: Fri, 6 Jan 2023 12:58:32 +0100 Subject: [PATCH] Disassociate users from schools on school delete. (#5343) * Disassociate users from schools on school delete. This PR alters the `DeleteEducationSchool` to also disassociate the users that were associated with the to-be-deleted school. * Add changelog. * Remove punctuation from changelog. * Remove redundant return statement. * Skip when user not find. --- ...-disassociate-users-from-deleted-school.md | 6 +++ .../graph/pkg/service/v0/educationschools.go | 37 ++++++++++++++++--- .../pkg/service/v0/educationschools_test.go | 24 +++++++++++- 3 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md diff --git a/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md b/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md new file mode 100644 index 000000000..59cd2f900 --- /dev/null +++ b/changelog/unreleased/bugfix-disassociate-users-from-deleted-school.md @@ -0,0 +1,6 @@ +Bugfix: Disassociate users from deleted school + +When a school is deleted, users should be disassociated from it. + +https://github.com/owncloud/ocis/pull/5343 +https://github.com/owncloud/ocis/issues/5246 diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index 08c51dbca..b3e9fcb20 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -11,6 +11,7 @@ import ( "github.com/CiscoM31/godata" libregraph "github.com/owncloud/libre-graph-api-go" + "github.com/owncloud/ocis/v2/services/graph/pkg/identity" "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode" "github.com/go-chi/chi/v5" @@ -195,17 +196,32 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) { return } + logger.Debug().Str("schoolID", schoolID).Msg("Getting users of school") + users, err := g.identityEducationBackend.GetEducationSchoolUsers(r.Context(), schoolID) + if err != nil { + logger.Debug().Err(err).Msg("could not get school users: backend error") + renderInternalServerError(w, r, err) + return + } + + for _, user := range users { + logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("calling delete member on backend") + if err := g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, *user.Id); err != nil { + if errors.Is(err, identity.ErrNotFound) { + logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("user not found") + continue + } + logger.Debug().Err(err).Msg("could not delete school member: backend error") + renderInternalServerError(w, r, err) + } + } + logger.Debug().Str("id", schoolID).Msg("calling delete school on backend") err = g.identityEducationBackend.DeleteEducationSchool(r.Context(), schoolID) if err != nil { logger.Debug().Err(err).Msg("could not delete school: backend error") - var errcode errorcode.Error - if errors.As(err, &errcode) { - errcode.Render(w, r) - } else { - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) - } + renderInternalServerError(w, r, err) return } @@ -221,6 +237,15 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) { render.NoContent(w, r) } +func renderInternalServerError(w http.ResponseWriter, r *http.Request, err error) { + var errcode errorcode.Error + if errors.As(err, &errcode) { + errcode.Render(w, r) + } else { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } +} + // GetEducationSchoolUsers implements the Service interface. func (g Graph) GetEducationSchoolUsers(w http.ResponseWriter, r *http.Request) { logger := g.logger.SubloggerWithRequestID(r.Context()) diff --git a/services/graph/pkg/service/v0/educationschools_test.go b/services/graph/pkg/service/v0/educationschools_test.go index 52c240477..b94878cc3 100644 --- a/services/graph/pkg/service/v0/educationschools_test.go +++ b/services/graph/pkg/service/v0/educationschools_test.go @@ -50,7 +50,6 @@ var _ = Describe("Schools", func() { ) BeforeEach(func() { - identityEducationBackend = &identitymocks.EducationBackend{} gatewayClient = &mocks.GatewayClient{} newSchool = libregraph.NewEducationSchool() @@ -336,6 +335,7 @@ var _ = Describe("Schools", func() { It("deletes the school", func() { identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{}, nil) r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/education/schools", nil) rctx := chi.NewRouteContext() rctx.URLParams.Add("schoolID", *newSchool.Id) @@ -345,6 +345,28 @@ var _ = Describe("Schools", func() { Expect(rr.Code).To(Equal(http.StatusNoContent)) identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "DeleteEducationSchool", 1) }) + + It("removes the users from the school", func() { + user1 := libregraph.NewEducationUser() + user1.SetId("user1") + user2 := libregraph.NewEducationUser() + user2.SetId("user2") + identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{user1, user2}, nil) + identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user1.Id).Return(nil) + identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user2.Id).Return(nil) + + r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/education/schools", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("schoolID", *newSchool.Id) + r = r.WithContext(context.WithValue(ctxpkg.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) + svc.DeleteEducationSchool(rr, r) + + Expect(rr.Code).To(Equal(http.StatusNoContent)) + identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "DeleteEducationSchool", 1) + identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "RemoveUserFromEducationSchool", 2) + identityEducationBackend.AssertNumberOfCalls(GinkgoT(), "GetEducationSchoolUsers", 1) + }) }) Describe("GetEducationSchoolUsers", func() {