2128/confirm prompt validator not invoked as expected (#2378)

* fix(abap-deploy-config-inquirer ): handle confirm validator not being invoked

* fix(abap-deploy-config-inquirer ): add changeset

* fix(abap-deploy-config-inquirer ): fix tests

* fix(abap-deploy-config-inquirer ): further cleanup based on testing

* fix(abap-deploy-config-inquirer ): add more tests around scp setter method

* fix(abap-deploy-config-inquirer ): add comment

* fix(abap-deploy-config-inquirer ): add more robust test

* fix(abap-deploy-config-inquirer ): address sonar issue

* fix(abap-deploy-config-inquirer ): address sonar issue

* fix(abap-deploy-config-inquirer ): address sonar issue

.changeset/thick-owls-collect.md

@@ -0,0 +1,5 @@
+---
+'@sap-ux/abap-deploy-config-inquirer': patch
+---
+
+Handle validator method not being invoked for confirm field

packages/abap-deploy-config-inquirer/src/prompts/conditions.ts

@@ -1,6 +1,5 @@
 import { isAppStudio } from '@sap-ux/btp-utils';
 import { PromptState } from './prompt-state';
-import { clientDoesNotExistOrInvalid } from '../validator-utils';
 import { findBackendSystemByUrl, initTransportConfig } from '../utils';
 import { handleTransportConfigError } from '../error-handler';
 import { t } from '../i18n';
@@ -53,52 +52,45 @@ export function showScpQuestion(previousAnswers: AbapDeployConfigAnswersInternal
 }
 
 /**
- * Client condition to determine if the client question should be shown.
+ * Client condition to determine if the client question should be shown. Validates the SCP confirmation and project configuration properties.
  *
- * @param isS4HanaCloudSystem - is S/4 HANA Cloud system
+ * @param scp - is SCP system
  * @returns boolean
  */
-function showClientCondition(isS4HanaCloudSystem?: boolean): boolean {
+function showClientCondition(scp?: boolean): boolean {
     return Boolean(
-        !isAppStudio() &&
-            clientDoesNotExistOrInvalid(PromptState.abapDeployConfig.client) &&
-            !PromptState.abapDeployConfig.scp &&
-            !isS4HanaCloudSystem
+        !isAppStudio() && !PromptState.abapDeployConfig?.isS4HC && !scp && !PromptState.abapDeployConfig?.scp
     );
 }
 
 /**
  * Determines if the client choice question should be shown.
  *
+ * @param previousAnswers - previous answers
  * @param client - client
- * @param isS4HanaCloudSystem - is S/4 HANA Cloud system
  * @returns boolean
  */
-export function showClientChoiceQuestion(client?: string, isS4HanaCloudSystem?: boolean): boolean {
+export function showClientChoiceQuestion(previousAnswers?: AbapDeployConfigAnswersInternal, client?: string): boolean {
     if (PromptState.isYUI || !client) {
         return false;
     }
-
-    return showClientCondition(isS4HanaCloudSystem);
+    return showClientCondition(previousAnswers?.scp) && previousAnswers?.targetSystem === TargetSystemType.Url;
 }
 
 /**
- * Determines if the client question should be shown.
- * Note: In some instances, when a yaml conf is parsed, double quoted properties i.e. client: "100" are saved as a number instead of a string.
+ * Determines if the client question should be shown under very specific conditions.
  *
- * @param clientChoice - client choice from previous answers
- * @param client - client
- * @param isS4HanaCloudSystem - is S/4 HANA Cloud system
+ * @param previousAnswers - previous answers
  * @returns boolean
  */
-export function showClientQuestion(clientChoice?: string, client?: string, isS4HanaCloudSystem?: boolean): boolean {
-    const clientCondition = showClientCondition(isS4HanaCloudSystem);
-
-    if (clientCondition && client) {
-        PromptState.abapDeployConfig.client = String(client);
-    }
-    const showOnCli = clientChoice === ClientChoiceValue.New || !client;
-    return !PromptState.isYUI ? showOnCli && clientCondition : clientCondition;
+export function showClientQuestion(previousAnswers?: AbapDeployConfigAnswersInternal): boolean {
+    const clientCondition = showClientCondition(previousAnswers?.scp);
+    const isTargetUrl = previousAnswers?.targetSystem === TargetSystemType.Url;
+    const showCli = !PromptState.isYUI
+        ? previousAnswers?.clientChoice === ClientChoiceValue.New || isTargetUrl
+        : isTargetUrl;
+    const showYui = PromptState.isYUI ? isTargetUrl : false;
+    return (showYui && clientCondition) || (showCli && clientCondition);
 }
 
 /**

packages/abap-deploy-config-inquirer/src/prompts/questions/abap-target.ts

@@ -3,7 +3,6 @@ import {
     validateClientChoiceQuestion,
     validateClient,
     validateDestinationQuestion,
-    validateScpQuestion,
     validateTargetSystem,
     validateTargetSystemUrlCli,
     validateUrl,
@@ -26,6 +25,7 @@ import {
 } from '../../types';
 import type { InputQuestion, ListQuestion, ConfirmQuestion, YUIQuestion } from '@sap-ux/inquirer-common';
 import type { Question } from 'inquirer';
+import { TargetSystemType } from '../../types';
 
 /**
  * Returns the destination prompt.
@@ -158,18 +158,31 @@ function getUrlPrompt(
  * @param backendTarget - backend target
  * @returns confirm question for scp
  */
-function getScpPrompt(backendTarget?: BackendTarget): Question<AbapDeployConfigAnswersInternal> {
-    return {
-        when: (previousAnswers: AbapDeployConfigAnswersInternal): boolean => showScpQuestion(previousAnswers),
-        type: 'confirm',
-        name: promptNames.scp,
-        message: t('prompts.target.scp.message'),
-        guiOptions: {
-            breadcrumb: t('prompts.target.scp.breadcrumb')
+function getScpPrompt(backendTarget?: BackendTarget): Question<AbapDeployConfigAnswersInternal>[] {
+    const prompts: (ConfirmQuestion<AbapDeployConfigAnswersInternal> | Question)[] = [
+        {
+            when: (previousAnswers: AbapDeployConfigAnswersInternal): boolean => showScpQuestion(previousAnswers),
+            type: 'confirm',
+            name: promptNames.scp,
+            message: t('prompts.target.scp.message'),
+            guiOptions: {
+                breadcrumb: t('prompts.target.scp.breadcrumb')
+            },
+            default: (): boolean | undefined => backendTarget?.abapTarget?.scp
+        }
+    ];
+    // Setter prompt to ensure the state for both CLI and YUI is updated
+    prompts.push({
+        when: (answers: AbapDeployConfigAnswersInternal): boolean => {
+            const scpChoice = answers[promptNames.scp];
+            const targetChoice = answers[promptNames.targetSystem];
+            // scpChoice by default is true so only update state if target system is a URL
+            PromptState.abapDeployConfig.scp = !!(targetChoice === TargetSystemType.Url && scpChoice);
+            return false;
         },
-        default: (): boolean | undefined => backendTarget?.abapTarget?.scp,
-        validate: (scp: boolean): boolean | string => validateScpQuestion(scp)
-    } as ConfirmQuestion<AbapDeployConfigAnswersInternal>;
+        name: promptNames.scpSetter
+    } as Question);
+    return prompts;
 }
 
 /**
@@ -183,8 +196,8 @@ function getClientChoicePrompt(
 ): (YUIQuestion<AbapDeployConfigAnswersInternal> | Question)[] {
     const prompts: (ListQuestion<AbapDeployConfigAnswersInternal> | Question)[] = [
         {
-            when: (): boolean =>
-                showClientChoiceQuestion(backendTarget?.abapTarget?.client, PromptState.abapDeployConfig?.isS4HC),
+            when: (previousAnswers: AbapDeployConfigAnswersInternal): boolean =>
+                showClientChoiceQuestion(previousAnswers, backendTarget?.abapTarget?.client),
             type: 'list',
             name: promptNames.clientChoice,
             message: t('prompts.target.clientChoice.message'),
@@ -217,25 +230,20 @@ function getClientChoicePrompt(
 /**
  * Returns the client prompt.
  *
- * @param backendTarget - backend target
  * @returns input question for client
  */
-function getClientPrompt(backendTarget?: BackendTarget): Question<AbapDeployConfigAnswersInternal> {
+function getClientPrompt(): Question<AbapDeployConfigAnswersInternal> {
     return {
         when: (previousAnswers: AbapDeployConfigAnswersInternal): boolean => {
-            return showClientQuestion(
-                previousAnswers.clientChoice,
-                backendTarget?.abapTarget?.client,
-                PromptState.abapDeployConfig?.isS4HC
-            );
+            return showClientQuestion(previousAnswers);
         },
         type: 'input',
         name: promptNames.client,
         message: t('prompts.target.client.message'),
         guiOptions: {
             breadcrumb: t('prompts.target.client.breadcrumb')
         },
-        default: (): string | undefined => backendTarget?.abapTarget?.client,
+        default: (): string | undefined => PromptState.abapDeployConfig?.client, // Already set from previous step, if passed in from yaml config for example
         filter: (input: string): string => input?.trim(),
         validate: (client: string): boolean | string => validateClient(client)
     } as InputQuestion<AbapDeployConfigAnswersInternal>;
@@ -256,8 +264,8 @@ export async function getAbapTargetPrompts(
         ...getDestinationPrompt(abapSystemChoices, destinations, options.backendTarget),
         ...getTargetSystemPrompt(abapSystemChoices),
         getUrlPrompt(destinations, options.backendTarget),
-        getScpPrompt(options.backendTarget),
+        ...getScpPrompt(options.backendTarget),
         ...getClientChoicePrompt(options.backendTarget),
-        getClientPrompt(options.backendTarget)
+        getClientPrompt()
     ];
 }

packages/abap-deploy-config-inquirer/src/prompts/validators.ts

@@ -158,17 +158,6 @@ export function validateTargetSystemUrlCli(targetSystem?: string, choices?: Abap
     }
 }
 
-/**
- * Validates the SCP question and sets the SCP in the prompt state.
- *
- * @param input - if confirm was selected
- * @returns boolean
- */
-export function validateScpQuestion(input: boolean): boolean {
-    PromptState.abapDeployConfig.scp = input;
-    return true;
-}
-
 /**
  * Validates and updates the client property in the state.
  *

packages/abap-deploy-config-inquirer/src/types.ts

@@ -59,6 +59,7 @@ export enum promptNames {
     targetSystemCliSetter = 'targetSystemCliSetter',
     url = 'url',
     scp = 'scp',
+    scpSetter = 'scpSetter',
     clientChoice = 'clientChoice',
     clientChoiceCliSetter = 'clientChoiceCliSetter',
     client = 'client',

packages/abap-deploy-config-inquirer/src/validator-utils.ts

@@ -43,16 +43,6 @@ export function isValidClient(client: string): boolean {
     return !!regex.exec(client);
 }
 
-/**
- * Returns true if the input does not exist, or if it exists but it is invalid.
- *
- * @param client - input string
- * @returns boolean
- */
-export function clientDoesNotExistOrInvalid(client?: string): boolean {
-    return Boolean(!client || (client && !isValidClient(client)));
-}
-
 /**
  * Returns the list of packages for the given input.
  *

packages/abap-deploy-config-inquirer/test/index.test.ts

@@ -16,7 +16,7 @@ describe('index', () => {
         });
         const prompts = await getPrompts({}, undefined, false);
         expect(prompts.answers).toBeDefined();
-        expect(prompts.prompts.length).toBe(23);
+        expect(prompts.prompts.length).toBe(24);
     });
 
     it('should prompt with inquirer adapter', async () => {

packages/abap-deploy-config-inquirer/test/prompts/conditions.test.ts

@@ -1,5 +1,11 @@
 import { isAppStudio } from '@sap-ux/btp-utils';
-import { ClientChoiceValue, PackageInputChoices, TransportChoices, TransportConfig } from '../../src/types';
+import {
+    ClientChoiceValue,
+    PackageInputChoices,
+    TargetSystemType,
+    TransportChoices,
+    TransportConfig
+} from '../../src/types';
 import {
     defaultOrShowManualPackageQuestion,
     defaultOrShowManualTransportQuestion,
@@ -70,25 +76,93 @@ describe('Test abap deploy config inquirer conditions', () => {
     test('should show client choice question', () => {
         mockIsAppStudio.mockReturnValueOnce(false);
         PromptState.isYUI = false;
-        expect(showClientChoiceQuestion('100', false)).toBe(true);
+        PromptState.abapDeployConfig.isS4HC = false;
+        expect(
+            showClientChoiceQuestion({ scp: false, targetSystem: TargetSystemType.Url, url: '', package: '' }, '100')
+        ).toBe(true);
+        PromptState.resetAbapDeployConfig();
+        // Should not show client choice question if SCP is enabled
+        PromptState.abapDeployConfig.isS4HC = false;
+        PromptState.abapDeployConfig.scp = true;
+        expect(
+            showClientChoiceQuestion({ scp: false, targetSystem: TargetSystemType.Url, url: '', package: '' }, '100')
+        ).toBe(false);
+        PromptState.resetAbapDeployConfig();
+        // Should not show client choice question if target system is not a URL
+        PromptState.abapDeployConfig.isS4HC = false;
+        PromptState.abapDeployConfig.scp = true;
+        expect(showClientChoiceQuestion({ scp: true, url: '', package: '' }, '100')).toBe(false);
+        PromptState.resetAbapDeployConfig();
     });
 
     test('should not show client choice question', () => {
         mockIsAppStudio.mockReturnValueOnce(false);
         PromptState.isYUI = false;
-        expect(showClientChoiceQuestion(undefined, true)).toBe(false);
+        PromptState.abapDeployConfig.isS4HC = true;
+        expect(
+            showClientChoiceQuestion({ scp: true, targetSystem: TargetSystemType.Url, url: '', package: '' }, undefined)
+        ).toBe(false);
+        PromptState.resetAbapDeployConfig();
     });
 
-    test('should show client question', () => {
-        PromptState.isYUI = true;
-        mockIsAppStudio.mockReturnValueOnce(false);
-        expect(showClientQuestion(undefined, undefined, false)).toBe(true);
-    });
+    it.each([
+        { isYui: true, scpEnabled: false, scpDisabled: true, clientChoice: undefined },
+        {
+            isYui: false,
+            scpEnabled: false,
+            scpDisabled: true,
+            clientChoice: ClientChoiceValue.New
+        },
+        { isYui: false, scpEnabled: false, scpDisabled: true, clientChoice: undefined }
+    ])(
+        'Validate showClientQuestion for different environments isYui: $isYui, clientChoice: $clientChoice',
+        ({ isYui, scpEnabled, scpDisabled, clientChoice }) => {
+            PromptState.resetAbapDeployConfig();
+            PromptState.isYUI = isYui;
+            mockIsAppStudio.mockReturnValueOnce(false);
+            // Validate client question if SCP is enabled
+            PromptState.abapDeployConfig.isS4HC = false;
+            expect(showClientQuestion({ scp: true, targetSystem: TargetSystemType.Url, url: '', package: '' })).toBe(
+                scpEnabled
+            );
+            PromptState.resetAbapDeployConfig();
+            // Validate client question if SCP is disabled
+            PromptState.abapDeployConfig.client = '100';
+            PromptState.abapDeployConfig.isS4HC = false;
+            expect(
+                showClientQuestion({
+                    scp: false,
+                    clientChoice,
+                    targetSystem: TargetSystemType.Url,
+                    url: '',
+                    package: ''
+                })
+            ).toBe(scpDisabled);
+            // Should always be shown if target system is not SCP and is URL for both CLI and YUI
+            expect(
+                showClientQuestion({
+                    scp: false,
+                    clientChoice: ClientChoiceValue.Blank,
+                    targetSystem: TargetSystemType.Url,
+                    url: '',
+                    package: ''
+                })
+            ).toBe(true);
+            PromptState.resetAbapDeployConfig();
+        }
+    );
 
     test('should show client question (CLI)', () => {
         PromptState.isYUI = false;
         mockIsAppStudio.mockReturnValue(false);
-        expect(showClientQuestion(ClientChoiceValue.New, undefined, false)).toBe(true);
+        expect(
+            showClientQuestion({
+                clientChoice: ClientChoiceValue.New,
+                targetSystem: TargetSystemType.Url,
+                url: '',
+                package: ''
+            })
+        ).toBe(true);
     });
 
     test('should show username question', async () => {