Repository: owncloud/ocis
Pull request #5339 (2023-01-05 09:18:15)

GET/DELETE with school id or school number
This implements the GET/DELETE parts of #5245. PATCH requires changes in the EducationSchools service, so that will be in a separate PR.

update-docs[bot]: Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a [changelog](https://github.com/owncloud/ocis/blob/master/changelog/README.md) item based on your changes.


Patch:
diff --git a/services/graph/pkg/identity/ldap_school.go b/services/graph/pkg/identity/ldap_school.go
index f344a8d0431..3119d3854c5 100644
--- a/services/graph/pkg/identity/ldap_school.go
+++ b/services/graph/pkg/identity/ldap_school.go
@@ -135,7 +135,7 @@ func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error {
 	if !i.writeEnabled {
 		return errReadOnly
 	}
-	e, err := i.getSchoolByID(id)
+	e, err := i.getSchoolByNumberOrID(id)
 	if err != nil {
 		return err
 	}
@@ -150,10 +150,10 @@ func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error {
 }
 
 // GetEducationSchool implements the EducationBackend interface for the LDAP backend.
-func (i *LDAP) GetEducationSchool(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.EducationSchool, error) {
+func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrID string, queryParam url.Values) (*libregraph.EducationSchool, error) {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("GetEducationSchool")
-	e, err := i.getSchoolByID(nameOrID)
+	e, err := i.getSchoolByNumberOrID(numberOrID)
 	if err != nil {
 		return nil, err
 	}
@@ -211,7 +211,7 @@ func (i *LDAP) GetEducationSchoolUsers(ctx context.Context, id string) ([]*libre
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("GetEducationSchoolUsers")
 
-	schoolEntry, err := i.getSchoolByID(id)
+	schoolEntry, err := i.getSchoolByNumberOrID(id)
 	if err != nil {
 		return nil, err
 	}
@@ -261,7 +261,7 @@ func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolID string, m
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("AddUsersToEducationSchool")
 
-	schoolEntry, err := i.getSchoolByID(schoolID)
+	schoolEntry, err := i.getSchoolByNumberOrID(schoolID)
 	if err != nil {
 		return err
 	}
@@ -306,7 +306,7 @@ func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolID strin
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("RemoveUserFromEducationSchool")
 
-	schoolEntry, err := i.getSchoolByID(schoolID)
+	schoolEntry, err := i.getSchoolByNumberOrID(schoolID)
 	if err != nil {
 		return err
 	}
@@ -347,9 +347,15 @@ func (i *LDAP) getSchoolByDN(dn string) (*ldap.Entry, error) {
 	return i.getEntryByDN(dn, attrs, filter)
 }
 
-func (i *LDAP) getSchoolByID(id string) (*ldap.Entry, error) {
-	id = ldap.EscapeFilter(id)
-	filter := fmt.Sprintf("(%s=%s)", i.educationConfig.schoolAttributeMap.id, id)
+func (i *LDAP) getSchoolByNumberOrID(numberOrID string) (*ldap.Entry, error) {
+	numberOrID = ldap.EscapeFilter(numberOrID)
+	filter := fmt.Sprintf(
+		"(|(%s=%s)(%s=%s))",
+		i.educationConfig.schoolAttributeMap.id,
+		numberOrID,
+		i.educationConfig.schoolAttributeMap.schoolNumber,
+		numberOrID,
+	)
 	return i.getSchoolByFilter(filter)
 }
 


Test patch:
diff --git a/services/graph/pkg/identity/ldap_school_test.go b/services/graph/pkg/identity/ldap_school_test.go
index 0f1b980b248..953c92c1ee0 100644
--- a/services/graph/pkg/identity/ldap_school_test.go
+++ b/services/graph/pkg/identity/ldap_school_test.go
@@ -76,77 +76,139 @@ func TestCreateEducationSchool(t *testing.T) {
 }
 
 func TestDeleteEducationSchool(t *testing.T) {
-	lm := &mocks.Client{}
-	sr1 := &ldap.SearchRequest{
-		BaseDN:     "",
-		Scope:      2,
-		SizeLimit:  1,
-		Filter:     "(&(objectClass=ocEducationSchool)(owncloudUUID=abcd-defg))",
-		Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
-		Controls:   []ldap.Control(nil),
-	}
-	sr2 := &ldap.SearchRequest{
-		BaseDN:     "",
-		Scope:      2,
-		SizeLimit:  1,
-		Filter:     "(&(objectClass=ocEducationSchool)(owncloudUUID=xxxx-xxxx))",
-		Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
-		Controls:   []ldap.Control(nil),
+	tests := []struct {
+		name                 string
+		numberOrId           string
+		filter               string
+		expectedItemNotFound bool
+	}{
+		{
+			name:                 "Test delete school using schoolId",
+			numberOrId:           "abcd-defg",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
+			expectedItemNotFound: false,
+		},
+		{
+			name:                 "Test delete school using unknown schoolId",
+			numberOrId:           "xxxx-xxxx",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=xxxx-xxxx)(ocEducationSchoolNumber=xxxx-xxxx)))",
+			expectedItemNotFound: true,
+		},
+		{
+			name:                 "Test delete school using schoolNumber",
+			numberOrId:           "0123",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=0123)(ocEducationSchoolNumber=0123)))",
+			expectedItemNotFound: false,
+		},
+		{
+			name:                 "Test delete school using unknown schoolNumber",
+			numberOrId:           "3210",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=3210)(ocEducationSchoolNumber=3210)))",
+			expectedItemNotFound: true,
+		},
 	}
