addressed code review feedback

Saurabh7019 · Saurabh7019 · commit 90f195907f5d · 2025-08-09T12:29:04.000+02:00
diff --git a/.devproxy/api-specs/sharepoint.yaml b/.devproxy/api-specs/sharepoint.yaml
@@ -104,6 +104,47 @@ paths:
       responses:
         200:
           description: OK
+  /_api/web/GetFileById({fileId})/versions/:
+    get:
+      parameters:
+        - name: fileId
+          in: path
+          required: true
+          schema:
+            type: string
+          example: "'19bbfec4-4425-4660-95cb-da1887baa7b9'"
+      security:
+        - delegated:
+            - AllSites.Read
+            - AllSites.Write
+            - AllSites.Manage
+            - AllSites.FullControl
+      responses:
+        200:
+          description: OK
+  /_api/web/GetFileById({fileId})/versions({versionId})/SetExpirationDate():
+    post:
+      parameters:
+        - name: fileId
+          in: path
+          required: true
+          schema:
+            type: string
+          example: "'19bbfec4-4425-4660-95cb-da1887baa7b9'"
+        - name: versionId
+          in: path
+          required: true
+          schema:
+            type: integer
+          example: 1030
+      security:
+        - delegated:
+            - AllSites.Write
+            - AllSites.Manage
+            - AllSites.FullControl
+      responses:
+        200:
+          description: OK
   /_api/web/GetFolderByServerRelativePath(DecodedUrl={folderPath}):
     get:
       parameters:
diff --git a/docs/docs/cmd/spo/file/file-version-keep.mdx b/docs/docs/cmd/spo/file/file-version-keep.mdx
@@ -28,15 +28,34 @@ m365 spo file version keep [options]
 
 <Global />
 
+## Permissions
+
+<Tabs>
+  <TabItem value="Delegated" label="Delegated">
+  
+  | Resource   | Permissions                                      |
+  |------------|--------------------------------------------------|
+  | SharePoint | AllSites.Write, AllSites.Manage, AllSites.FullControl |
+
+  </TabItem>
+  <TabItem value="Application" label="Application">
+  
+  | Resource   | Permissions                                         |
+  |------------|-----------------------------------------------------|
+  | SharePoint | Sites.ReadWrite.All, Sites.Manage.All, Sites.FullControl.All |
+
+  </TabItem>
+</Tabs>
+
 ## Examples
 
-Mark a file version as never expiring by webUrl and fileUrl.
+Mark a file version as never expiring by file URL.
 
 ```sh
 m365 spo file version keep --webUrl "https://contoso.sharepoint.com/sites/marketing" --fileUrl "/sites/marketing/Documents/report.docx" --label "6.0"
 ```
 
