Adds spp autofillcolumn apply command. Closes #6203

[mkm17]

Adds spp autofillcolumn apply command. Closes #6203

Hi, just a few remarks on this command:

I’ve added the columnId and columnTitle parameters to allow selection of the column by either option.

I noticed that retrieving the list instance, like in the getDocumentLibraryInfo function, appears in several places in the solution. Could you advise on where we should store this common code? Is spo.ts the correct file for it?

Similarly, for retrieving a field, should the getColumn function also be placed in spo.ts?

Do you think I should make these updates in this PR, or would it be better to create a separate one?

[milanholemans]

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

[milanholemans]

Sorry for the late reply @mkm17, I totally missed your remark last time.

I noticed that retrieving the list instance, like in the getDocumentLibraryInfo function, appears in several places in the solution. Could you advise on where we should store this common code? Is spo.ts the correct file for it?

Is this something we are using at a lot of other spots? Seems like in your code you are only retrieving the list BaseType. I don't know if we need this in other places?

Similarly, for retrieving a field, should the getColumn function also be placed in spo.ts?

SPO util will be the right place I guess, yes.

Do you think I should make these updates in this PR, or would it be better to create a separate one?

To me, it kind of depends on how many places we'll have to replace code by this util function. If it's just one other command, you can easily add it to this PR. If there are like 5 other commands sharing the same logic, I'd opt for a separate issue/PR.

[mkm17]

Hi, according to the latest comments from @martinlingstuyl on the specification, I have changed the current apply command to set.

The prompt and isEnabled parameters are optional if the selected column already has the autofill setting applied. In the update case, we can modify both of these settings.

I have used the column property AutofillInfo to check if it is an update and the _api/machinelearning/SetColumnLLMInfo endpoint to update the autofill column settings.

I noticed an issue with the test stage in the PR checks, but all tests pass when I run them locally. Can we rerun these checks without changing the code?

[milanholemans]

I noticed an issue with the test stage in the PR checks, but all tests pass when I run them locally. Can we rerun these checks without changing the code?

I just did rerun them, but it seems like it doesn't help. I think something is wrong with the macOs pipeline, in the last couple of days I saw this pipeline failing on other PRs as well. We'll have to take a look at it.

[milanholemans]

Hi @mkm17, I rebased your PR to the latest main to see if our pipeline has been fixed. I hope you don't mind if we take you as a test. Since I performed a force push, I recommend you delete your local branch and pull again from your forked repo.

[mkm17]

Hi @milanholemans sorry for the delay. I've finally made some changes to this command. it's now ready for review.

[martinlingstuyl]

Hi @mkm17, I've reviewed your PR, looks like solid work. I do have some comments. Could you look into those and fix them?

[martinlingstuyl]

If this is an optionset, we should add something like "Specify either..."

[martinlingstuyl]

I see you've added columnId and columnTitle, as opposed to the original specs where you just had column. It's not clear from columnTitle that you can add an internalName as well (which is possible as I see in the code). I'd suggest we add an additional columnInternalName option to the optionset... just for clarities sake. What about you?

[mkm17]

@martinlingstuyl, good idea, I have added it!

[martinlingstuyl]

Duplicate interface

[martinlingstuyl]

You could also use .string().uuid(), then you don't need to fix the error message and add the custom validation. the validation error message is less clear there, but we will try to fix that centrally.

[martinlingstuyl]

same here (.string().uuid())

[martinlingstuyl]

Let's add an additional line-break here for readability

[martinlingstuyl]

Let's add an additional line-break here for readability

[martinlingstuyl]

You could also add this information to the documentation

[martinlingstuyl]

For clarity lets add this request to a private function as well, just as the updateAutoFillColumnSettings function

[martinlingstuyl]

Lets use commandOptionsSchema.parse() to parse/validate the options when we pass them into the command action function.

Suggested change

	await command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com/sites/sales', columnId: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d', prompt: 'test', verbose: true } });
	await command.action(logger, { options: commandOptionsSchema.parse({ siteUrl: 'https://contoso.sharepoint.com/sites/sales', columnId: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d', prompt: 'test', verbose: true }) });

[martinlingstuyl]

Oh and also could you fix/rebase the branch to fix the merge conflicts? Thanks in advance!

[mkm17]

@martinlingstuyl thank you for the review. I hope that I have covered all the suggestions.