-	lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil)
-	lm.On("Search", sr2).Return(&ldap.SearchResult{Entries: []*ldap.Entry{}}, nil)
-	dr1 := &ldap.DelRequest{
-		DN: "ou=Test School",
-	}
-	lm.On("Del", dr1).Return(nil)
-	b, err := getMockedBackend(lm, eduConfig, &logger)
-	assert.Nil(t, err)
 
-	err = b.DeleteEducationSchool(context.Background(), "abcd-defg")
-	lm.AssertNumberOfCalls(t, "Search", 1)
-	lm.AssertNumberOfCalls(t, "Del", 1)
-	assert.Nil(t, err)
+	for _, tt := range tests {
+		lm := &mocks.Client{}
+		sr := &ldap.SearchRequest{
+			BaseDN:     "",
+			Scope:      2,
+			SizeLimit:  1,
+			Filter:     tt.filter,
+			Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
+			Controls:   []ldap.Control(nil),
+		}
+		if tt.expectedItemNotFound {
+			lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{}}, nil)
+		} else {
+			lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil)
+		}
+		dr := &ldap.DelRequest{
+			DN: "ou=Test School",
+		}
+		lm.On("Del", dr).Return(nil)
 
-	err = b.DeleteEducationSchool(context.Background(), "xxxx-xxxx")
-	lm.AssertNumberOfCalls(t, "Search", 2)
-	lm.AssertNumberOfCalls(t, "Del", 1)
-	assert.NotNil(t, err)
-	assert.Equal(t, "itemNotFound", err.Error())
+		b, err := getMockedBackend(lm, eduConfig, &logger)
+		assert.Nil(t, err)
+
+		err = b.DeleteEducationSchool(context.Background(), tt.numberOrId)
+		lm.AssertNumberOfCalls(t, "Search", 1)
+
+		if tt.expectedItemNotFound {
+			lm.AssertNumberOfCalls(t, "Del", 0)
+			assert.NotNil(t, err)
+			assert.Equal(t, "itemNotFound", err.Error())
+		} else {
+			assert.Nil(t, err)
+		}
+	}
 }
 
 func TestGetEducationSchool(t *testing.T) {
-	lm := &mocks.Client{}
-	sr1 := &ldap.SearchRequest{
-		BaseDN:     "",
-		Scope:      2,
-		SizeLimit:  1,
-		Filter:     "(&(objectClass=ocEducationSchool)(owncloudUUID=abcd-defg))",
-		Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
-		Controls:   []ldap.Control(nil),
-	}
-	sr2 := &ldap.SearchRequest{
-		BaseDN:     "",
-		Scope:      2,
-		SizeLimit:  1,
-		Filter:     "(&(objectClass=ocEducationSchool)(owncloudUUID=xxxx-xxxx))",
-		Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
-		Controls:   []ldap.Control(nil),
+	tests := []struct {
+		name                 string
+		numberOrId           string
+		filter               string
+		expectedItemNotFound bool
+	}{
+		{
+			name:                 "Test search school using schoolId",
+			numberOrId:           "abcd-defg",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
+			expectedItemNotFound: false,
+		},
+		{
+			name:                 "Test search school using unknown schoolId",
+			numberOrId:           "xxxx-xxxx",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=xxxx-xxxx)(ocEducationSchoolNumber=xxxx-xxxx)))",
+			expectedItemNotFound: true,
+		},
+		{
+			name:                 "Test search school using schoolNumber",
+			numberOrId:           "0123",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=0123)(ocEducationSchoolNumber=0123)))",
+			expectedItemNotFound: false,
+		},
+		{
+			name:                 "Test search school using unknown schoolNumber",
+			numberOrId:           "3210",
+			filter:               "(&(objectClass=ocEducationSchool)(|(owncloudUUID=3210)(ocEducationSchoolNumber=3210)))",
+			expectedItemNotFound: true,
+		},
 	}
