Adds `spp model apply` command. Closes #6119 #6453

mkm17 · 2024-10-28T17:54:06Z

Adds spp model apply command. Closes #6119

I have made some changes to the command compared to the initial specification.

I realized I forgot to add a parameter needed to locate the content site where the model is stored: contentCenterUrl
Let me know if name is ok. We can change it to modelContentSiteUrl or something else.

I also modified the viewOption parameter. During testing, I saw that it’s possible to set three different values:NewViewAsDefault, DoNotChangeDefault, TileViewAsDefault
The API does not return an error when attempting to apply a model to a custom SharePoint List. However, I’ve added a condition to check if the list is a document library. I hope that’s ok.
potentially, when the spp model get command is approved, the getModel function from this command should be exported to the spp.ts file so it can be used by both commands.

milanholemans · 2024-10-28T20:25:35Z

Thank you for the PR and the additional info @mkm17! We'll get back to you ASAP.

Adam-it · 2024-10-29T10:36:37Z

@mkm17 I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

milanholemans · 2024-12-08T18:33:25Z

@mkm17, could you resolve the merge conflicts, please?

mkm17 · 2024-12-20T16:59:37Z

hi @milanholemans , I apologize for the delay, but I finally found some time to resolve the conflicts here. I have also reviewed the code and made the changes you pointed out in some other spp commands.

milanholemans · 2025-01-03T23:41:33Z

Hi @mkm17, sorry again, could you resolve the conflicts? Will try to review it ASAP when they are resolved.

mkm17 · 2025-01-12T18:23:01Z

Hi @mkm17, sorry again, could you resolve the conflicts? Will try to review it ASAP when they are resolved.

Conflicts resolved :)

milanholemans

First of all, sorry for the late review. The last couple of weeks were a bit hectic for me. Anyway, I did a first review, and it looks already quite good. In the meantime, could you also rebase your branch with the latest main? There are no merge conflicts, but main contains some updated logic about how we call the telemetry collection function.

src/m365/spp/commands/model/model-apply.ts

src/m365/spp/commands/model/model-apply.spec.ts

docs/docs/cmd/spp/model/model-apply.mdx

milanholemans · 2025-03-11T17:40:58Z

potentially, when the spp model get command is approved, the getModel function from this command should be exported to the spp.ts file so it can be used by both commands.

Great suggestion, feel free to add it to this PR as well.

mkm17 · 2025-03-18T09:43:47Z

I have rebased a solution and I have switched options to zod setup. I hope that is correct

milanholemans · 2025-03-18T21:19:57Z

Converting the PR to use a ZOD scheme is very much appreciated. Thanks @mkm17!

Copilot

Pull Request Overview

This PR adds a new command ("spp model apply") that applies a trained model to a SharePoint document library and updates related logic and tests. Key changes include:

Introducing and using a shared getModel function in spp.ts for fetching models by title or unique id.
Creating the new model-apply command with proper URL validation, error handling, and list information retrieval.
Updating tests and command registration to support the new command functionality.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/utils/spp.ts	Added getModel function to retrieve models by title or id
src/m365/spp/commands/model/model-apply.ts	Implemented the spp model apply command and integrated validation
src/m365/spp/commands.ts	Registered the new MODEL_APPLY command
src/utils/spp.spec.ts	Added tests for getModel behavior
src/m365/spp/commands/model/model-get.ts	Refactored to leverage the shared getModel function
src/m365/spp/commands/model/model-get.spec.ts	Updated tests to stub spp.getModel and handle error scenarios
docs/src/config/sidebars.ts	Updated documentation sidebar to include the new model apply command

Files not reviewed (1)

docs/docs/cmd/spp/model/model-apply.mdx: Language not supported

Comments suppressed due to low confidence (1)

src/m365/spp/commands/model/model-apply.ts:16

[nitpick] Consider renaming 'contentCenterUrl' to 'modelContentSiteUrl' for increased clarity on its purpose, although the current name is acceptable if aligned with team conventions.

contentCenterUrl: z.string()

src/m365/spp/commands/model/model-apply.ts

milanholemans

Hi @mkm17, sorry for another late review on this PR. I've did another review, I feel we're almost there, let's do a couple more changes first.

milanholemans · 2025-04-16T20:44:12Z

src/m365/spp/commands/model/model-apply.ts

+    ),
+    id: zod.alias('i', z.string()
+      .refine(id => validation.isValidGuid(id) === true, id => ({
+        message: `${id} is not a valid GUID for option 'id'.`


Using Zod scheme, we don't have to repeat the name of the failing option anymore. Zod wil print this automatically.

milanholemans · 2025-04-16T20:44:51Z

src/m365/spp/commands/model/model-apply.ts

+        message: `${id} is not a valid GUID for option 'id'.`
+      }))
+      .optional()),
+    title: zod.alias('t', z.string().optional()),


Suggested change

title: zod.alias('t', z.string().optional()),

title: zod.alias('t', z.string()).optional(),

milanholemans · 2025-04-16T20:45:52Z

src/m365/spp/commands/model/model-apply.ts

+    listId: z.string().refine(listId => validation.isValidGuid(listId) === true, listId => ({
+      message: `${listId} is not a valid GUID for option 'listId'.`
+    })).optional(),


Let's make it consistent with the other options. Let's:

Start the refine on a new line

We shouldn't repeat the option name in the error message anymore

milanholemans · 2025-04-16T20:46:31Z

src/m365/spp/commands/model/model-apply.ts

