Skip to content
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

Enhances spo listitem batch add, Closes #4666 #5172

Closed
wants to merge 6 commits into from

Conversation

nicodecleyre
Copy link
Contributor

Closes #4666

@milanholemans
Copy link
Contributor

Thank you, we'll try to review it ASAP!

@Jwaegebaert Jwaegebaert self-assigned this Aug 26, 2023
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @nicodecleyre. When testing it locally I couldn't get it to work in a script with the commandlet ConvertTo-CSV. As this is one of the main reasons the issue we should make sure this is functional. With the script I also tested it with the param -UseQuotes set to Never as I thought that the quotes could be causing problems but that didn't appear to be the case.

Besides this I've also noticed a few pointers you can take a look at.

docs/docs/cmd/spo/listitem/listitem-batch-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/listitem/listitem-batch-add.mdx Outdated Show resolved Hide resolved
src/m365/spo/commands/listitem/listitem-batch-add.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/listitem/listitem-batch-add.ts Outdated Show resolved Hide resolved
@Jwaegebaert Jwaegebaert marked this pull request as draft August 26, 2023 19:55
@nicodecleyre nicodecleyre force-pushed the spo-listitem-batch-add branch from 77b09a9 to 511394d Compare September 1, 2023 18:24
@nicodecleyre
Copy link
Contributor Author

Thanks for this PR @nicodecleyre. When testing it locally I couldn't get it to work in a script with the commandlet ConvertTo-CSV. As this is one of the main reasons the issue we should make sure this is functional. With the script I also tested it with the param -UseQuotes set to Never as I thought that the quotes could be causing problems but that didn't appear to be the case.

Besides this I've also noticed a few pointers you can take a look at.

tbh, i've been testing with just a string as the csvContent parameter is a string. So here is how i've been testing (to be clear, in the following examples I am printing the process.argv from src/index.ts):
image

Regarding using ConvertTo-CSV, this works, but it only works when using no space anywhere
image

If you do however use a space, it doesn't work as this space causes trouble, but the problem is on the level of how the options are read by the CLI. So i'm wondering how you saw this and maybe also @martinlingstuyl as he created the issue
image

@milanholemans
Copy link
Contributor

What I think is that you should surround $csvContent with quotes.
--csvContent "$csvContent".

If you don't do that, your command will look like:
--csvContent Item A which is not interpreted as 1 value.

@martinlingstuyl
Copy link
Contributor

You could try the route Milan suggests @nicodecleyre. If we use "$csvContent" we probably also need to escape " in the variable.

Some trial and error may be necessary. And this should also work with csv's with quoted columns!

@nicodecleyre
Copy link
Contributor Author

What I think is that you should surround $csvContent with quotes. --csvContent "$csvContent".

If you don't do that, your command will look like: --csvContent Item A which is not interpreted as 1 value.

Thx for your suggestion @milanholemans, unfortunately the command exhibits the same behavior 😞
image

@Jwaegebaert
Copy link
Contributor

Perhaps we should consider changing formatting.parseCsvToJson(csvContent!) a bit. Currently, sending the converted CSV object to the CLI isn't straightforward. But an approach like this works:

$json = @(
  @{
    "Name" = "John"
    "Age" = 30
    "City" = "New York"
  },
  @{
    "Name" = "Jane"
    "Age" = 25
    "City" = "London"
  },
  @{
    "Name" = "Bob"
    "Age" = 40
    "City" = "Paris"
  }
)

$csvData = $json | ConvertTo-Csv -Delimiter ',' -UseQuotes Always -NoTypeInformation | ForEach-Object { $_ -replace '"', "'" }

# $csvData = 'City','Age','Name' 'New York','30','John' 'London','25','Jane' 'Paris','40','Bob'

Then execute the CLI command like this:

$> m365 spo listitem batch add --csvContent $csvData --webUrl ... --listTitle ... --verbose 

Executing command spo listitem batch add with options {"options":{"csvContent":"'City','Age','Name' 'New York','30','John' 'London','25','Jane' 'Paris','40','Bob'","webUrl":"...","listTitle":"...","verbose":true,"debug":true,"output":"json"}}
Starting to create batch items from csv from content

This way, we'll have a CSV object ready to be sent to the CLI. However, it's worth noting that the parseCsvToJson utility isn't able to handle this format just yet. This is an alternative approach to consider. 😊

