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

Issue a TSO Command #1245

Merged
merged 24 commits into from
Apr 16, 2021
Merged

Issue a TSO Command #1245

merged 24 commits into from
Apr 16, 2021

Conversation

JillieBeanSim
Copy link
Contributor

@JillieBeanSim JillieBeanSim commented Mar 22, 2021

Signed-off-by: Billie Simmons 49491949+JillieBeanSim@users.noreply.github.com

Proposed changes

Be able to issue a TSO command using Zowe Explorer

Release Notes

Milestone: 1.14.0

Changelog: Added Issue TSO Command UI capability via right-click action on tree session and also available in the command palate. New API ICommand with issueTsoCommand() and issueMvsCommand() for consumption by Zowe Explorer Extenders.

NOTE see #1245 (comment) for information about new TSO integration test

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)

Further comments

For zosmf profiles there is a check for a tso profile, if non exist the user is prompted for account number, if one exists it will be automatically used for the account number, and if multiple exist the user will be presented with a list of tso profiles to choose from for the account number.

For extenders, I have a check for the new API ICommand and if it is not existing I have an Error message that it is Not implemented yet.

tso-command-ZE

With the request to have the validation check change the icon automatically, I have run into an issue since this is available via all 3 trees and I do have the validation setting the icon but this is only showing when the session is clicked on after and not automatically, also a second validation is not run for that session.

tso-command-ZE-validation

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim self-assigned this Mar 30, 2021
@JillieBeanSim JillieBeanSim added this to the 1.14 Release milestone Mar 30, 2021
@JillieBeanSim JillieBeanSim added 21PI1 API Extender Issues that allow Zowe Explorer to be extended externally priority-high Production outage - this quarter or at least next quarter labels Mar 30, 2021
@JillieBeanSim JillieBeanSim linked an issue Mar 30, 2021 that may be closed by this pull request
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim marked this pull request as ready for review April 12, 2021 15:41
jellypuno and others added 5 commits April 12, 2021 18:03
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Apr 13, 2021

MVS integration test fixed and new TSO integration test added

There is a change in the testProfileData.examples.ts which will need to be reflected in testProfileData.ts for tsoProfile Name, this will allow the TSO integration test to access the named tso profile for account number needed for tso session with zosmf profile.
image

@JillieBeanSim
Copy link
Contributor Author

ZE-integration-mvs-tso

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@lauren-li
Copy link
Contributor

@JillieBeanSim I'm not all the way done looking through the code for this yet, but it is functionally looking very good so far, and all unit/expected integration tests are passing. A couple small things I was wondering about:

  • Would it make sense to check/ask for account number before the command?
  • Do we want the "Not implemented yet" message to specify the extender or profile type that isn't supported? When we display "Not implemented yet", it kind of looks like we're trying to say this hasn't been implemented at all in Zowe Explorer yet (rather than for a specific extender). Perhaps this is something we want to create a reusable message for (for other new functionality we add in the future) - in which case it might be out of scope for this PR.

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Apr 15, 2021

@JillieBeanSim I'm not all the way done looking through the code for this yet, but it is functionally looking very good so far, and all unit/expected integration tests are passing. A couple small things I was wondering about:

  • Would it make sense to check/ask for account number before the command?
  • Do we want the "Not implemented yet" message to specify the extender or profile type that isn't supported? When we display "Not implemented yet", it kind of looks like we're trying to say this hasn't been implemented at all in Zowe Explorer yet (rather than for a specific extender). Perhaps this is something we want to create a reusable message for (for other new functionality we add in the future) - in which case it might be out of scope for this PR.

@lauren-li These comments have been addressed for this PR, but having the not implemented message with profile type as reusable across Zowe Explorer would need to be done separately as the arguments sent to Error Handling function and everything that calls it would need to be rewritten to pass the whole profile: IProfileLoaded (to include type) vs just profile.name so that would be a huge change. I have it displaying with the profile type for MVS and TSO commands for now if extender hasn't brought in the new ICommandApi
image

I moved the prompt for acctNum to before the command prompt, but this prompt is specific to zosmf profiles at this moment as FTP doesn't have it implemented yet and RSE doesn't require it. Without being able to test other scenarios I am leaving it up to the extender to obtain their account number if needed in their API file vs unnecessarily prompting for it if not needed and confusing the user. If a time comes that we may need to extend this scenario we can address it in the future.

@jellypuno
Copy link
Contributor

This is amazing! Thank you for working on this @JillieBeanSim . I just skimmed through the code and tested the functionality itself and it looks good for me. I think there are additional features that I would like to see in the future. I can open a separate issue for that.

  1. Enable users to create TSO profiles or save the account number in a buffer so I don't have to input it every time I want to issue a TSO command.
  2. Separating the persisted MVS and TSO commands. So far, they are combined and it confuses me sometimes.

But overall, it works for me. Thank you!

@JillieBeanSim
Copy link
Contributor Author

This is amazing! Thank you for working on this @JillieBeanSim . I just skimmed through the code and tested the functionality itself and it looks good for me. I think there are additional features that I would like to see in the future. I can open a separate issue for that.

  1. Enable users to create TSO profiles or save the account number in a buffer so I don't have to input it every time I want to issue a TSO command.
  2. Separating the persisted MVS and TSO commands. So far, they are combined and it confuses me sometimes.

But overall, it works for me. Thank you!

@jellypuno by persisted MVS & TSO commands do you mean the output window? I just noticed the dropdown will have 2 of the same Zowe Commands where 1 is MVS and the other is TSO. I totally didn't notice that before and thought I was doing something good reusing the code vs having it duplicated lol. We can totally fix that in a hot fix or next release.

Copy link
Contributor

@lauren-li lauren-li left a comment

Choose a reason for hiding this comment

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

I moved the prompt for acctNum to before the command prompt, but this prompt is specific to zosmf profiles at this moment as FTP doesn't have it implemented yet and RSE doesn't require it. Without being able to test other scenarios I am leaving it up to the extender to obtain their account number if needed in their API file vs unnecessarily prompting for it if not needed and confusing the user. If a time comes that we may need to extend this scenario we can address it in the future.

This sounds good to me. Thanks @JillieBeanSim for addressing my comments!

I also agree we probably want to separate the persisted commands for MVS vs TSO (i.e. the ones that appear in the dropdown), as well as their outputs, but I think it's fine to do this in a future PR. The TSO command functionality itself seems to be working very well. All unit and expected integration tests (plus the command submission test) are still passing for me.

This PR looks good to me!

Note: Per previous scrums, the Theia tests will be investigated and fixed in a separate PR.

@lauren-li lauren-li merged commit 0db2d8e into master Apr 16, 2021
@lauren-li lauren-li deleted the tso-command branch April 16, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Extender Issues that allow Zowe Explorer to be extended externally priority-high Production outage - this quarter or at least next quarter
Projects
None yet
3 participants