+}
+
+class SppModelApplyCommand extends SpoCommand {
+  public readonly viewOptions: string[] = ['NewViewAsDefault', 'DoNotChangeDefault', 'TileViewAsDefault'];


Are we using this anywhere?

milanholemans · 2025-04-16T20:48:03Z

src/m365/spp/commands/model/model-apply.ts

+      .refine(options => !(options.id && options.title), {
+        message: `Specify either 'id' or 'title', but not both`
+      })


Using this refinement, we can just leave id and title empty.
Let's use the refinement below for id and title so we are sure exactly 1 value is provided.

milanholemans · 2025-04-16T21:17:34Z

src/m365/spp/commands/model/model-apply.spec.ts

Could we add an additional test that ensures the command doesn't output anything as result?

milanholemans · 2025-04-16T21:20:13Z

src/utils/spp.ts

+   * @returns SharePoint Premium model
+   */
+
+  async getModel(contentCenterUrl: string, title?: string, id?: string): Promise<SppModel> {


Personally, I'm more a fan of splitting this into two functions instead of having a 'useless' parameter.
getModelById and getModelByTitle would cover the load better.

milanholemans · 2025-04-16T21:22:31Z

src/utils/spp.spec.ts

+    await spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f');
+    assert(stubGet.calledOnce);


Instead of doing this, let's check for the output the function generates.

Suggested change

await spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f');

assert(stubGet.calledOnce);

const actual = await spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f');

assert.deepStrictEqual(actual, expected);

milanholemans · 2025-04-16T21:23:35Z

src/utils/spp.spec.ts

+
+
+    await assert.rejects(spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f'),
+      'File Not Found.');


Suggested change

'File Not Found.');

new Error('File Not Found.'));

milanholemans · 2025-04-16T21:40:03Z

src/m365/spp/commands/model/model-apply.ts

+      if (this.verbose) {
+        await logger.log(`Applying a model to a document library...`);
+      }


I'm a fan of adding more verbose logging to our commands. I suggest that we log something for about every request we do.

Assert the content center is valid

Getting model info

Getting list info

Applying model to list

mkm17 · 2025-05-13T21:56:03Z

@milanholemans Hi, thank you again for the review. I’ve also made some updates to other commands to align them with the changes in the spp.ts file.

Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024

mkm17 force-pushed the issues/6119_spp_model_apply branch 2 times, most recently from c69155f to f366ea7 Compare November 1, 2024 20:31

milanholemans marked this pull request as draft December 8, 2024 18:33

mkm17 force-pushed the issues/6119_spp_model_apply branch from f366ea7 to d7d8e52 Compare December 20, 2024 11:18

mkm17 marked this pull request as ready for review December 20, 2024 16:59

milanholemans self-assigned this Jan 3, 2025

milanholemans marked this pull request as draft January 3, 2025 23:41

mkm17 force-pushed the issues/6119_spp_model_apply branch from d7d8e52 to 4bd3c1e Compare January 12, 2025 18:14

mkm17 marked this pull request as ready for review January 12, 2025 18:22

milanholemans requested changes Mar 11, 2025

View reviewed changes

milanholemans marked this pull request as draft March 11, 2025 17:40

mkm17 force-pushed the issues/6119_spp_model_apply branch 4 times, most recently from 122ba9a to e0cb486 Compare March 18, 2025 09:42

mkm17 marked this pull request as ready for review March 18, 2025 09:46

milanholemans requested a review from Copilot March 18, 2025 22:15

Copilot AI reviewed Mar 18, 2025

View reviewed changes

src/m365/spp/commands/model/model-apply.ts Show resolved Hide resolved

milanholemans requested changes Apr 16, 2025

View reviewed changes

milanholemans marked this pull request as draft April 16, 2025 21:41

Adds spp model apply command. Closes pnp#6119

ad06a1d

mkm17 force-pushed the issues/6119_spp_model_apply branch from e0cb486 to ad06a1d Compare May 13, 2025 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `spp model apply` command. Closes #6119 #6453

Adds `spp model apply` command. Closes #6119 #6453

mkm17 commented Oct 28, 2024 •

edited

Loading

milanholemans commented Oct 28, 2024

Adam-it commented Oct 29, 2024

milanholemans commented Dec 8, 2024

mkm17 commented Dec 20, 2024

milanholemans commented Jan 3, 2025

mkm17 commented Jan 12, 2025

milanholemans left a comment

milanholemans commented Mar 11, 2025

mkm17 commented Mar 18, 2025

milanholemans commented Mar 18, 2025

Copilot AI left a comment

milanholemans left a comment

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

milanholemans Apr 16, 2025

mkm17 commented May 13, 2025

	title: zod.alias('t', z.string().optional()),
	title: zod.alias('t', z.string()).optional(),

		await spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f');
		assert(stubGet.calledOnce);



		await assert.rejects(spp.getModel('https://contoso.sharepoint.com/sites/portal', undefined, '9b1b1e42-794b-4c71-93ac-5ed92488b67f'),
		'File Not Found.');

Adds spp model apply command. Closes #6119 #6453

Are you sure you want to change the base?

Adds spp model apply command. Closes #6119 #6453

Conversation

mkm17 commented Oct 28, 2024 • edited Loading

milanholemans commented Oct 28, 2024

Adam-it commented Oct 29, 2024

milanholemans commented Dec 8, 2024

mkm17 commented Dec 20, 2024

milanholemans commented Jan 3, 2025

mkm17 commented Jan 12, 2025

milanholemans left a comment

Choose a reason for hiding this comment

milanholemans commented Mar 11, 2025

mkm17 commented Mar 18, 2025

milanholemans commented Mar 18, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

milanholemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkm17 commented May 13, 2025

Adds `spp model apply` command. Closes #6119 #6453

Adds `spp model apply` command. Closes #6119 #6453

mkm17 commented Oct 28, 2024 •

edited

Loading