Multiple issues: Fixed errors with updating optional profile fields

[katelynienaber]

Proposed changes

This is one change that fixes several issues.

1. Fix for issue Editing Base Path on a ZE Connection Profile Doesn't Save it #989: There has been a bugfix! 😃 Which means in Zowe Explorer we can now pass an IUpdateProfile object to CliProfileManager.update()...and we don't have to change the profile object key names to resemble CLI command params. This was the cause of Editing Base Path on a ZE Connection Profile Doesn't Save it #989.

2. Fix for issue Enhance Optional Profiles #1095: I set merge: false in the profile, so only the new values will be kept, not the old (ie. now we don't have to store an empty string as a placeholder for an optional field).

3. Fix for issue Default responseTimeout=0 Causes Zowe CLI Error #1016: There are two new conditions in our updateProfile() function, which will remove the responseTimeout field if it was set to 0, and any other field if it was set to an empty string. And because of fix 2 (above), Default responseTimeout=0 Causes Zowe CLI Error #1016 should not reoccur.

Linked issues (bugs):
#989
#1016
#1095

Other linked issues (to-dos that are no longer needed):
#1038
#1090

Release Notes

Milestone: 1.12
Changelog: Fixed errors with updating optional profile fields

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

• I have read the CONTRIBUTOR GUIDANCE wiki
• PR title follows Conventional Commits Guidelines
• PR Description is included
• gif or screenshot is included if visual changes are made
• yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
• All checks have passed (DCO, Jenkins and Code Coverage)
• I have added unit test and it is passing
• I have added integration test and it is passing
• There is coverage for the code that I have added
• I have tested it manually and there are no regressions found
• I have added necessary documentation (if appropriate)
• Any PR dependencies have been merged and published (if appropriate)

[JillieBeanSim]

Hey @katelynienaber I was testing this PR, while creating a profile I still get "" for optional values left blank along with the 0 value for encoding and response timeout that where left blank/entered passed.

Then I edited the same profile leaving those same values blank and still got 0 in the yaml for encoding and response timeout.

[JillieBeanSim]

see comment above for issues I ran into.

the changes requested are:
Should we use this same way of thinking during create?
Also, would like to see the default value for encoding used or leave it out of the yaml
Finally, shouldn't it leave response timeout out of the yaml if value = 0?

[katelynienaber]

see comment above for issues I ran into.

the changes requested are:
Should we use this same way of thinking during create?
Also, would like to see the default value for encoding used or leave it out of the yaml
Finally, shouldn't it leave response timeout out of the yaml if value = 0?

Hey @JillieBeanSim, thanks for reviewing! :D Give it a shot now.

Should we use this same way of thinking during create?
I think so (and now Create should work in the new way, not storing empty values). Create is kind of hacky right now because the CliProfileManager.save function does some validation, and won't let you pass in non-string values for user & password. So I'm calling our updateProfile function on newly-created profiles, just after the profile manager is refreshed, to remove any blank fields.

Also, would like to see the default value for encoding used or leave it out of the yaml
This one should be fixed. I rearranged the conditions in that loop I added.

Finally, shouldn't it leave response timeout out of the yaml if value = 0?
This should also be good now.

[jellypuno]

Should we use this same way of thinking during create?
I think so (and now Create should work in the new way, not storing empty values). Create is kind of hacky right now because the CliProfileManager.save function does some validation, and won't let you pass in non-string values for user & password. So I'm calling our updateProfile function on newly-created profiles, just after the profile manager is refreshed, to remove any blank fields.

@katelynienaber Let's think of another way and not implement a hack. We can check zowe/cli code as to how they implmented this.

@JillieBeanSim When you say "same way of thinking during create", Are you thinking of re-factoring that process? Code-wise or UX-wise or both? If yes, do you a design that we can look into?

[katelynienaber]

@JillieBeanSim Ok, now we have a not-hacky solution :) @jellypuno pointed out that this option could allow passing optional parameters, and it works (at least for me)

Please check again when you have time ^^"

[JillieBeanSim]

@JillieBeanSim When you say "same way of thinking during create", Are you thinking of re-factoring that process? Code-wise or UX-wise or both? If yes, do you a design that we can look into?

@jellypuno I was only thinking of the way it was saved to the yaml, ie. no empty strings and 0 values, not a UI change.

Thanks for looking into my questions and concerns @katelynienaber & @jellypuno I will retest the PR today.

[JillieBeanSim]

@katelynienaber the functionality works well now and as expected for create and update of profiles without optional user/pass and for the empty encoding and response timeout values. I do see this message pop up sometimes after editing a profile that doesn't have the values mentioned. Not sure but I believe this error is being handled in @jellypuno PR #1100 I know I ran into it there as well. If this is correct @jellypuno let me know and I will approve this PR

[zdmullen]

Merge this after Jelly's PR #1100!

[katelynienaber]

@JillieBeanSim I think this issue was fixed in @jellypuno's PR, could you check it out again? I don't see it in mine.

[lauren-li]

This PR is looking good to me. All unit and expected integration tests are passing. I am able to create and edit profiles without specifying username, password, encoding, and/or timeout, and the yaml is created/updated without empty fields, which is really great. I do sometimes see the error that @JillieBeanSim mentioned in her comment above, ... but I can also get it to happen in v1.11.1 the same way (but not reliably). We may want to look into it as a separate issue.

Another issue I encountered while testing, which I also see in 1.11.1 (and thus not a blocker for this PR) is that I get prompted for credentials immediately upon finishing creation or editing of a profile without username/password. I suppose this behavior has been around for a while, but somehow I only just now noticed it, and it feels unexpected since I am not trying to use the profile to access anything yet; I'm just udpating/creating it. Again, this could be investigated as a separate issue. I just wanted to mention it here so others are aware that it isn't a new behavior with this PR.

Thanks @katelynienaber for the fixes in this PR! It was such a pain to deal with empty profile fields when troubleshooting/testing other profile-related PRs, and I am happy to see that getting resolved.

[katelynienaber]

@lauren-li @JillieBeanSim It's fine with me if you want to wait on this one, it wasn't promised in the PI goals :D We should bring it up with @zdmullen and @jellypuno as well.

Billie if you already have something in progress to fix this error you can have it XD I am glad to jump on if you need me and ofc review too.

[JillieBeanSim]

This PR works great now with #1178 added in. I don't see any of the issues we were seeing before. Thanks @katelynienaber for doing this work. All expected tests and checks are passing

[lauren-li]

This looks good to me now! All unit and expected integration tests are passing. Omitting credentials when creating and updating a profile in Zowe Explorer now omits the respective fields instead of creating empty strings. With the merging-in of PR #1178, this works much more seamlessly than before (i.e. no new "error" pop-ups). This PR also appears to be "backwards compatible" with the existing credential-less profiles, in that those profiles are still able to be used, and updating them with empty credentials will just completely remove the blank user and password fields.

Thanks @katelynienaber for your hard work on this PR!