-Mark a file version as never expiring by webUrl and fileId.
+Mark a file version as never expiring by file ID.
 
 ```sh
 m365 spo file version keep --webUrl "https://contoso.sharepoint.com/sites/marketing" --fileId "12345678-90ab-cdef-1234-567890abcdef" --label "6.0"
diff --git a/src/m365/spo/commands/file/file-version-keep.spec.ts b/src/m365/spo/commands/file/file-version-keep.spec.ts
@@ -85,6 +85,11 @@ describe(commands.FILE_VERSION_KEEP, () => {
     assert.strictEqual(actual.success, false);
   });
 
+  it('fails validation if label is not specified', async () => {
+    const actual = commandOptionsSchema.safeParse({ webUrl: validWebUrl, fileUrl: validFileUrl });
+    assert.strictEqual(actual.success, false);
+  });
+
   it('passes validation if fileUrl is specified', async () => {
     const actual = await command.validate({ options: { webUrl: validWebUrl, label: validLabel, fileUrl: validFileUrl } }, commandInfo);
     assert.strictEqual(actual, true);
@@ -97,7 +102,7 @@ describe(commands.FILE_VERSION_KEEP, () => {
 
   it('ensures that a specific file version will never expire (fileUrl)', async () => {
     sinon.stub(request, 'get').callsFake(async (opts) => {
-      if (opts.url === `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions/?$filter=VersionLabel eq '${validLabel}'`) {
+      if (opts.url === `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions/?$filter=VersionLabel eq '${validLabel}'&$select=Id`) {
         return {
           value: [
             {
@@ -111,7 +116,7 @@ describe(commands.FILE_VERSION_KEEP, () => {
       throw 'Invalid request';
     });
 
-    sinon.stub(request, 'post').callsFake(async (opts) => {
+    const postStub: sinon.SinonStub = sinon.stub(request, 'post').callsFake(async (opts) => {
       if (opts.url === `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions(1)/SetExpirationDate()`) {
         return;
       }
@@ -120,11 +125,12 @@ describe(commands.FILE_VERSION_KEEP, () => {
     });
 
     await command.action(logger, { options: { webUrl: validWebUrl, fileUrl: validFileUrl, label: validLabel, verbose: true } });
+    assert.strictEqual(postStub.lastCall.args[0].url, `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions(1)/SetExpirationDate()`);
   });
 
   it('ensures that a specific file version will never expire (fileId)', async () => {
     sinon.stub(request, 'get').callsFake(async (opts) => {
-      if (opts.url === `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions/?$filter=VersionLabel eq '${validLabel}'`) {
+      if (opts.url === `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions/?$filter=VersionLabel eq '${validLabel}'&$select=Id`) {
         return {
           value: [
             {
@@ -138,7 +144,7 @@ describe(commands.FILE_VERSION_KEEP, () => {
       throw 'Invalid request';
     });
 
-    sinon.stub(request, 'post').callsFake(async (opts) => {
+    const postStub: sinon.SinonStub = sinon.stub(request, 'post').callsFake(async (opts) => {
       if (opts.url === `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions(1)/SetExpirationDate()`) {
         return;
       }
@@ -147,11 +153,12 @@ describe(commands.FILE_VERSION_KEEP, () => {
     });
 
     await command.action(logger, { options: { webUrl: validWebUrl, fileId: validFileId, label: validLabel, verbose: true } });
+    assert.strictEqual(postStub.lastCall.args[0].url, `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions(1)/SetExpirationDate()`);
   });
 
   it('correctly handles error when the specified version does not exist', async () => {
     sinon.stub(request, 'get').callsFake(async (opts) => {
-      if (opts.url === `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions/?$filter=VersionLabel eq '${validLabel}'`) {
+      if (opts.url === `${validWebUrl}/_api/web/GetFileByServerRelativePath(DecodedUrl='${formatting.encodeQueryParameter(validFileUrl)}')/versions/?$filter=VersionLabel eq '${validLabel}'&$select=Id`) {
         return { value: [] };
       }
 
@@ -163,7 +170,7 @@ describe(commands.FILE_VERSION_KEEP, () => {
 
   it('correctly handles API OData error', async () => {
     sinon.stub(request, 'get').callsFake(async (opts) => {
-      if (opts.url === `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions/?$filter=VersionLabel eq '${validLabel}'`) {
+      if (opts.url === `${validWebUrl}/_api/web/GetFileById('${validFileId}')/versions/?$filter=VersionLabel eq '${validLabel}'&$select=Id`) {
         throw {
           error: {
             'odata.error': {
diff --git a/src/m365/spo/commands/file/file-version-keep.ts b/src/m365/spo/commands/file/file-version-keep.ts
@@ -17,10 +17,10 @@ export const options = globalOptionsZod
       }))
     ),
     fileUrl: z.string().optional(),
-    fileId: zod.alias('i', z.string().optional()
+    fileId: zod.alias('i', z.string()
       .refine(id => id === undefined || validation.isValidGuid(id), id => ({
         message: `'${id}' is not a valid GUID.`
-      }))
+      })).optional()
     ),
     label: z.string()
   })
@@ -47,7 +47,7 @@ class SpoFileVersionKeepCommand extends SpoCommand {
 
   public getRefinedSchema(schema: z.ZodTypeAny): z.ZodEffects<any> | undefined {
     return schema
-      .refine(options => (options.fileUrl !== undefined) !== (options.fileId !== undefined), {
+      .refine(options => [options.fileUrl, options.fileId].filter(o => o !== undefined).length === 1, {
         message: `Specify 'fileUrl' or 'fileId', but not both.`
       });
   }
@@ -58,12 +58,12 @@ class SpoFileVersionKeepCommand extends SpoCommand {
     }
 
     try {
-      const fileUrl = this.getFileUrl(args.options.webUrl, args.options.fileUrl, args.options.fileId);
+      const baseApiUrl = this.getBaseApiUrl(args.options.webUrl, args.options.fileUrl, args.options.fileId);
 
       const requestVersionOptions: CliRequestOptions = {
-        url: `${fileUrl}/versions/?$filter=VersionLabel eq '${args.options.label}'`,
+        url: `${baseApiUrl}/versions/?$filter=VersionLabel eq '${args.options.label}'&$select=Id`,
         headers: {
-          'accept': 'application/json;odata=nometadata'
+          accept: 'application/json;odata=nometadata'
         },
         responseType: 'json'
       };
@@ -76,9 +76,9 @@ class SpoFileVersionKeepCommand extends SpoCommand {
       }
 
       const requestExpirationOptions: CliRequestOptions = {
-        url: `${fileUrl}/versions(${version.ID})/SetExpirationDate()`,
+        url: `${baseApiUrl}/versions(${version.ID})/SetExpirationDate()`,
         headers: {
-          'accept': 'application/json;odata=nometadata',
+          accept: 'application/json;odata=nometadata',
           'content-type': 'application/json'
         },
         responseType: 'json'
@@ -91,7 +91,7 @@ class SpoFileVersionKeepCommand extends SpoCommand {
     }
   }
 
-  private getFileUrl(webUrl: string, fileUrl?: string, fileId?: string): string {
+  private getBaseApiUrl(webUrl: string, fileUrl?: string, fileId?: string): string {
     let requestUrl: string;
 
     if (fileUrl) {