@martinlingstuyl
Copy link
Contributor

@nicodecleyre: the problem seems to be the double quotes around the csv field values. Can you try escaping them? The same restriction goes up here as with feeding it complex Json: get the right escapes in place.

@martinlingstuyl
Copy link
Contributor

@Jwaegebaert, that would not be valid json, so I'd like us to steer clear of it.

@Jwaegebaert
Copy link
Contributor

@martinlingstuyl not sure what you mean by 'not valid json'?

@martinlingstuyl
Copy link
Contributor

Time to stop working for me I guess 😂

@martinlingstuyl
Copy link
Contributor

Anyway, I do think we need to check out first if escaping the quotes will be enough.

@nicodecleyre
Copy link
Contributor Author

nicodecleyre commented Sep 29, 2023

You mean escaping the quotes in PowerShell itself or escaping the quotes in the command? Because the problem lies in the way process.argv treats the options and it never reaches the command

@martinlingstuyl
Copy link
Contributor

I mean escaping the quotes in PowerShell itself. Option values like JSON strings have the same issues. We have a couple of paragraphs on the docs explaining how to properly escape in different versions of PowerShell, due to the nature of how PowerShell passes these values on to the lowerlying native commands.

@martinlingstuyl
Copy link
Contributor

What version of PowerShell do you use?

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Sep 29, 2023

Id try something like

$csvContent.Replace('"','\"')

for example.

@nicodecleyre
Copy link
Contributor Author

Hi all,

$csvContent.Replace('"','\"') seems do to the trick! Taking this into account i have added an example to the documentation as requested. Please let me know if there is still anything missing!

Thank you all for your help!

@nicodecleyre nicodecleyre marked this pull request as ready for review October 31, 2023 16:41
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Hey @nicodecleyre, pardon for the delayed response. Good one to add an example straight in the commands. However, I'm still having issues getting it to work for me. This is how I'm trying it at the moment:

$obj = @(
  @{
    Name = "Item A"
    Age = 10
  },
  @{
    Name = "Item B"
    Age = 20
  }
) 


$csv = ConvertTo-Csv -InputObject $obj -Delimiter "," -NoTypeInformation

$csvContent = $($csv | Out-String).Replace('"','\"')

m365 spo listitem batch add --csvContent $csvContent --webUrl "https://contoso.sharepoint.com/sites/Examplesite" --listTitle "bulk" --debug

Each time I get Error: Cannot read properties of undefined (reading 'forEach'). When I take a look at the data sent to _api/$batch it also doesn't look well formatted.

"data": "--batch_81575b16-73cd-4ec8-a0c8-90cbbab1e77e\nContent-Type: multipart/mixed; boundary=\"changeset_dec3739e-80d8-4e67-abe7-9bb2be9ee66d\"\n\nContent-Transfer-Encoding: binary\n\n--changeset_dec3739e-80d8-4e67-abe7-9bb2be9ee66d\nContent-Type: application/http\nContent-Transfer-Encoding: binary\n\nPOST https://contoso.sharepoint.com/sites/Examplesite/_api/web/lists/getByTitle('bulk')/AddValidateUpdateItemUsingPath HTTP/1.1\nAccept: application/json;odata=nometadata\nContent-Type: application/json;odata=verbose\nIf-Match: *\n\n{\n\"formValues\": [{\"FieldName\":\"\\\\\\\"Length\\\\\\\"\",\"FieldValue\":\"\\\\\\\"2\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"LongLength\\\\\\\"\",\"FieldValue\":\"\\\\\\\"2\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"Rank\\\\\\\"\",\"FieldValue\":\"\\\\\\\"1\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"SyncRoot\\\\\\\"\",\"FieldValue\":\"\\\\\\\"System.Object[]\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"IsReadOnly\\\\\\\"\",\"FieldValue\":\"\\\\\\\"False\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"IsFixedSize\\\\\\\"\",\"FieldValue\":\"\\\\\\\"True\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"IsSynchronized\\\\\\\"\",\"FieldValue\":\"\\\\\\\"False\\\\\\\"\"},{\"FieldName\":\"\\\\\\\"Count\\\\\\\"\",\"FieldValue\":\"\\\\\\\"2\\\\\\\"\"}]\n}\n\n--changeset_dec3739e-80d8-4e67-abe7-9bb2be9ee66d--\n\n--batch_81575b16-73cd-4ec8-a0c8-90cbbab1e77e--\n"

