Skip to content
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

Validate json and don't allow null parameter values when running workflow #866

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Sep 20, 2024

<!-- ELLIPSIS_HIDDEN -->
🚀 This description was created by a5e4c80

fix: add null check validation for workflow parameters and update import paths

Summary:

Add null check validation for required workflow parameters and update import paths in RunWorkflowForm.tsx and WorkflowRunParameters.tsx.

Key points:

  • Validation: Add null check validation in RunWorkflowForm to ensure required fields are not null.
  • Imports: Update WorkflowParameter import path from @/api/types to ./types/workflowTypes in RunWorkflowForm.tsx and WorkflowRunParameters.tsx.

Generated with ❤️ by ellipsis.dev

…src/'

<!-- ELLIPSIS_HIDDEN -->

| 🚀 | This description was created by 31e6dddd6a9efbec766b7f08e652e4fcaca36a04  |
|--------|--------|

fix: add null check validation for required workflow parameters and update import paths

### Summary:
Added null check validation for required fields in `RunWorkflowForm` and updated import paths for `WorkflowParameter`.

**Key points**:
- **Validation**:
  - Added null check validation in `RunWorkflowForm` to ensure required fields are not empty.
- **Imports**:
  - Moved `WorkflowParameter` import from `@/api/types` to `./types/workflowTypes` in `RunWorkflowForm.tsx` and `WorkflowRunParameters.tsx`.

----
Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev)

<!-- ELLIPSIS_HIDDEN -->
@ykeremy ykeremy added the sync label Sep 20, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to a5e4c80 in 25 seconds

More details
  • Looked at 47 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:155
  • Draft comment:
    Consider checking for undefined as well to ensure all required fields are validated properly.
                        if (value === null || value === undefined) {
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about a change made in the diff, specifically the validation logic for form fields. Checking for 'undefined' could be relevant if the form field can be 'undefined', which is common in JavaScript. The suggestion is actionable and clear, as it directly relates to form validation logic.
    The current code might already handle 'undefined' values elsewhere, or 'undefined' might not be a valid state for these fields. Without more context, it's hard to be certain if the change is necessary.
    Given that 'undefined' is a common state for uninitialized variables in JavaScript, the suggestion to check for it in validation logic is reasonable and likely beneficial.
    Keep the comment as it suggests a reasonable improvement to the validation logic by checking for 'undefined', which is a common scenario in JavaScript.

Workflow ID: wflow_HH5BbXsm4fgiQHBv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on a5e4c80 in 31 seconds

More details
  • Looked at 47 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:155
  • Draft comment:
    Consider checking for empty strings as well, as they can also represent missing values in forms.
                        if (value === null || value === '') {
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is relevant to the change made in the diff, which added a validation rule for null values. Checking for empty strings is a common practice in form validation to ensure that fields are not left blank. This suggestion seems actionable and could improve the robustness of the validation logic.
    The current validation logic might already handle empty strings elsewhere, or the application might have specific requirements that allow empty strings. Without full context, it's hard to be certain that this change is necessary.
    Given the context provided, it seems reasonable to assume that checking for empty strings would be a beneficial addition to the validation logic, unless there is a specific reason not to do so.
    The comment is relevant and suggests an actionable improvement to the validation logic. It should be kept as it addresses a potential issue with form validation.

Workflow ID: wflow_UcOSnR9Vdt3A5CXi


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@msalihaltun msalihaltun merged commit 0beb929 into main Sep 20, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/run-workflow-required-parameters branch September 20, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants