SLVS-1373 Validate credentials in OrganizationSelectionDialog #5634
Conversation
terraform-techuser
changed the title
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| if (manualSelection is true) | ||
| var isSelectedManualOrganizationValid = await ValidateManualOrganizationAsync(manualOrganizationSelectionDialog); | ||
|
|
||
| if (isSelectedManualOrganizationValid) |
There was a problem hiding this comment.
I think this code can be simplified if you remove this check and put the old check back. It would assign SelectedOrganization whichever value is selected (list or manual), and then always validate the SelectedOrganization in ViewModel in a uniform way. The error text can be made generic to not worry about mentioning manual organization selection in it. This will be even more relevant once we add the duplication check.
There was a problem hiding this comment.
I removed the assigning of the SelectedOrganization from the manual selection for two reasons:
-
Once the SelectedOrganization is assigned, the OK button becomes enabled. This should not be the case, though, because the organization is not valid. Therefore not assigning the SelectedOrganization in the first place, makes it more consistent.
-
The SelectedOrganization refers to the organization selected from the list. But the manual one that was chosen is not part of the list. This brings a lot of confusion: the view model property (hence also the Combobox.SelectedItem) is set to a string, but that string is not part of the list. You will notice that WPF does not add a combobox item that is selected, if you bind its view model to a string (as long as it's not part of the list, then it should not be a selected one)
-
It is more understanding for the user to have separate error messages for the manual selection or for the list selection. Imagine that the list of organizations is long. It would be very difficult for the user to know which organization was wrong, without scrolling like crazy and trying to find out if the organization was from the list or a manual one that he entered.
There was a problem hiding this comment.
We can split the list selection from the value that is validated (the organization value then can be passed as a parameter to ViewModel.Validate). SelectedOrganization can be reset after validation fails. The error text can include the organization key. There will be no confusion if we reset selection and show the key in the error.
There was a problem hiding this comment.
What is the gain of setting the SelectedOrganization if we will anyway pass the organization value to the Validate method?
There was a problem hiding this comment.
So the thing is. We will need to do a few common things for both scenarious
- catch exception for both manual and list selection 😉
- show duplication errors with connection id
- set some sort of final value for the manage connections dialog to use. currently we create connectioninfo here, discard it, and then create another connecioninfo in manage connections dialog using SelectedOrganization
Try to tick all 3 of those boxes with refactoring. You don't have to do it in the way I initially suggested (I though it was a bit more simple to fix)
There was a problem hiding this comment.
Point 1 and 2 are solved by the fact that we use the same method in the ViewModel: ValidateConnectionForOrganizationAsync
I could work on point 3 and find a different approach
| @@ -76,4 +80,24 @@ internal void UpdateOrganizations(AdapterResponseWithData<List<OrganizationDispl | |||
| responseWithData.ResponseData.ForEach(AddOrganization); | |||
| RaisePropertyChanged(nameof(NoOrganizationExists)); | |||
| } | |||
|
|
|||
| internal async Task<bool> ValidateConnectionForOrganizationAsync(string organizationKey, string warningText) | |||
There was a problem hiding this comment.
Can be made private, see other comment. The warning text does not need to be parametrized as well.
There was a problem hiding this comment.
I think it does. For usability purposes.
…f ConnectionInfo in ManageConnectionsDialog and OrganizationSelection
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
|
||
| private void OnConnectionValidationSucceeded(string organizationKey) | ||
| { | ||
| ViewModel.UpdateConnectionInfo(organizationKey); |
There was a problem hiding this comment.
why not call this inside the validation method and use the ConnectionInfo object that was already created there?
There was a problem hiding this comment.
Because it should only be done once the validation succeeded and the dialog is closed.
| @@ -35,6 +40,7 @@ public OrganizationDisplay SelectedOrganization | |||
| set | |||
| { | |||
| selectedOrganization = value; | |||
| UpdateConnectionInfo(selectedOrganization?.Key); | |||
There was a problem hiding this comment.
Here
…ame method. Remove logic to keep ConnectionInfo in sync with the selected organization
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
georgii-borovinskikh-sonarsource
merged commit 02f0efd
into
feature/new-connected-mode
georgii-borovinskikh-sonarsource
deleted the
gt/organizations-dialog-validate-connection
branch
SLVS-1373