Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

provisions to identify fields with multiple value #5109

Merged
merged 8 commits into from
Jan 29, 2024
Merged

provisions to identify fields with multiple value #5109

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6faf18c
provisions to identify fields with multiple value
HiteshRepo Jan 24, 2024
b770d02
fix test TestColumnDefinitionable_LegacyColumns
HiteshRepo Jan 24, 2024
162d32e
updates function name that sets multiple enabled by data type
HiteshRepo Jan 25, 2024
e2f2732
skips setting odatatype for text columns
HiteshRepo Jan 25, 2024
98b7e5e
fix lint
HiteshRepo Jan 25, 2024
abdf620
Merge remote-tracking branch 'origin/main' into lists-multiple
HiteshRepo Jan 29, 2024
55aed0d
address review comment
HiteshRepo Jan 29, 2024
3b2bdc5
Merge branch 'main' into lists-multiple
aviator-app[bot] Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/pkg/services/m365/api/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ const (
ChildCountFieldNamePart = "ChildCount"
LookupIDFieldNamePart = "LookupId"

ODataTypeFieldNamePart = "@odata.type"
ODataTypeFieldNameStringVal = "Collection(Edm.String)"

ReadOnlyOrHiddenFieldNamePrefix = "_"
DescoratorFieldNamePrefix = "@"

Expand Down
72 changes: 60 additions & 12 deletions src/pkg/services/m365/api/lists.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package api
import (
"context"
"fmt"
"reflect"
"strings"

"github.com/alcionai/clues"
Expand All @@ -18,6 +19,11 @@ import (

var ErrSkippableListTemplate = clues.New("unable to create lists with skippable templates")

type columnDetails struct {
isMultipleEnabled bool
hasDefaultedToText bool
}

// ---------------------------------------------------------------------------
// controller
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -277,7 +283,7 @@ func BytesToListable(bytes []byte) (models.Listable, error) {
// not attached in this method.
// ListItems are not included in creation of new list, and have to be restored
// in separate call.
func ToListable(orig models.Listable, listName string) (models.Listable, map[string]any) {
func ToListable(orig models.Listable, listName string) (models.Listable, map[string]*columnDetails) {
newList := models.NewList()

newList.SetContentTypes(orig.GetContentTypes())
Expand All @@ -294,7 +300,7 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str
newList.SetParentReference(orig.GetParentReference())

columns := make([]models.ColumnDefinitionable, 0)
columnNames := map[string]any{TitleColumnName: nil}
columnNames := map[string]*columnDetails{TitleColumnName: {}}

for _, cd := range orig.GetColumns() {
var (
Expand All @@ -316,8 +322,7 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str
continue
}

columns = append(columns, cloneColumnDefinitionable(cd))
columnNames[ptr.Val(cd.GetName())] = nil
columns = append(columns, cloneColumnDefinitionable(cd, columnNames))
}

newList.SetColumns(columns)
Expand All @@ -327,7 +332,10 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str

// cloneColumnDefinitionable utility function for encapsulating models.ColumnDefinitionable data
// into new object for upload.
func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDefinitionable {
func cloneColumnDefinitionable(
orig models.ColumnDefinitionable,
columnNames map[string]*columnDetails,
) models.ColumnDefinitionable {
newColumn := models.NewColumnDefinition()

// column attributes
Expand All @@ -351,7 +359,7 @@ func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDe
newColumn.SetEnforceUniqueValues(orig.GetEnforceUniqueValues())

// column types
setColumnType(newColumn, orig)
setColumnType(newColumn, orig, columnNames)

// Requires nil checks to avoid Graph error: 'General exception while processing'
defaultValue := orig.GetDefaultValue()
Expand All @@ -367,7 +375,13 @@ func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDe
return newColumn
}

func setColumnType(newColumn *models.ColumnDefinition, orig models.ColumnDefinitionable) {
func setColumnType(
newColumn *models.ColumnDefinition,
orig models.ColumnDefinitionable,
columnNames map[string]*columnDetails,
) {
colDetails := &columnDetails{}

switch {
case orig.GetText() != nil:
newColumn.SetText(orig.GetText())
Expand Down Expand Up @@ -398,14 +412,18 @@ func setColumnType(newColumn *models.ColumnDefinition, orig models.ColumnDefinit
case orig.GetPersonOrGroup() != nil:
newColumn.SetPersonOrGroup(orig.GetPersonOrGroup())
default:
colDetails.hasDefaultedToText = true

newColumn.SetText(models.NewTextColumn())
}

columnNames[ptr.Val(newColumn.GetName())] = colDetails
}

// CloneListItem creates a new `SharePoint.ListItem` and stores the original item's
// M365 data into it set fields.
// - https://learn.microsoft.com/en-us/graph/api/resources/listitem?view=graph-rest-1.0
func CloneListItem(orig models.ListItemable, columnNames map[string]any) models.ListItemable {
func CloneListItem(orig models.ListItemable, columnNames map[string]*columnDetails) models.ListItemable {
newItem := models.NewListItem()

// list item data
Expand Down Expand Up @@ -442,7 +460,7 @@ func CloneListItem(orig models.ListItemable, columnNames map[string]any) models.
// additionalData map
// Further documentation on FieldValueSets:
// - https://learn.microsoft.com/en-us/graph/api/resources/fieldvalueset?view=graph-rest-1.0
func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]any) models.FieldValueSetable {
func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]*columnDetails) models.FieldValueSetable {
fields := models.NewFieldValueSet()

additionalData := setAdditionalDataByColumnNames(orig, columnNames)
Expand All @@ -463,7 +481,7 @@ func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]any

func setAdditionalDataByColumnNames(
orig models.FieldValueSetable,
columnNames map[string]any,
columnNames map[string]*columnDetails,
) map[string]any {
if orig == nil {
return make(map[string]any)
Expand All @@ -472,15 +490,45 @@ func setAdditionalDataByColumnNames(
fieldData := orig.GetAdditionalData()
filteredData := make(map[string]any)

for colName := range columnNames {
if _, ok := fieldData[colName]; ok {
for colName, colDetails := range columnNames {
if val, ok := fieldData[colName]; ok {
setMultipleEnabledByFieldData(val, colDetails)

filteredData[colName] = fieldData[colName]
}

specifyODataType(filteredData, colDetails, colName)
}

return filteredData
}

func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) {
// for columns like 'choice', even though it has an option to hold single/multiple values,
// the columnDefinition property 'allowMultipleValues' is not available.
// Hence we determine single/multiple from the actual field data.
if reflect.TypeOf(val).Kind() == reflect.Slice {
colDetails.isMultipleEnabled = true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preference on general func behavior: rather than setting values a provided object (which causes coupling between the caller and the func), funcs that provide return values and let callers act on them (which provides cohesion without coupling), are easier to maintain in the long term. In this case, it's a better pattern to have a func whose only job is to check whether the value is a slice. Ex:

Suggested change
func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) {
// for columns like 'choice', even though it has an option to hold single/multiple values,
// the columnDefinition property 'allowMultipleValues' is not available.
// Hence we determine single/multiple from the actual field data.
if reflect.TypeOf(val).Kind() == reflect.Slice {
colDetails.isMultipleEnabled = true
}
}
func isSlice(val any) bool {
return reflect.TypeOf(val).Kind() == reflect.Slice
}


// when creating list items with multiple values for a single column
// we let the API know that we are sending a collection.
// Hence this adds an additional field '<columnName>@@odata.type'
// with value depending on type of column.
func specifyODataType(filteredData map[string]any, colDetails *columnDetails, colName string) {
// text column itself does not allow holding multiple values
// some columns like 'term'/'managed metadata' have,
// but they get defaulted to text column.
if colDetails.hasDefaultedToText {
return
}

if colDetails.isMultipleEnabled {
filteredData[colName+ODataTypeFieldNamePart] = ODataTypeFieldNameStringVal
}
}

func hasAddressFields(additionalData map[string]any) (map[string]any, string, bool) {
for k, v := range additionalData {
nestedFields, ok := v.(map[string]any)
Expand Down
103 changes: 86 additions & 17 deletions src/pkg/services/m365/api/lists_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,12 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_GetValidation() {
for _, test := range tests {
suite.Run(test.name, func() {
t := suite.T()
colNames := map[string]*columnDetails{}

orig := test.getOrig()
newCd := cloneColumnDefinitionable(orig)
newCd := cloneColumnDefinitionable(orig, colNames)

require.NotEmpty(t, newCd)

test.expect(t, newCd.GetValidation())
})
}
Expand Down Expand Up @@ -219,9 +219,10 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_GetDefaultValue() {
for _, test := range tests {
suite.Run(test.name, func() {
t := suite.T()
colNames := map[string]*columnDetails{}

orig := test.getOrig()
newCd := cloneColumnDefinitionable(orig)
newCd := cloneColumnDefinitionable(orig, colNames)

require.NotEmpty(t, newCd)
test.expect(t, newCd)
Expand Down Expand Up @@ -277,9 +278,8 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_ColumnType() {
for _, test := range tests {
suite.Run(test.name, func() {
t := suite.T()

orig := test.getOrig()
newCd := cloneColumnDefinitionable(orig)
colNames := map[string]*columnDetails{}
newCd := cloneColumnDefinitionable(test.getOrig(), colNames)

require.NotEmpty(t, newCd)
assert.True(t, test.checkFn(newCd))
Expand Down Expand Up @@ -332,7 +332,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() {
name string
getList func() *models.List
length int
expectedColNames map[string]any
expectedColNames map[string]*columnDetails
}{
{
name: "all legacy columns",
Expand All @@ -346,7 +346,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() {
return lst
},
length: 0,
expectedColNames: map[string]any{TitleColumnName: nil},
expectedColNames: map[string]*columnDetails{TitleColumnName: {}},
},
{
name: "title and legacy columns",
Expand All @@ -361,7 +361,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() {
return lst
},
length: 0,
expectedColNames: map[string]any{TitleColumnName: nil},
expectedColNames: map[string]*columnDetails{TitleColumnName: {}},
},
{
name: "readonly and legacy columns",
Expand All @@ -376,7 +376,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() {
return lst
},
length: 0,
expectedColNames: map[string]any{TitleColumnName: nil},
expectedColNames: map[string]*columnDetails{TitleColumnName: {}},
},
{
name: "legacy and a text column",
Expand All @@ -391,9 +391,9 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() {
return lst
},
length: 1,
expectedColNames: map[string]any{
TitleColumnName: nil,
textColumnName: nil,
expectedColNames: map[string]*columnDetails{
TitleColumnName: {},
textColumnName: {},
},
},
}
Expand Down Expand Up @@ -432,7 +432,7 @@ func (suite *ListsUnitSuite) TestFieldValueSetable() {
origFs := models.NewFieldValueSet()
origFs.SetAdditionalData(additionalData)

colNames := map[string]any{}
colNames := map[string]*columnDetails{}

fs := retrieveFieldData(origFs, colNames)
fsAdditionalData := fs.GetAdditionalData()
Expand All @@ -442,7 +442,7 @@ func (suite *ListsUnitSuite) TestFieldValueSetable() {
origFs = models.NewFieldValueSet()
origFs.SetAdditionalData(additionalData)

colNames["itemName"] = struct{}{}
colNames["itemName"] = &columnDetails{}

fs = retrieveFieldData(origFs, colNames)
fsAdditionalData = fs.GetAdditionalData()
Expand Down Expand Up @@ -509,8 +509,8 @@ func (suite *ListsUnitSuite) TestFieldValueSetable_Location() {
origFs := models.NewFieldValueSet()
origFs.SetAdditionalData(additionalData)

colNames := map[string]any{
"MyAddress": nil,
colNames := map[string]*columnDetails{
"MyAddress": {},
}

fs := retrieveFieldData(origFs, colNames)
Expand Down Expand Up @@ -703,6 +703,75 @@ func (suite *ListsUnitSuite) TestConcatenateHyperlinkFields() {
}
}

func (suite *ListsUnitSuite) TestSetAdditionalDataByColumnNames() {
t := suite.T()

tests := []struct {
name string
additionalData map[string]any
colName string
assertFn assert.BoolAssertionFunc
}{
{
name: "choice column, single value",
additionalData: map[string]any{
"choice": "good",
},
colName: "choice",
assertFn: assert.False,
},
{
name: "choice column, multiple values",
additionalData: map[string]any{
"choice": []string{"good", "ok"},
},
colName: "choice",
assertFn: assert.True,
},
{
name: "person column, single value",
additionalData: map[string]any{
"PersonsLookupId": 10,
},
colName: "PersonsLookupId",
assertFn: assert.False,
},
{
name: "person column, multiple values",
additionalData: map[string]any{
"Persons": []map[string]any{
{
"LookupId": 10,
"LookupValue": "Who1",
"Email": "[email protected]",
},
{
"LookupId": 11,
"LookupValue": "Who2",
"Email": "[email protected]",
},
},
},
colName: "Persons",
assertFn: assert.True,
},
}

for _, test := range tests {
origFs := models.NewFieldValueSet()
origFs.SetAdditionalData(test.additionalData)

colNames := map[string]*columnDetails{
test.colName: {},
}

suite.Run(test.name, func() {
setAdditionalDataByColumnNames(origFs, colNames)
test.assertFn(t, colNames[test.colName].isMultipleEnabled)
})
}
}

type ListsAPIIntgSuite struct {
tester.Suite
its intgTesterSetup
Expand Down
Loading