feat(create): add interactive prompting and connection checking for system management commands#4949
Open
korotkovao wants to merge 42 commits into
Open
feat(create): add interactive prompting and connection checking for system management commands#4949korotkovao wants to merge 42 commits into
korotkovao wants to merge 42 commits into
Conversation
- Add interactive prompts when flags not provided for add/update/remove system - Add connection verification with --skip-check flag - Add confirmation prompt for remove with --force flag - All existing flag-based usage remains unchanged
- Mock prompts and connection check modules - Update tests to handle interactive flows - Add tests for --skip-check and --force flags - All 27 tests passing
- Add changeset for minor version bump - Update README with new interactive mode examples - Update SKILL.md (auto-generated) - Add MANUAL_TESTING.md to .gitignore
🦋 Changeset detectedLatest commit: c73d6a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
SummaryThe following content is AI-generated and provides a summary of the pull request: DescriptionEnhances the Key changes:
Examples: # Fully interactive
npx @sap-ux/create add system
# Connection check bypass
npx @sap-ux/create add system --name "My System" --url https://example.com --skip-check
# Remove with confirmation bypass
npx @sap-ux/create remove system --name "My System" --forceType of change
How have you tested?
Checklist:
PR Bot InformationVersion:
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The PR introduces interactive prompting and connection verification for add system, update system, and remove system commands. Several substantive issues were identified and commented:
- Changeset missing required prefix — the summary line must start with
FEAT:or CI validation will fail. replaceEnvVariablescalled too late inadd system— URL validation and the duplicate-system lookup both run before env-var references are expanded, meaning env-var URLs would fail thenew URL()check and the duplicate check would use the unexpanded string.- CLI option defaults silently suppress interactive prompts —
--type,--auth, and--connection-typeall have default values registered on.option(), soparams.systemType,params.authenticationType, andparams.connectionTypeare neverundefinedeven when the user omits those flags. As a result,promptForSystemConfignever prompts for those fields in interactive mode. ||instead of??for credential fallback inupdate system— falsy values (e.g. empty-string password) would incorrectly fall back to the stored credential rather than using the explicitly supplied empty value.- Test assertion on prompt
type: 'select'in the remove-system confirmation test likely does not match the actual prompt type, making the assertion vacuous. - Mismatch between expected error message and code path in the update-system "no fields" test — the test expects
'At least one field must be selected'but the source code's guard (!Object.keys(patchRecord).length) logs a different message; the two paths need to be reconciled.
- Add system-prompts.ts with interactive prompting functions - Add system-connection.ts with connection verification - Move replaceEnvVariables() before URL validation and duplicate check (fixes env var expansion timing) - Replace || with ?? for nullish coalescing in credential fallback - Use empty strings instead of undefined for clearCredentials - Fix test assertion: change 'select' to 'confirm' for remove prompt
…for system management Address code review feedback from IainSAP (PR #4949): 1. **Add system name uniqueness validation** - Check that system name is unique when adding a new system (both in prompts and when provided via flag) - Check that system name is unique when updating (excluding current system) - Case-insensitive comparison to prevent near-duplicates 2. **Add comprehensive prompt input validations** - validateNonEmpty: Ensures required fields are not empty - validateUrl: Validates URL format using native URL constructor - validateSystemNameUniqueness: Checks name uniqueness for new systems - validateSystemNameUniquenessForUpdate: Checks name uniqueness when updating 3. **Apply validations to prompts** - promptForSystemConfig: name (with uniqueness check) and url validation - promptForSystemIdentifier: url validation - promptForFieldUpdates: name (uniqueness for update), username, and password validation 4. **Validate flags when provided** - Add name uniqueness check in addSystem() when name is provided via --name flag - Existing URL validation continues to work for --url flag All tests pass, lint clean.
…flags When updating a system and providing --name flag, validate that the new name doesn't conflict with other existing systems (excluding the current system being updated). This completes the validation coverage for both add and update operations, whether using prompts or flags.
Extract common uniqueness check logic into a shared helper function to reduce code duplication and improve maintainability. This addresses potential SonarQube code duplication issues. Changes: - Add checkSystemNameUniqueness() helper function with optional exclude parameter - Refactor validateSystemNameUniqueness() to use the helper - Refactor validateSystemNameUniquenessForUpdate() to use the helper All tests pass, lint clean.
Add validation to reject system names that are empty or contain only whitespace when provided via flags. This validation was already present in prompts but was missing from the flag-based flow. Validates in both add and update operations before attempting to save the system.
Add extensive test coverage for new validation functions to meet SonarQube quality gate requirements (80% coverage on new code). New tests cover: - System name uniqueness validation (case-insensitive, add and update flows) - Empty and whitespace-only name rejection - URL format validation - Username and password non-empty validation - Graceful degradation when service is unavailable - Update name validation that excludes current system Coverage for system-prompts.ts improved from 56.57% to 98.68%. All 246 tests passing.
cianmSAP
reviewed
Jul 16, 2026
| return true; | ||
| } catch (error) { | ||
| // If we can't check uniqueness, allow it through | ||
| // (better to let the user proceed than block on a check failure) |
Contributor
There was a problem hiding this comment.
I don't think this is a good idea ...
Based on team feedback, this refactors system name validation to avoid duplication: - Created isSystemNameTaken() in inquirer-common as single source of truth - Reused validateClient() from project-input-validator (was missing before) - Added client field validation to system prompts - Updated all tests with proper mocks for isSystemNameTaken - Both inquirer-common and create packages build successfully - All 51 system-prompts tests pass Benefits: ✅ Single source of truth - isSystemNameTaken() can be used by all packages ✅ Reuses existing validateClient from project-input-validator ✅ No new package created (as requested) ✅ Client validation properly enforced (was missing) Addresses feedback from: - Cian: extract the logic into a function maybe in common utils - Iain: extract the validators to a shared module for reuse
Fixes a bug where client validation was bypassed when the --client flag was used. The validation only ran for interactive prompts but not for flag values. Changes: - Import validateClient from @sap-ux/project-input-validator - Add validation check after URL validation in add system command - Validate that client is either empty or 3 digits (000-999) - Show clear error message for invalid client format - Add unit tests for invalid, valid, and empty client scenarios Manual Testing: ✅ Invalid client '12' is rejected with clear error message ✅ Valid client '100' is accepted ✅ Empty client '' is accepted Note: Unit tests added but cannot run due to pre-existing import issue in test file (unrelated to this fix): @sap-ux/btp-utils missing isAbapODataDestination export
Per Cian's feedback, changed error handling to be more conservative: - If we cannot check for duplicate names (store access fails), block the operation - Previously allowed through on error (graceful degradation) which could create duplicates - Now returns clear error message: 'Unable to check system name uniqueness. Please try again.' Changes: - Removed try-catch from isSystemNameTaken() - let errors propagate - Added try-catch in validation functions with appropriate error message - Updated test: 'should reject on service error' instead of 'should allow through' - Updated JSDoc to document that function throws on store access failure Test Results: ✅ All 51 tests pass ✅ Coverage: 94.66% (above 80% requirement) ✅ 0 lint errors Addresses: #4949 (comment)
cianmSAP
reviewed
Jul 16, 2026
| const service = await getService<BackendSystem, BackendSystemKey>({ | ||
| entityName: 'system' | ||
| }); | ||
| const allSystems = await service.getAll(); |
Contributor
There was a problem hiding this comment.
Needs to specify all 3 connection types
Contributor
Author
There was a problem hiding this comment.
Thanks, good point, updated
Fixed incomplete mocks in 6 test files that were causing module loading errors.
The mocks only provided specific exports but Jest ESM requires all exports to be present.
Issue: When mocking '@sap-ux/btp-utils' and '@sap-ux/project-access', only specific
functions were mocked (e.g., isAppStudio), but the modules export many more functions.
When Jest tried to load the source modules, it failed with 'does not provide an export' errors.
Solution: Import the real module and spread it into the mock, then override specific
functions. This preserves all exports while allowing targeted mocking.
Pattern:
const actualModule = await import('@sap-ux/module');
jest.unstable_mockModule('@sap-ux/module', () => ({
...actualModule,
specificFunction: mockFn
}));
Fixed files:
- test/unit/cli/add/system.test.ts (btp-utils)
- test/unit/cli/update/system.test.ts (btp-utils)
- test/unit/cli/remove/system.test.ts (btp-utils)
- test/unit/cli/list/system.test.ts (btp-utils)
- test/unit/cli/get/system.test.ts (btp-utils)
- test/unit/cli/create-fiori.test.ts (project-access)
Test Results:
✅ Before: 29/33 test suites passing (4 failed to load)
✅ After: 33/33 test suites passing
✅ Before: 216/216 tests (in passing suites)
✅ After: 249/249 tests passing
…ckages Implemented code reuse for system name validation as requested in code review. All packages now use the common isSystemNameTaken function with proper connection type filtering. Changes: - Enhanced isSystemNameTaken to accept optional SystemNameValidationOptions - Defaults to checking all 3 connection types (abap_catalog, odata_service, generic_host) - Supports custom connection type filtering for specific use cases - Supports excludeSystem option for update scenarios Fixed: - create/add/system.ts: Replaced inline duplicate logic with common function - create/update/system.ts: Replaced inline duplicate logic with common function - odata-service-inquirer: Fixed bug (was using exact match, now case-insensitive + trim) Verified: - All 3 connection types properly checked - Case-insensitive matching works - Name trimming works - excludeSystem option works for updates - 11/11 manual CLI tests passed Related: Code review feedback from Cian Morrin
Fixed lint errors by removing unused type imports that were only needed for type annotations but not actually used in the code. Changes: - create/add/system.ts: Remove unused SystemNameValidationOptions import - create/update/system.ts: Remove unused SystemNameValidationOptions import - create/utils/system-prompts.ts: Remove unused SystemNameValidationOptions import - odata-service-inquirer/validators.ts: Remove unused SystemNameValidationOptions import Lint results: - odata-service-inquirer: 0 errors (was 1 error) - create: 0 errors (was 3 errors)
…mmands Fixes SonarQube code smells in packages/create: 1. Reduced cognitive complexity in add/system.ts (17→≤15) - Extract validateSystemConfig() helper for field validation - Extract checkForDuplicates() helper for duplicate checking 2. Reduced cognitive complexity in update/system.ts (27→≤15) - Extract buildPatchFromParams() for CLI flag validation - Extract determinePatch() for update flow control - Extract verifyCredentialsUpdate() for connection verification 3. Fixed exception handling in system-prompts.ts - Added proper error logging with console.error() - Maintained user-friendly error messages for service failures 4. Maintained correct empty string handling - Optional fields (client, username, password) convert "" to undefined - Used ternary pattern instead of nullish coalescing where needed Changes: - packages/create/src/cli/add/system.ts - packages/create/src/cli/update/system.ts - packages/create/src/cli/utils/system-prompts.ts Verification: ✅ Build passes (pnpm build) ✅ Lint passes (0 errors, pre-existing warnings only) ✅ All tests pass (249/249) ✅ Coverage maintained at 93.18% ✅ No breaking changes, full backward compatibility
IainSAP
reviewed
Jul 17, 2026
| const service = await getService<BackendSystem, BackendSystemKey>({ | ||
| entityName: 'system' | ||
| }); | ||
| const allSystems = await service.getAll({ |
Contributor
There was a problem hiding this comment.
Does filtering here make sense , the name should be globally unique for name?
…force global uniqueness - Rename isSystemNameTaken to systemNameExists across all packages for better clarity - Remove connection type filtering to enforce global system name uniqueness - Revert unnecessary one-liner extraction in odata-service-inquirer - Fix lint issues: add @throws type annotation and use nullish coalescing operator This addresses code review feedback to prevent confusing duplicate system names across different connection types. System names must now be globally unique. Files modified: - packages/inquirer-common/src/validators/system-name-validator.ts - packages/inquirer-common/src/index.ts - packages/odata-service-inquirer/src/prompts/datasources/sap-system/validators.ts - packages/create/src/cli/add/system.ts - packages/create/src/cli/update/system.ts - packages/create/src/cli/utils/system-prompts.ts - packages/create/test/unit/cli/utils/system-prompts.test.ts
Replace ternary expressions with nullish coalescing operator (??) while preserving correct behavior for empty strings. Empty strings from prompts are converted to undefined, but explicitly provided empty strings are preserved. This addresses SonarQube code smells (3 minor issues on lines 234, 238, 239) for better code consistency and maintainability. Changes: - client: Use ?? with fallback to || for empty string handling - username: Use ?? with fallback to || for empty string handling - password: Use ?? with fallback to || for empty string handling
…ure feedback Per Iain's code review: - Revert odata-service-inquirer to original implementation (no shared validation) - Remove system-name-validator from inquirer-common (avoiding circular deps with store) - Move validation logic to create package where it's needed for add/update operations - Preserve excludeSystem logic for update scenarios Changes: - Moved system-name-validator from inquirer-common to create/src/cli/utils/ - Reverted odata-service-inquirer to original isSystemNameInUse implementation - Updated all create package imports to use local validation - Removed inquirer-common dependency on @sap-ux/store Note: Per Iain's feedback, connection manager (sap-systems-ext) validation should also be updated to remove connection type filtering and potentially share code. This will be addressed in follow-up work with Cian.
…tion Address code review feedback by consolidating system name uniqueness validation into a single reusable function in the store package. Changes: - Add isSystemNameInUse() function in @sap-ux/store/utils/system-name - Update @sap-ux/create to use centralized validation for add/update - Update @sap-ux/sap-systems-ext to use centralized validation - Remove filtering to ensure global name uniqueness across all systems - Maintain case-insensitive comparison with trimming - Support excluding current system in update scenarios - Revert odata-service-inquirer to keep original implementation Review feedback addressed: - Centralized validation logic (Cian, Iain) - No connection type filtering for global uniqueness (Iain) - No circular dependencies - validated dependency chain (Iain) - Kept odata-service-inquirer unchanged (Iain) Testing: - All packages: 449/449 unit tests pass (100%) - Lint: 0 errors (only pre-existing warnings) - Build: All packages build successfully - Coverage: >80% maintained (create: 90.24% for system-prompts) - VSIX: Built successfully (4.32 MB) BREAKING CHANGE: None - fully backward compatible
IainSAP
reviewed
Jul 17, 2026
| * @returns true if name already exists, false otherwise | ||
| * @throws {Error} if unable to check the store | ||
| */ | ||
| export async function isSystemNameInUse(systemName: string): Promise<boolean> { |
Contributor
There was a problem hiding this comment.
This is different logic to the function its replacing which includes filtering : https://github.com/SAP/open-ux-tools/pull/4949/changes#diff-043e26f3bcffebff45e2bc60cab8b9806e01ab31e21a666f8f8d02be5733730cL40
Contributor
Author
There was a problem hiding this comment.
Which would you prefer @IainSAP ?
Keep the filtering behavior (restore the original logic) or global uniqueness?
IainSAP
reviewed
Jul 17, 2026
| "@sap-ux/logger": "workspace:*", | ||
| "@sap-ux/project-access": "workspace:*", | ||
| "@sap-ux/odata-service-writer": "workspace:*", | ||
| "@sap-ux/store": "workspace:*", |
IainSAP
reviewed
Jul 17, 2026
| "path": "../project-access" | ||
| }, | ||
| { | ||
| "path": "../store" |
Per Iain's review feedback, the store dependency is not needed in inquirer-common package. The validation logic is now centralized in the store package and consumed directly by create and sap-systems-ext. Changes: - Remove @sap-ux/store from package.json dependencies - Remove store from tsconfig.json references This avoids potential circular dependency issues and keeps inquirer-common focused on its core functionality.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#4948
Summary
Enhances the
@sap-ux/createCLI's system management commands (add system,update system,remove system) with interactive prompting and connection verification.Key Changes
add systemandupdate systemnow verify backend connectivity before saving (with--skip-checkflag to bypass)remove systemnow prompts for confirmation (with--forceflag to bypass)Examples
Add System - Interactive mode:
```bash
npx @sap-ux/create add system
Prompts for: name, URL, client, type, auth method, username, serviceKey
Verifies connection before saving
```
Update System - Mixed mode (some flags + prompts):
```bash
npx @sap-ux/create update system --name "My System" --url https://updated.example.com
Prompts only for missing fields
Verifies connection before saving
```
Remove System - With confirmation:
```bash
npx @sap-ux/create remove system --name "My System"
Prompts: "Are you sure you want to remove system 'My System'?"
```
Testing
Changeset
@sap-ux/create