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

refactor: combine add and edit environment modals #2131

Merged
merged 3 commits into from
Mar 7, 2022

Conversation

kyteinsky
Copy link
Contributor

Description

Combines add and edit modals for ease of use.

@liyasthomas @AndrewBastin

Please check if the PR does what it needs to.

@netlify
Copy link

netlify bot commented Feb 25, 2022

👷 Deploy request for hoppscotch pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: abf5524

liyasthomas
liyasthomas previously approved these changes Feb 26, 2022
Copy link
Member

@liyasthomas liyasthomas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

There is now an issue with saving an environment

Steps to reproduce

  1. Create an environment and add some variables.
  2. Save the environment
  3. Now click on the newly created environment to edit it.
  4. Observe that the environment variables not appearing.

@kyteinsky
Copy link
Contributor Author

@AndrewBastin I am really out of it these days, huh

liyasthomas
liyasthomas previously approved these changes Feb 28, 2022
Copy link
Member

@liyasthomas liyasthomas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

One minor correction and then one missed implementation.

Steps to reproduce:

  1. Disable any active environment selection (set to no environment)
  2. Create a REST Request, add a pre-request script with the given content
pw.env.set("x", "y");
  1. Execute the request.

The execution will lead to the assignment of the variable to environment, but since none is selected, there should be an alert in the "Test Results" tab asking what to do with it. If we selected, Create Environment there, it opens up the Edit Environment Modal but without proper configuration (It shows "Edit" instead of "New", it doesn't populate the environment configuration that will be added etc.)

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

It would be cool if on clicking "Create New Envrionment" from Test Result Alert, it fills up the environment variables the Test Results have found out onto the modal.

@kyteinsky
Copy link
Contributor Author

One thing I find inconsistent is the Global environment. Consider this scenario:

  • Don't select any environment
  • Run a pre-request script to set an env variable variable: value
  • Click on Add to Global button in Test Results tab
  • variable gets added to Global
  • Now change value to value2 in pre-request scripts and click Send
  • value is changed to value2 inside Global env
  • Now change the variable (key) field in pre-request scripts to somekey and click Send
  • Test Results show that no env is selected

@AndrewBastin
Copy link
Member

@kyteinsky Yup, that is an interesting observation.

@liyasthomas, did you miss the code to make sure changes to the global only not trigger that alert box ?

@liyasthomas
Copy link
Member

liyasthomas commented Mar 7, 2022

One thing I find inconsistent is the Global environment. Consider this scenario:

  • Don't select any environment
  • Run a pre-request script to set an env variable variable: value
  • Click on Add to Global button in Test Results tab
  • variable gets added to Global
  • Now change value to value2 in pre-request scripts and click Send
  • value is changed to value2 inside Global env
  • Now change the variable (key) field in pre-request scripts to somekey and click Send
  • Test Results show that no env is selected

I guess this is the expected behavior. If you don't select any environment, we'll prompt you to add the new variable to either "Global" or "Create new environment".
As you changed the variable key from variable to somekey, Hoppscotch treats it as a new variable addition thus must ask what to do with the new environment variables.

@kyteinsky @AndrewBastin

@AndrewBastin
Copy link
Member

AndrewBastin commented Mar 7, 2022

@liyasthomas ahh gotcha. that situation is handled, I thought @kyteinsky was talking (I misread) about Global updations (which we do ignore, I verified just now).

@kyteinsky, if you change the key, that is considered a new key.

Global variables are kinda transparent and affects all the requests, in order to prevent the user absolutely not knowing about it, unlike an environment variable being set, all ways of creating Global Variables are intentionally explicit (requiring user's consent). Selecting no environment doesn't mean Global is the executing environment, it doesn't have the same behaviors as a regular environment. There is always an affinity to the executing environment for making changes.

So the behavior you brought up is intentional

@kyteinsky
Copy link
Contributor Author

Oh okay. It just seemed a bit queer to me that this happened. If that is the expected behaviour, then there's no issue. Although, imo replacing Select environment with Global would've cleared things up a bit for the end user.

@AndrewBastin @liyasthomas

@liyasthomas
Copy link
Member

Ahh, Global environments are not something that users can "switch context into" something but rather a set of environment variables that's available in all environment contexts.

@kyteinsky
Copy link
Contributor Author

I see. Now everything seems to come together. The way Global behaves.

AndrewBastin
AndrewBastin previously approved these changes Mar 7, 2022
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@liyasthomas a final check pass ?

liyasthomas
liyasthomas previously approved these changes Mar 7, 2022
@AndrewBastin AndrewBastin dismissed stale reviews from liyasthomas and themself via 7600fb7 March 7, 2022 10:56
@AndrewBastin AndrewBastin self-requested a review March 7, 2022 11:01
@AndrewBastin AndrewBastin merged commit ce652b5 into hoppscotch:main Mar 7, 2022
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.

None yet

3 participants