| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-7268 Improve UX when IDSDK is not installed on system running Dynamo #2 #15799
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
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7268
| } | ||
| catch (Exception e) | ||
| { | ||
| // Log the exception and show a notification to the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added this to handle a crash that I encountered while testing, I think there will be more work done for cluster autocomplete integration and this will likely change, so we can handle it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one comment
| @@ -149,6 +150,7 @@ private void LoginButton_OnClick(object sender, RoutedEventArgs e) | |||
|
|
|||
| private void LogoutOption_Click(object sender, RoutedEventArgs e) | |||
| { | |||
| if (!DynamoViewModel.IsIDSDKInitialized()) return; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to avoid needing to add these checks everywhere? What about adding this check, or an event that eventually raises this check to getters that access data that would have required initialization to succeed in the authprovider - ie the token.
With the current solution proposed here devs need to know that they should add these checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first attempt(#15790) was exactly that, but problem with that is we use those getters at multiple places, so for example, when user clicks on SignIn button, the event would be fired 6 times and the popup will be shown 6 times. I met with @pinzart to discuss a solution, this approach may be more code, but gives more control.
|
Tested with IDSDK disabled, and it LGTM from that point of view. |
Purpose
This is a different implementation of the previous PR
In this we manage each method that calls the Auth individually.
This is updated so that we can show a warning for each user action.
So now each user action like login, triggering autocomplete or publishing a package etc. that requires Auth will throw the warning of IDSDK is not initialized/available.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Improve UX when IDSDK is not installed on system running Dynamo #2
Reviewers