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

Search Criteria Support in the Web UI #964

Closed
wants to merge 8 commits into from

Conversation

JazzyMichael
Copy link
Contributor

@JazzyMichael JazzyMichael commented Mar 7, 2022

Resolves: 492

Adds support for Search Criteria and nested parameter fields for rules.

This recording demonstrates the current application without the "search" option in the UI. Since search functionality is implemented in the back-end but not the front-end, navigating to a rule that uses search crashes the application.

Search-Feature-Before.mov

This next recording demonstrates the changes: Added search criteria input fields, as well as not crashing the app when selecting a rule using search:

Search-Feature-Recording.mov

@JazzyMichael JazzyMichael changed the title search criteria in the UI Search Criteria Support in the Web UI Mar 7, 2022
@pradnya-orchestral
Copy link
Contributor

pradnya-orchestral commented Mar 24, 2022

@Jappzy , I have tested your changes, if we are selecting search from dropdown list, then condition is mandatory fields, so there should be some parameter field need to send from UI while saving data in PUT API to backend, so that we can get something for condition parameter in GET API. Here, need to check this because we are not getting anything for condition parameter in Get API so that on UI it is showing empty, I have made red square to highlight in following screenshot. Can you please check this?

condition screen

@JazzyMichael
Copy link
Contributor Author

@pradnya-orchestral Could you re-test? I added a screen recording and fixed the default values getting included in the PUT request.

@mickmcgrath13
Copy link
Contributor

@pradnya-orchestral are you able to re-review?

@Jappzy once things are good, we'll need to merge master -> this branch

@CLAassistant
Copy link

CLAassistant commented May 11, 2022

CLA assistant check
All committers have signed the CLA.

@arm4b arm4b added the RFR label Jun 9, 2022
@arm4b arm4b modified the milestone: 3.8.0 Jun 9, 2022
@nzlosh
Copy link
Contributor

nzlosh commented Sep 14, 2022

@pradnya-orchestral have you been able to test since @Jappzy applied the fixes for the problem you mentioned?

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

From reading and based on the screen recording, this looks good to me. Without any more experienced JS reviewers, I'm inclined to merge this. If there are any issues, they should be found before release.

@cognifloyd cognifloyd added this to the 3.8.0 milestone Sep 19, 2022
@arm4b arm4b self-requested a review September 20, 2022 21:48
@arm4b arm4b self-assigned this Sep 20, 2022
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

The feature implementation is missing the main point of rule with search criteria which could include multiple patterns.

And so this rule fails to render, despite being properly registered in st2:

---
name: test_search2_err
pack: st2
description: "Test search rule in web ui with multiple criteria patterns"
enabled: true

trigger:
  type: "core.st2.webhook"
  parameters:
    url: "webhook-alert/handle-alert"

criteria:
  trigger.body.alert:
    type: "search"
    condition: any
    pattern:
      item.data:
        type: "equals"
        pattern: "foo"
      item.time: # additional pattern added
        type: "equals"
        pattern: "12:00"

action:
  ref: "core.echo"
  parameters:
    message: "{{ trigger.body }}"

https://docs.stackstorm.com/rules.html#advanced-comparison highlights more examples

Comment on lines +57 to 60
const searchConditions = {
'any': 'Any',
'all': 'All',
};
Copy link
Member

Choose a reason for hiding this comment

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

There are more search conditions supported, according to https://docs.stackstorm.com/rules.html#advanced-comparison

Suggested change
const searchConditions = {
'any': 'Any',
'all': 'All',
};
const searchConditions = {
'any': 'Any',
'all': 'All',
'any2any': 'Any2Any',
'all2any': 'All2Any',
};

Comment on lines +382 to +383
pattern: pattern['item.data'].pattern,
type: pattern['item.data'].type,
Copy link
Member

@arm4b arm4b Sep 21, 2022

Choose a reason for hiding this comment

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

Looks like there was a misunderstanding in implementation and so this PR is always expecting to have item.data in the rule pattern.

I now see this is coming from the example:
https://github.com/bitovi-stackstorm-exchange/st2-ui-workflow-tests/blob/main/rules/validate-alert.yaml#L16

But based on https://github.com/bitovi-stackstorm-exchange/st2-ui-workflow-tests/blob/4f576228aad17652ceb47b1f444f2624f4c116c8/rules/validate-alert.yaml#L21

instead of item.data could be item.foo, item.bar and so on, as that refers to the dictionary item.

And so based on implementation here, this works:

---
name: test_search1_ok
pack: st2
enabled: true

trigger:
  type: "core.st2.webhook"
  parameters:
    url: "webhook-alert/handle-alert"

criteria:
  trigger.body.alert:
    type: "search"
    condition: any
    pattern:
      item.data: # works
        type: "equals"
        pattern: "foo"

action:
  ref: "core.echo"
  parameters:
    message: "{{ trigger.body }}"

While this rule doesn't (it should):

---
name: test_search1_err
pack: st2
enabled: true

trigger:
  type: "core.st2.webhook"
  parameters:
    url: "webhook-alert/handle-alert"

criteria:
  trigger.body.alert:
    type: "search"
    condition: any
    pattern:
      item.time: # changed, it's just a dict item
        type: "equals"
        pattern: "foo"

action:
  ref: "core.echo"
  parameters:
    message: "{{ trigger.body }}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants