Skip to content

feat: Show proper error when environment variables are not valued [INS-3641]#7227

Merged
CurryYangxx merged 12 commits intodevelopfrom
feat/env-variables-proper-error
Apr 10, 2024
Merged

feat: Show proper error when environment variables are not valued [INS-3641]#7227
CurryYangxx merged 12 commits intodevelopfrom
feat/env-variables-proper-error

Conversation

@CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented Apr 2, 2024

Related To INS-3641

This change is to give appropriate prompts when environment variables are missing, rather than directly displaying error messages.

Changes:

  • After clicking the send button, if the environment variable is missing, a modal(include the missing field) will show and let the user choose whether to continue the request or cancel.
  • If user choose continue,we need to ignore environment missing error in the request logic.
image

@CurryYangxx CurryYangxx requested a review from a team April 2, 2024 02:15
@CurryYangxx CurryYangxx requested review from a team and filfreire April 2, 2024 08:51
@jackkav
Copy link
Contributor

jackkav commented Apr 3, 2024

I'd suggest extracting the common modal to a different PR, its unclear from the description why its required in this scope.
also ignoreOnUndefined and throwOnUndefined are conflicting, would be better to use throwOnUndefined, since it is my understanding that we would now like to be able to send request with missing variables. Can explain more on slack.

@subnetmarco
Copy link
Member

"Execute anyways" (first letter upper case).

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 8, 2024

I'd suggest extracting the common modal to a different PR, its unclear from the description why its required in this scope. also ignoreOnUndefined and throwOnUndefined are conflicting, would be better to use throwOnUndefined, since it is my understanding that we would now like to be able to send request with missing variables. Can explain more on slack.

@jackkav The common modal code has been extracted to #7250

Copy link
Contributor

@jackkav jackkav 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 so far, note: nunjucks will remove the template when ignoring it. This is not easy to change, we would need to replace nunjucks, or send unrendered text rather than a partial render with the template removed.

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 8, 2024

This PR depends on #7250 ,please not merge until #7250 merged

@CurryYangxx
Copy link
Member Author

CurryYangxx commented Apr 9, 2024

looks good so far, note: nunjucks will remove the template when ignoring it. This is not easy to change, we would need to replace nunjucks, or send unrendered text rather than a partial render with the template removed.

@jackkav It is not easy to change.Now when user click the continue button, we will set the nunjucks throwOnUndefined config as true, so we can't perceived error and don't know if we need to use unrendered text.
There are two ways to solve.

  1. Keep the throwOnUndefined config as true, we handle error by ourselves.When there is a error, we use the origin text.But there is a problem, if user use multiple vars in one line, the result returned by the following line of code does not meet our expectations

input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: '{{ var1 }}{{ var2 }}'

  1. replace nunjucks

I think that finding another template engine to replace Nunjucks would be better if necessary.

@CurryYangxx CurryYangxx requested a review from jackkav April 9, 2024 07:30
@jackkav
Copy link
Contributor

jackkav commented Apr 9, 2024

Understood perhaps we can reconsider point 1 and 2 in the future. To be clear our option 1 trade off is as follows.
When missing var2, we will show an error but if continue is pressed we do either:
Partial render and exclusion
input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: 'aa'
Unrendered
input: ( '{{ var1 }}{{ var2 }}', { var1: 'aa' } ) output: '{{ var1 }}{{ var2 }}'

I find either satisfactory for this PR scope however for users coming from postman, I think Unrendered would be less surprising. I'll leave it to your judgement whether to make this change in a future PR.

jackkav
jackkav previously approved these changes Apr 9, 2024
@CurryYangxx CurryYangxx requested a review from jackkav April 9, 2024 11:28
@CurryYangxx CurryYangxx enabled auto-merge (squash) April 10, 2024 02:09
@CurryYangxx CurryYangxx merged commit 75c6a94 into develop Apr 10, 2024
@CurryYangxx CurryYangxx deleted the feat/env-variables-proper-error branch April 10, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants