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

fix(create-recordings): add error-views and refactor target listener #687

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Nov 22, 2022

Fixed #673
Fixed #686

The following fixes were applied:

  • Add a missing error view to CustomRecordingForm.
  • Add a missing error view to SnapShotRecordingForm (only auth failure and ssl error will show).
  • Refactor target selection listener of CustomRecordingForm to update UI when the target selection is changed.

Others:

  • Add unit tests for SnapShotRecordingForm.

@tthvo tthvo added the fix label Nov 22, 2022
@tthvo tthvo force-pushed the sub-route-error branch 2 times, most recently from e49b884 to 7f883ae Compare November 22, 2022 18:42
@tthvo
Copy link
Member Author

tthvo commented Nov 22, 2022

Error handler for SnapShotRecordingForm is a little complex:

  • Must add listener for authRetry to update form because failed api requests from custom recording tab also trigger any listener for authFailure and valid credentials do not update snapshot tab because it originates from custom recording tab.
  • Must add listener for sslFailure because context.api.createRecording only returns true/false. Other errors are shown through notification messages.
  • Must add listener for context.target.target to reset form if the target selection is changed.

@tthvo tthvo marked this pull request as ready for review November 22, 2022 18:49
maxcao13
maxcao13 previously approved these changes Nov 27, 2022
Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

Nice, I like the changes, great job!

@tthvo
Copy link
Member Author

tthvo commented Nov 28, 2022

Ohh thanks! Looking back again at CustomRecordingForm, I think having 2 independent api calls for recording form data (i.e. template dropdown and recordingOptions) might cause an issue if only 1 call fails (i.e the result of the api request that comes later will decide if the error view should be shown).

To tackle this, I added a small fix with forkJoin that avoid above issues :D However, this means only the first error will be reported on screen if both api calls throw.

You can review again^^

@andrewazores
Copy link
Member

Looks good, just rebase to resolve the conflict please.

@andrewazores andrewazores merged commit 89842bb into cryostatio:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
3 participants