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.

Sign up for GitHub

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

Jump to bottom

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

Draft

mkm17 wants to merge 1 commit into pnp:main from mkm17:issues/6119_spp_model_apply

Contributor

mkm17 commented Oct 28, 2024 •

edited

Loading

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.

Contributor

milanholemans commented Oct 28, 2024

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

Adam-it added the hacktoberfest-accepted label

Member

Adam-it commented Oct 29, 2024

@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 🤩

mkm17 force-pushed the issues/6119_spp_model_apply branch 2 times, most recently from c69155f to f366ea7 Compare

November 1, 2024 20:31

Contributor

milanholemans commented Dec 8, 2024

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

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

Contributor Author

mkm17 commented Dec 20, 2024

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.

mkm17 marked this pull request as ready for review

December 20, 2024 16:59

Contributor

milanholemans commented Jan 3, 2025

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

milanholemans self-assigned this

milanholemans marked this pull request as draft

January 3, 2025 23:41


          Adds spp model apply command. Closes pnp#6119

4bd3c1e

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

Contributor Author

mkm17 commented Jan 12, 2025

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

Conflicts resolved :)

milanholemans requested changes

View reviewed changes

Contributor

milanholemans left a comment

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

+                #initOptions(): void {
+                  this.options.unshift(
+                    {
+                      option: '-u, --siteUrl <siteUrl>'

Contributor

milanholemans Mar 11, 2025

Since a list can be on a sub site, I'm wondering if we should rename this option to webUrl instead. However, it would be inconsistent with the other spp model commands. Is it possible to create/list/remove models on a sub site? If so, maybe we should consider renaming their option as well.

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

Comment on lines +94 to +101

+                      if (args.options.id && !validation.isValidGuid(args.options.id)) {
+                        return `${args.options.id} in parameter id is not a valid GUID`;
+                      }
+                      if (args.options.listId &&
+                        !validation.isValidGuid(args.options.listId)) {
+                        return `${args.options.listId} in parameter listId is not a valid GUID`;
+                      }

Contributor

milanholemans Mar 11, 2025

Suggested change

      
                    if (args.options.id && !validation.isValidGuid(args.options.id)) {
          
                      return `${args.options.id} in parameter id is not a valid GUID`;
          
                    }
          
                    if (args.options.listId &&
          
                      !validation.isValidGuid(args.options.listId)) {
          
                      return `${args.options.listId} in parameter listId is not a valid GUID`;
          
                    }
          
                    if (args.options.id && !validation.isValidGuid(args.options.id)) {
          
                      return `${args.options.id} is not a valid GUID for option 'id'.`;
          
                    }
          
                    if (args.options.listId &&
          
                      !validation.isValidGuid(args.options.listId)) {
          
                      return `${args.options.listId} is not a valid GUID for option 'listId'.`;
          
                    }

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

+                        return `${args.options.listId} in parameter listId is not a valid GUID`;
+                      }
+                      if (typeof args.options.viewOption !== 'undefined') {

Contributor

milanholemans Mar 11, 2025

We shouldn't compare it with a string, right?

Suggested change

      
                    if (typeof args.options.viewOption !== 'undefined') {
          
                    if (typeof args.options.viewOption !== undefined) {

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

+                      }
+                      if (typeof args.options.viewOption !== 'undefined') {
+                        if (!this.viewOptions.some(viewOption => viewOption.toLocaleLowerCase() === args.options.viewOption?.toLowerCase())) {

Contributor

milanholemans Mar 11, 2025

Suggested change

      
                      if (!this.viewOptions.some(viewOption => viewOption.toLocaleLowerCase() === args.options.viewOption?.toLowerCase())) {
          
                      if (!this.viewOptions.some(viewOption => viewOption.toLowerCase() === args.options.viewOption?.toLowerCase())) {

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

+                      if (typeof args.options.viewOption !== 'undefined') {
+                        if (!this.viewOptions.some(viewOption => viewOption.toLocaleLowerCase() === args.options.viewOption?.toLowerCase())) {
+                          return `The value of parameter viewOption must be ${this.viewOptions.join(', ')}`;

Contributor

milanholemans Mar 11, 2025

Suggested change

      
                        return `The value of parameter viewOption must be ${this.viewOptions.join(', ')}`;
          
                        return `The value of parameter viewOption must be: ${this.viewOptions.join(', ')}.`;

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

+                before(() => {
+                  sinon.stub(auth, 'restoreAuth').resolves();
+                  sinon.stub(telemetry, 'trackEvent').returns();

Contributor

milanholemans Mar 11, 2025

Suggested change

      
                sinon.stub(telemetry, 'trackEvent').returns();
          
                sinon.stub(telemetry, 'trackEvent').resolves();

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

+                  await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com/sites/sales', contentCenterUrl: 'https://contoso.sharepoint.com/sites/contentCenter', id: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d' } }), new CommandError('List does not exist. The page you selected contains a list that does not exist. It may have been deleted by another user.'));
+                });
+                it('corretly handles error when try to apply model to SP list', async () => {

Contributor

milanholemans Mar 11, 2025

Suggested change

      
              it('corretly handles error when try to apply model to SP list', async () => {
          
              it('correctly handles error when try to apply model to SP list', async () => {

Test names below have the same typo.

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

+                    throw `${opts.url} is invalid request`;
+                  });
+                  await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com/sites/sales', contentCenterUrl: 'https://contoso.sharepoint.com/sites/contentCenter', id: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d' } }), new CommandError('List does not exist. The page you selected contains a list that does not exist. It may have been deleted by another user.'));

Contributor

milanholemans Mar 11, 2025

Let's put the error on a new line to make it a bit more readable (same with other tests).

Suggested change

      
                await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com/sites/sales', contentCenterUrl: 'https://contoso.sharepoint.com/sites/contentCenter', id: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d' } }), new CommandError('List does not exist. The page you selected contains a list that does not exist. It may have been deleted by another user.'));
          
                await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com/sites/sales', contentCenterUrl: 'https://contoso.sharepoint.com/sites/contentCenter', id: '9b1b1e42-794b-4c71-93ac-5ed92488b67f', listId: '421b1e42-794b-4c71-93ac-5ed92488b67d' } }),
          
                new CommandError('List does not exist. The page you selected contains a list that does not exist. It may have been deleted by another user.'));

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

+              : Server or web-relative URL of the library to which the model will be applied. Specify either `listTitle`, `listId`, or `listUrl` but not multiple.
+              `--viewOption [viewOption]`
+              : Defines whether the model view should be set as the default view for the document library. Allowed values are: `NewViewAsDefault`, `DoNotChangeDefault`, `TileViewAsDefault`.

Contributor

milanholemans Mar 11, 2025

Let's also describe which value is used as default.

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

Comment on lines +47 to +69

+              Applies a trained document understanding model using its unique ID to a document library, identified by its title.
+              ```sh
+              m365 spp model apply --siteUrl "https://contoso.sharepoint.com" --contentCenterUrl "https://contoso.sharepoint.com/sites/ContentCenter" --id "7645e69d-21fb-4a24-a17a-9bdfa7cb63dc" --listTitle "Shared Documents"
+              ```
+              Applies a trained document understanding model using its display name to a document library, identified by its title.
+              ```sh
+              m365 spp model apply --siteUrl "https://contoso.sharepoint.com" --contentCenterUrl "https://contoso.sharepoint.com/sites/ContentCenter" --title "ModelExample" --listTitle "Shared Documents"
+              ```
+              Applies a trained document understanding model using its display name to a document library, identified by its URL.
+              ```sh
+              m365 spp model apply --siteUrl "https://contoso.sharepoint.com" --contentCenterUrl "https://contoso.sharepoint.com/sites/ContentCenter" --title "ModelExample" --listUrl "/Shared Documents"
+              ```
+              Applies a trained document understanding model using its display name to a document library, identified by its unique ID.
+              ```sh
+              m365 spp model apply --siteUrl "https://contoso.sharepoint.com" --contentCenterUrl "https://contoso.sharepoint.com/sites/ContentCenter" --title "ModelExample" --listId "b4cfa0d9-b3d7-49ae-a0f0-f14ffdd005f7"
+              ```

Contributor

milanholemans Mar 11, 2025

Could we also use viewOption at least in one example?

milanholemans marked this pull request as draft

March 11, 2025 17:40

Contributor

milanholemans commented Mar 11, 2025

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.

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

Labels

hacktoberfest-accepted