-	lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil)
-	lm.On("Search", sr2).Return(&ldap.SearchResult{Entries: []*ldap.Entry{}}, nil)
-	b, err := getMockedBackend(lm, eduConfig, &logger)
-	assert.Nil(t, err)
-	school, err := b.GetEducationSchool(context.Background(), "abcd-defg", nil)
-	lm.AssertNumberOfCalls(t, "Search", 1)
-	assert.Nil(t, err)
-	assert.Equal(t, "Test School", school.GetDisplayName())
-	assert.Equal(t, "abcd-defg", school.GetId())
-	assert.Equal(t, "0123", school.GetSchoolNumber())
 
-	school, err = b.GetEducationSchool(context.Background(), "xxxx-xxxx", nil)
-	lm.AssertNumberOfCalls(t, "Search", 2)
-	assert.NotNil(t, err)
-	assert.Equal(t, "itemNotFound", err.Error())
+	for _, tt := range tests {
+		lm := &mocks.Client{}
+		sr := &ldap.SearchRequest{
+			BaseDN:     "",
+			Scope:      2,
+			SizeLimit:  1,
+			Filter:     tt.filter,
+			Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
+			Controls:   []ldap.Control(nil),
+		}
+		if tt.expectedItemNotFound {
+			lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{}}, nil)
+		} else {
+			lm.On("Search", sr).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil)
+		}
+
+		b, err := getMockedBackend(lm, eduConfig, &logger)
+		assert.Nil(t, err)
+
+		school, err := b.GetEducationSchool(context.Background(), tt.numberOrId, nil)
+		lm.AssertNumberOfCalls(t, "Search", 1)
+
+		if tt.expectedItemNotFound {
+			assert.NotNil(t, err)
+			assert.Equal(t, "itemNotFound", err.Error())
+		} else {
+			assert.Nil(t, err)
+			assert.Equal(t, "Test School", school.GetDisplayName())
+			assert.Equal(t, "abcd-defg", school.GetId())
+			assert.Equal(t, "0123", school.GetSchoolNumber())
+		}
+	}
 }
 
 func TestGetEducationSchools(t *testing.T) {
@@ -172,7 +234,7 @@ var schoolByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{
 	BaseDN:     "",
 	Scope:      2,
 	SizeLimit:  1,
-	Filter:     "(&(objectClass=ocEducationSchool)(owncloudUUID=abcd-defg))",
+	Filter:     "(&(objectClass=ocEducationSchool)(|(owncloudUUID=abcd-defg)(ocEducationSchoolNumber=abcd-defg)))",
 	Attributes: []string{"ou", "owncloudUUID", "ocEducationSchoolNumber"},
 	Controls:   []ldap.Control(nil),
 }
diff --git a/tests/acceptance/expected-failures-webUI-on-OCIS-storage.md b/tests/acceptance/expected-failures-webUI-on-OCIS-storage.md
index a71f3b2644f..790e2a8a476 100644
--- a/tests/acceptance/expected-failures-webUI-on-OCIS-storage.md
+++ b/tests/acceptance/expected-failures-webUI-on-OCIS-storage.md
@@ -80,10 +80,10 @@ Other free text and markdown formatting can be used elsewhere in the document if
 -   [webUILogin/openidLogin.feature:60](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUILogin/openidLogin.feature#L60)
 
 ### [Comments in sidebar](https://github.com/owncloud/web/issues/1158)
--   [webUIFilesDetails/fileDetails.feature:84](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L90)
--   [webUIFilesDetails/fileDetails.feature:98](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L106)
--   [webUIFilesDetails/fileDetails.feature:113](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L123)
--   [webUIFilesDetails/fileDetails.feature:128](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L140)
+-   [webUIFilesDetails/fileDetails.feature:84](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L84)
+-   [webUIFilesDetails/fileDetails.feature:98](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L98)
+-   [webUIFilesDetails/fileDetails.feature:113](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L113)
+-   [webUIFilesDetails/fileDetails.feature:128](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L128)
 
 ### [Deletion of a recursive folder from trashbin is not possible](https://github.com/owncloud/product/issues/188)
 -   [webUITrashbinDelete/trashbinDelete.feature:51](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUITrashbinDelete/trashbinDelete.feature#L51)
@@ -120,8 +120,8 @@ Other free text and markdown formatting can be used elsewhere in the document if
 -   [webUIUpload/upload.feature:43](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIUpload/upload.feature#L43)
 
 ### [Favorites deactivated in ocis temporarily](https://github.com/owncloud/ocis/issues/1228)
--   [webUIFilesDetails/fileDetails.feature:47](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L50)
--   [webUIFilesDetails/fileDetails.feature:67](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L70)
+-   [webUIFilesDetails/fileDetails.feature:47](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L47)
+-   [webUIFilesDetails/fileDetails.feature:67](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIFilesDetails/fileDetails.feature#L67)
 -   [webUIRenameFiles/renameFiles.feature:249](https://github.com/owncloud/web/blob/master/tests/acceptance/features/webUIRenameFiles/renameFiles.feature#L249)
 
 ### [PROPFIND to sub-folder of a shared resources with same name gives 404](https://github.com/owncloud/ocis/issues/3859)