docs/docs/cmd/spo/listitem/listitem-batch-add.mdx Outdated Show resolved Hide resolved
@Jwaegebaert Jwaegebaert marked this pull request as draft November 23, 2023 18:52
@nicodecleyre nicodecleyre force-pushed the spo-listitem-batch-add branch from bff8a5b to 5f8185f Compare December 12, 2023 18:49
@nicodecleyre
Copy link
Contributor Author

$obj = @(
  @{
    Name = "Item A"
    Age = 10
  },
  @{
    Name = "Item B"
    Age = 20
  }
) 


$csv = ConvertTo-Csv -InputObject $obj -Delimiter "," -NoTypeInformation

Hi @Jwaegebaert, the problem lies in how PowerShell treats the object.. The way you did it, your array consists of hashtables. When you convert this with ConvertTo-Csv, it converts the properties of the hashtable, and not the actual object as you can see below.

image

In order for ConvertTo-Csv to convert the actual object, your array must consist of PSCustomObjects:

$obj = @(
  [PSCustomObject]@{
    Name = "Item A"
    Age = 10
  },
  [PSCustomObject]@{
    Name = "Item B"
    Age = 20
  }
) 

AND we have to use

$obj | ConvertTo-Csv -NoTypeInformation -Delimiter ","

Instead of

 ConvertTo-Csv -InputObject $obj -Delimiter "," -NoTypeInformation

This goes beyond the context of the CLI for Microsoft365 and is due to how ConvertTo-CSV handles an array object. I'll update the example with a multiple object array so that the user can use it properly.

To conclude, here an example of how it works:
image

Results in:
image

@nicodecleyre nicodecleyre marked this pull request as ready for review December 12, 2023 19:52
@Jwaegebaert
Copy link
Contributor

Hey, I did some digging and finally figured out what's causing the issue. I actually tried the suggestions you gave earlier, but they didn't do the trick for me. It's not really a big problem, more like a hassle. It seems you're running your scripts on PS version 5, while I'm using PS Core. I was able to smoothly add my object array to the SP list with PS 5, but with PS Core, it throws an error regarding the column '\"Name\"' not being found.

I personally prefer a solution that works with the latest version, PS Core, or one that works with both versions. I think we should explore a workaround for this. @pnp/cli-for-microsoft-365-maintainers, what do you guys think?

@Jwaegebaert Jwaegebaert marked this pull request as draft December 17, 2023 18:45
@nicodecleyre
Copy link
Contributor Author

Hey, I did some digging and finally figured out what's causing the issue. I actually tried the suggestions you gave earlier, but they didn't do the trick for me. It's not really a big problem, more like a hassle. It seems you're running your scripts on PS version 5, while I'm using PS Core. I was able to smoothly add my object array to the SP list with PS 5, but with PS Core, it throws an error regarding the column '\"Name\"' not being found.

I personally prefer a solution that works with the latest version, PS Core, or one that works with both versions. I think we should explore a workaround for this. @pnp/cli-for-microsoft-365-maintainers, what do you guys think?

I see.. PS 5 on the left, PS 7 on the right
image

@nicodecleyre
Copy link
Contributor Author

@Jwaegebaert, i've added a fix for PS 7. Tested in both PS 5 & PS 7 and it works in both.
Can you take another look whenever you have time please?

@nicodecleyre nicodecleyre marked this pull request as ready for review December 17, 2023 21:15
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

Awesome work with the quick fix @nicodecleyre, I've a few more pointers and then we should be ready to get this live.

docs/docs/cmd/spo/listitem/listitem-batch-add.mdx Outdated Show resolved Hide resolved
src/m365/spo/commands/listitem/listitem-batch-add.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/listitem/listitem-batch-add.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/listitem/listitem-batch-add.ts Outdated Show resolved Hide resolved
@Jwaegebaert Jwaegebaert marked this pull request as draft December 17, 2023 21:57
@nicodecleyre nicodecleyre marked this pull request as ready for review December 18, 2023 17:12
Copy link
Contributor

@Jwaegebaert Jwaegebaert left a comment

Choose a reason for hiding this comment

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

@nicodecleyre awesome work! I'll get this merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: add 'content' option to 'spo listitem batch add'
4 participants