provision to post and delete lists or list items

[HiteshRepo]

• adds methods to Lists client wrapper to create list and list items
• adds methods to Lists client wrapper to delete list
• handles graph error: General exception while processing by column validation nil check
• handles graph error: Unable to identify column definition
• handles location type of columns and its data

Does this PR need a docs update or release note?

• ⛔ No

Type of change

• 🌻 Feature

Issue(s)

#4754

Test Plan

• 💪 Manual
• ⚡ Unit test
• 💚 E2E

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[ryanfkeepers]

suggestion: turn these values into constants. Use those consts as both the keys in your list structure maps, as well as here in the tests.

[ryanfkeepers]

Suggested change

	return nil, clues.WrapWC(ctx, err, "creating old list")
	return nil, clues.WrapWC(ctx, err, "generating list from stored bytes")

[ryanfkeepers]

This behavior is opaque. Can you please add a comment explaining what's being handled here, and why that handling is necessary? Comments like that will help all future maintainers grok the implementation.

[ryanfkeepers]

Suggested change

// Restore to List base to M365 back store

[ryanfkeepers]

nit: prefer multiline on func chains for code readability.

Suggested change

	restoredList, err := c.Stable.Client().Sites().BySiteId(siteID).Lists().Post(ctx, newList, nil)
	restoredList, err := c.Stable.
	Client().
	Sites().
	BySiteId(siteID).
	Lists().
	Post(ctx, newList, nil)

[ryanfkeepers]

hardcoded strings should get const values, please. This applies to all cases in this PR, not only this line.

[ryanfkeepers]

I like your impulse to centralize these values. It's good for code readability and cleanliness. Looking at the usage of these maps, however, I think there's a couple extra protections that would be nice. In general, this suggestion comes from how using global maps can produce a smell, since alterations to those maps at runtime can be difficult to detect and debug.

For the most part, it seems like you're using the maps purely as a check for whether a key exists in a certain set. For those uses, I'd suggest getter funcs like hasKey(string) bool.

In one other case, you're actually looking for a slice of the keys, and are hacking the map to do the same. In that case I'd suggest a func like keys() []string.

Putting it together, you can easily get both cases with a type wrapper, like so:

type keySet map[string]struct {}

func (ks keySet) hasKey(k string) bool {...}
func (ks keySet) keys() []string {...}

var legacyColumns = keySet{...}

That makes it safe and easy for implementations to do the same check you're currently doing by instead calling if legacyColumns.hasKey(k) {...}

[ryanfkeepers]

you can pack the assert call into the property itself here.

Suggested change

	checkNil bool
	expect assert.ValueAssertionFunc

This lets you change the test value to:

expect: assert.Nil // or assert.NotNil

and verify the result like so

newCd := cloneColumnDefinitionable(orig)
require.NotEmpty(t, newCd)
test.expect(t, newCd.GetValidation())

[ryanfkeepers]

For test naming, code standard is to follow golang's testable example naming schemes. see: https://go.dev/blog/examples#example-function-names

In short, it goes something like TestStructName_FuncName_subtestExplanation.

so in this case you're testing:

Suggested change

	func (suite *ListsUnitSuite) TestColumnDefinitionValidationSetWhenProvided() {
	func (suite *ListsUnitSuite) TestColumnDefinitionable_GetValidation() {

[ryanfkeepers]

This is a lot of chaff code. Is there any way to centralize it, or parts of it, for ease of re-use across multipl tests?

[ryanfkeepers]

looking good! Thank you for the updates.

[ryanfkeepers]

as a warning, this may collide with the golang maps package, which we use elsewhere. I don't know a better name off the top of my head. Maybe keys? Then you can change the value from keys.KeySet to just Set, for keys.Set?

[ryanfkeepers]

Nit: no underscore needed when naming the function under test directly. Underscores are for methods, variables, and extensions. eg:

func TestStruct()
func TestStruct_Method()
func TestFunc()
func TestStruct_hasSpecificProperty()
func TestStruct_Method_doesSomethingSpecific()
func TestFunc_doesSomethingSpecific()

Suggested change

	func (suite *KeySetTestSuite) Test_HasKey() {
	func (suite *KeySetTestSuite) TestHasKey() {

[sonarcloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud