Skip to content
This repository has been archived by the owner on Jul 12, 2022. It is now read-only.

Adding conditional steps to config prompt #454

Merged

Conversation

henriquemoraeszup
Copy link
Contributor

@henriquemoraeszup henriquemoraeszup commented Aug 21, 2020

Description

As requested on issue #390, this PR allows the developer to specify optional inputs according to previous user inputs. The user needs to create a key called condition on an optional step and provide a variable reference from any previous variable defined, an operator for comparison (==, !=, >, >=, <, <=), and a value to compare. If the condition exists and fails, the current step is not shown.

This is a simple implementation some of the points taken in account are explained below:

  • The comparison operators are implemented in a case like manner. It is not very elegant. The better approach would be to evaluate the expression, but it was not chosen because:
    1. Go does not come with a native eval function and it would require to add a dependency to the project for a simple task
    2. Since many times the user input is free we could risk him adding harmful code as a value and executing it on an eval command or letting the author add malicious code on the operator variable
  • This approach only attends simple expressions between a single variable and a value. Complex multi-variable expressions are not supported for the sake of avoiding overengineering. This should cover most of the cases, I think it is better to wait for the community to demand more complex interactions before we actually implement them.
  • There are discussions to create tree-like decisions to prompt the user. This implementation does not rule that out, we can still implement a decision tree with conditional nodes, so forward-compatibility is accounted for.
  • In case the user uses a wrong operator. An error is thrown
  • In case the user defines a non existing variable name. An error is thrown

Suggestions are welcome!

How to verify it

Create a condition key on config.json of a formula like on this example

{
      "cache": {
        "active": true,
        "newLabel": "Type new value. ",
        "qty": 6
      },
      "label": "Type : ",
      "name": "sample_text",
      "condition": {
          "variable": "sample_list",
          "operator": "==",
          "value": "in_list1"
      },
      "type": "text"
    },

Run the formula and fail the condition to have this prompt not shown

Description for the changelog

Adding conditional steps to config prompt

Aug-20-2020 21-27-16

Signed-off-by: Henrique Moraes <henrique.moraes@zup.com.br>

Adding conditional steps to config prompt
@henriquemoraeszup henriquemoraeszup force-pushed the feature/conditional-variables branch from 170f69f to 43c7f5e Compare August 21, 2020 01:08
@henriquemoraeszup henriquemoraeszup linked an issue Aug 21, 2020 that may be closed by this pull request
@henriquemoraeszup henriquemoraeszup self-assigned this Aug 21, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2020

Codecov Report

Merging #454 into master will increase coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
+ Coverage   66.75%   67.20%   +0.45%     
==========================================
  Files          80       80              
  Lines        2755     2784      +29     
==========================================
+ Hits         1839     1871      +32     
+ Misses        735      733       -2     
+ Partials      181      180       -1     
Impacted Files Coverage Δ
pkg/formula/formula.go 90.90% <ø> (ø)
pkg/formula/runner/inputs.go 98.74% <100.00%> (+2.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9e11d7...069d543. Read the comment docs.

@henriquemoraeszup henriquemoraeszup added the 🚧 WIP Work in Progress label Aug 21, 2020
Signed-off-by: Henrique Moraes <henrique.moraes@zup.com.br>
@henriquemoraeszup henriquemoraeszup added ✔️ ready-for-review ready for review ✨ feature Suggest a new feature or enhancement to the Ritchie project and removed 🚧 WIP Work in Progress labels Aug 21, 2020
@kaduartur kaduartur added this to the 2.0.5 milestone Aug 21, 2020
@brunasilvazup brunasilvazup self-requested a review August 26, 2020 18:20
@brunasilvazup brunasilvazup added the 🆘 help wanted Extra attention is needed label Aug 26, 2020
@kaduartur kaduartur removed this from the 2.0.5 milestone Aug 27, 2020
Copy link
Contributor

@kaduartur kaduartur left a comment

Choose a reason for hiding this comment

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

Good job 🥇

pkg/formula/runner/inputs.go Outdated Show resolved Hide resolved
pkg/formula/runner/inputs.go Outdated Show resolved Hide resolved
@kaduartur kaduartur added waiting reply Waiting for an answer to a comment and removed 🆘 help wanted Extra attention is needed labels Aug 27, 2020
Signed-off-by: Henrique Moraes <henrique.moraes@zup.com.br>
Signed-off-by: Henrique Moraes <henrique.moraes@zup.com.br>
Signed-off-by: Henrique Moraes <henrique.moraes@zup.com.br>
@henriquemoraeszup henriquemoraeszup removed the waiting reply Waiting for an answer to a comment label Aug 28, 2020
@rasouza
Copy link

rasouza commented Aug 28, 2020

Amazing job indeed 👏

One further improvement suggestion: we could implement a parser to get rid of the condition object and let it be written as simples as

...
"condition": "sample_list == in_list1"
...

This implementation of yours is great already, though. Thanks!

@kaduartur
Copy link
Contributor

/merge qa

@ritchie-bot
Copy link
Contributor

ritchie-bot bot commented Aug 28, 2020

👌 Merged branch feature/conditional-variables into qa

@kaduartur kaduartur removed the ✔️ ready-for-review ready for review label Aug 28, 2020
@kaduartur kaduartur merged commit c1b335b into ZupIT:master Aug 28, 2020
@henriquemoraeszup
Copy link
Contributor Author

henriquemoraeszup commented Aug 28, 2020

Actually the parser is a good idea, it would allow us to cover the point above on multi-variables as well. We can leave as an improvement =]

Hashicorp's bparser could take us a long way
https://github.com/hashicorp/go-bexpr

Amazing job indeed 👏

One further improvement suggestion: we could implement a parser to get rid of the condition object and let it be written as simples as

...
"condition": "sample_list == in_list1"
...

This implementation of yours is great already, though. Thanks!

@brunasilvazup brunasilvazup added this to the 2.0.5 milestone Aug 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✨ feature Suggest a new feature or enhancement to the Ritchie project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional options on config.json
6 participants