-
Notifications
You must be signed in to change notification settings - Fork 614
NMS-19161: Integrate api for creating/updating the source #8188
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.
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
NMS-19161: Integrate api for creating/updating the source #8188
Conversation
…-19161-create-source-api-integration
|
Waiting for this PR to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR integrates an API for creating and updating event configuration sources. The main change corrects a typo in the store method name from refreshSourcesfilters to refreshSourcesFilters and implements a new dialog component for creating event configuration sources with proper form validation, error handling, and success state management.
Key changes:
- Fixed method name typo:
refreshSourcesfilters→refreshSourcesFilters - Added
addEventConfigSourceservice function with HTTP error handling (400, 409, 500 status codes) - Enhanced CreateEventConfigurationDialog with vendor and description fields, success state UI, and navigation to created sources
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/stores/eventConfigStore.ts | Corrected method name typo in store |
| ui/src/services/eventConfigService.ts | Added new service function for creating event configuration sources |
| ui/src/components/EventConfiguration/Dialog/CreateEventConfigurationDialog.vue | Enhanced dialog with additional fields, success state, and navigation |
| ui/src/components/EventConfiguration/EventConfigSourceTable.vue | Updated method call to use corrected name |
| ui/src/components/EventConfiguration/Dialog/DeleteEventConfigSourceDialog.vue | Updated method call to use corrected name |
| ui/tests/stores/eventConfigStore.test.ts | Updated test to use corrected method name |
| ui/tests/components/EventConfiguration/EventConfigSourceTable.test.ts | Updated test to use corrected method name |
| ui/tests/components/EventConfiguration/Dialog/CreateEventConfigurationDialog.test.ts | Added comprehensive test coverage for new dialog functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { FeatherDialog } from '@featherds/dialog' | ||
| import { FeatherInput } from '@featherds/input' | ||
| import { FeatherTextarea } from '@featherds/textarea' | ||
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing import for useRouter and ref/Ref/computed from Vue. The component references useRouter(), ref(), Ref, and computed() but these are not imported in the script setup block.
| import { ref, Ref, computed } from 'vue' | |
| import { useRouter } from 'vue-router' |
| const mockSuccessResponse = (id: number, name: string, fileOrder: number = 0) => ({ | ||
| id, | ||
| name, | ||
| fileOrder, | ||
| status: 201 as const | ||
| }) |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper function mockSuccessResponse duplicates the response structure defined in the service. Consider extracting this to a shared test utility to maintain consistency across test files if this pattern is used elsewhere.
…-19161-create-source-api-integration
…-19161-create-source-api-integration
…-19161-create-source-api-integration
…-19161-create-source-api-integration
synqotik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments.
ui/tests/components/EventConfiguration/Dialog/CreateEventConfigurationDialog.test.ts
Outdated
Show resolved
Hide resolved
| ;(wrapper.vm as any).vendor = 'TestVendor' | ||
| ;(addEventConfigSource as any).mockResolvedValue(mockSuccessResponse(123, 'TestConfig', 0)) | ||
|
|
||
| await wrapper.vm.$nextTick() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 lines are called a bunch of times. Consider moving them to a function called clickCreateButton, then you can call:
await clickCreateButton(wrapper)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing setWrapperRefs. This also needs to be fixed (clickCreateButton).
synqotik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment, please add something like clickCreateButton in the tests to cut down on duplicate code.
A note, in the future, can use jest parameterized tests for cases where we test a lot of similar things.
| ;(wrapper.vm as any).vendor = 'TestVendor' | ||
| ;(addEventConfigSource as any).mockResolvedValue(mockSuccessResponse(123, 'TestConfig', 0)) | ||
|
|
||
| await wrapper.vm.$nextTick() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing setWrapperRefs. This also needs to be fixed (clickCreateButton).
synqotik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
External References