Skip to content

SLVS-1373 Validate credentials in OrganizationSelectionDialog #5634

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 27, 2024
Merged

SLVS-1373 Validate credentials in OrganizationSelectionDialog #5634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64638d0
SLVS-1373 Use CredentialsModel in ValidateConnectionAsync instead of …
gabriela-trutan-sonarsource Aug 26, 2024
dce2080
SLVS-1373 Validate connection when manually choosing an organization.
gabriela-trutan-sonarsource Aug 26, 2024
eeba39f
SLVS-1373 Validate connection after the organization was selected fro…
gabriela-trutan-sonarsource Aug 26, 2024
aa09f6a
SLVS-1373 Apply review feedback: reuse more logic, prevent creation o…
gabriela-trutan-sonarsource Aug 27, 2024
6a9c851
SLVS-1373 Bundle validation and updating state of the dialog in the s…
gabriela-trutan-sonarsource Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 23 additions & 26 deletions src/ConnectedMode.UnitTests/UI/OrganizationSelection/OrganizationSelectionViewModelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,31 @@ public void Ctor_OrganizationList_SetsPropertyValue()
testSubject.Organizations[0].Should().Be(organization);
}

[TestMethod]
public void ConnectionInfo_SetByDefaultToSonarCloudWithIdToNull()
{
testSubject.ConnectionInfo.Id.Should().BeNull();
testSubject.ConnectionInfo.ServerType.Should().Be(ConnectionServerType.SonarCloud);
}

[TestMethod]
public void SelectedOrganization_NotSet_ValueIsNull()
{
testSubject.SelectedOrganization.Should().BeNull();
testSubject.IsValidSelectedOrganization.Should().BeFalse();
}

[TestMethod]
public void SelectedOrganization_ValueChanges_UpdatesConnectionInfo()
{
testSubject.SelectedOrganization = null;
testSubject.ConnectionInfo.Id.Should().BeNull();

testSubject.SelectedOrganization = new OrganizationDisplay("key", "name");
testSubject.ConnectionInfo.Id.Should().Be(testSubject.SelectedOrganization.Key);
testSubject.ConnectionInfo.ServerType.Should().Be(ConnectionServerType.SonarCloud);
}

[TestMethod]
public void SelectedOrganization_Set_RaisesEvents()
{
Expand Down Expand Up @@ -244,35 +262,14 @@ await progressReporterViewModel.Received(1)
}

[TestMethod]
public async Task ValidateConnectionAsync_CallsExecuteTaskWithProgressAsync()
{
testSubject.SelectedOrganization = new OrganizationDisplay("myKey", "myName");

await testSubject.ValidateConnectionAsync();

await progressReporterViewModel.Received(1)
.ExecuteTaskWithProgressAsync(Arg.Is<ITaskToPerformParams<AdapterResponse>>(x =>
IsExpectedSlCoreAdapterValidateConnectionAsync(x.TaskToPerform, testSubject.SelectedOrganization.Key) &&
x.ProgressStatus == UiResources.ValidatingConnectionProgressText &&
x.WarningText == UiResources.ValidatingConnectionFailedText));
}

[TestMethod]
public void CreateConnectionInfo_OrganizationIsSelected_ReturnsSonarCloudConnectionWithOrganizationKey()
public void UpdateConnectionInfo_ValueChanges_UpdatesConnectionInfo()
{
var organizationKey = "key";

var connectionInfo = OrganizationSelectionViewModel.CreateConnectionInfo(organizationKey);
testSubject.ConnectionInfo.Id.Should().BeNull();

connectionInfo.Should().NotBeNull();
connectionInfo.Id.Should().Be(organizationKey);
connectionInfo.ServerType.Should().Be(ConnectionServerType.SonarCloud);
}
testSubject.UpdateConnectionInfo("newKey");

[TestMethod]
public void CreateConnectionInfo_OrganizationIsNotSelected_ThrowsException()
{
Assert.ThrowsException<ArgumentException>(() => OrganizationSelectionViewModel.CreateConnectionInfo(null));
testSubject.ConnectionInfo.Id.Should().Be("newKey");
testSubject.ConnectionInfo.ServerType.Should().Be(ConnectionServerType.SonarCloud);
}

private bool IsExpectedSlCoreAdapterValidateConnectionAsync(Func<Task<AdapterResponse>> xTaskToPerform, string organizationKey)
Expand Down
7 changes: 2 additions & 5 deletions src/ConnectedMode/UI/ManageConnections/ManageConnectionsDialog.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,8 @@ private ConnectionInfo FinalizeConnection(ConnectionInfo newConnectionInfo, Cred
}

var organizationSelectionDialog = new OrganizationSelectionDialog(connectedModeServices, credentialsDialog.ViewModel.GetCredentialsModel());
if (organizationSelectionDialog.ShowDialog(this) == true)
{
return newConnectionInfo with { Id = organizationSelectionDialog.ViewModel.SelectedOrganization.Key };
}
return null;

return organizationSelectionDialog.ShowDialog(this) == true ? organizationSelectionDialog.ViewModel.ConnectionInfo : null;
}

private void ManageConnectionsWindow_OnInitialized(object sender, EventArgs e)
Expand Down
29 changes: 18 additions & 11 deletions src/ConnectedMode/UI/OrganizationSelection/OrganizationSelectionDialog.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,35 +42,36 @@ public OrganizationSelectionDialog(IConnectedModeServices connectedModeServices,

private async void OkButton_OnClick(object sender, RoutedEventArgs e)
{
var isConnectionValid = await ViewModel.ValidateConnectionAsync();
var isConnectionValid = await ValidateConnectionForSelectedOrganizationAsync(ViewModel.SelectedOrganization.Key);
if(isConnectionValid)
{
DialogResult = true;
OnConnectionValidationSucceeded(ViewModel.SelectedOrganization.Key);
}
}

private async void ChooseAnotherOrganizationButton_OnClick(object sender, RoutedEventArgs e)
{
ViewModel.SelectedOrganization = null;
var manualOrganizationSelectionDialog = new ManualOrganizationSelectionDialog();
var isSelectedManualOrganizationValid = await ValidateManualOrganizationAsync(manualOrganizationSelectionDialog);
var manualSelectionDialogSucceeded = manualOrganizationSelectionDialog.ShowDialog(this);
if(manualSelectionDialogSucceeded is not true)
{
return;
}

var isSelectedManualOrganizationValid = await ValidateConnectionForSelectedOrganizationAsync(manualOrganizationSelectionDialog.ViewModel.OrganizationKey);
if (isSelectedManualOrganizationValid)
Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the gain of setting the SelectedOrganization if we will anyway pass the organization value to the Validate method?

Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So the thing is. We will need to do a few common things for both scenarious

  1. catch exception for both manual and list selection 😉
  2. show duplication errors with connection id
  3. 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)

Copy link
Contributor Author

@gabriela-trutan-sonarsource gabriela-trutan-sonarsource Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

{
ViewModel.SelectedOrganization = new OrganizationDisplay(manualOrganizationSelectionDialog.ViewModel.OrganizationKey, null);
DialogResult = true;
OnConnectionValidationSucceeded(manualOrganizationSelectionDialog.ViewModel.OrganizationKey);
}
}

private async Task<bool> ValidateManualOrganizationAsync(ManualOrganizationSelectionDialog manualOrganizationSelectionDialog)
private async Task<bool> ValidateConnectionForSelectedOrganizationAsync(string selectedOrganizationKey)
{
try
{
var manualSelectionDialogSucceeded = manualOrganizationSelectionDialog.ShowDialog(this);
var organizationSelectionInvalidMsg = string.Format(UiResources.ManualOrganziationSelectionFailedText, manualOrganizationSelectionDialog.ViewModel.OrganizationKey);
var isConnectionValid = await ViewModel.ValidateConnectionForOrganizationAsync(manualOrganizationSelectionDialog.ViewModel.OrganizationKey, organizationSelectionInvalidMsg);

return manualSelectionDialogSucceeded is true && isConnectionValid;
var organizationSelectionInvalidMsg = string.Format(UiResources.ValidatingOrganziationSelectionFailedText, selectedOrganizationKey);
return await ViewModel.ValidateConnectionForOrganizationAsync(selectedOrganizationKey, organizationSelectionInvalidMsg);
}
catch (Exception e) when (!ErrorHandler.IsCriticalException(e))
{
Expand All @@ -83,5 +84,11 @@ private async void OrganizationSelectionDialog_OnLoaded(object sender, RoutedEve
{
await ViewModel.LoadOrganizationsAsync();
}

private void OnConnectionValidationSucceeded(string organizationKey)
{
ViewModel.UpdateConnectionInfo(organizationKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not call this inside the validation method and use the ConnectionInfo object that was already created there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it should only be done once the validation succeeded and the dialog is closed.

DialogResult = true;
}
}

23 changes: 10 additions & 13 deletions src/ConnectedMode/UI/OrganizationSelection/OrganizationSelectionViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ namespace SonarLint.VisualStudio.ConnectedMode.UI.OrganizationSelection;

public class OrganizationSelectionViewModel(ICredentialsModel credentialsModel, ISlCoreConnectionAdapter connectionAdapter, IProgressReporterViewModel progressReporterViewModel) : ViewModelBase
{
/// <summary>
/// The <see cref="ConnectionInfo"/> that is used to connect to the server, whose <see cref="ConnectionInfo.Id"/> can be different from the <see cref="SelectedOrganization"/>
/// due to the fact that an organization key can also be entered manually rather than selected form the list of <see cref="Organizations"/>.
/// </summary>
public ConnectionInfo ConnectionInfo { get; private set; } = new(null, ConnectionServerType.SonarCloud);
public IProgressReporterViewModel ProgressReporterViewModel { get; } = progressReporterViewModel;

public OrganizationDisplay SelectedOrganization
Expand All @@ -35,6 +40,7 @@ public OrganizationDisplay SelectedOrganization
set
{
selectedOrganization = value;
UpdateConnectionInfo(selectedOrganization?.Key);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here

RaisePropertyChanged();
RaisePropertyChanged(nameof(IsValidSelectedOrganization));
}
Expand Down Expand Up @@ -64,11 +70,6 @@ public async Task LoadOrganizationsAsync()
await ProgressReporterViewModel.ExecuteTaskWithProgressAsync(organizationLoadingParams);
}

public async Task<bool> ValidateConnectionAsync()
{
return await ValidateConnectionForOrganizationAsync(SelectedOrganization.Key, UiResources.ValidatingConnectionFailedText);
}

internal async Task<AdapterResponseWithData<List<OrganizationDisplay>>> AdapterLoadOrganizationsAsync()
{
return await connectionAdapter.GetOrganizationsAsync(credentialsModel);
Expand All @@ -83,21 +84,17 @@ internal void UpdateOrganizations(AdapterResponseWithData<List<OrganizationDispl

internal async Task<bool> ValidateConnectionForOrganizationAsync(string organizationKey, string warningText)
Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can be made private, see other comment. The warning text does not need to be parametrized as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it does. For usability purposes.

{
var connectionInfo = CreateConnectionInfo(organizationKey);
var connectionInfoToValidate = ConnectionInfo with { Id = organizationKey };
var validationParams = new TaskToPerformParams<AdapterResponse>(
async () => await connectionAdapter.ValidateConnectionAsync(connectionInfo, credentialsModel),
async () => await connectionAdapter.ValidateConnectionAsync(connectionInfoToValidate, credentialsModel),
UiResources.ValidatingConnectionProgressText,
warningText);
var adapterResponse = await ProgressReporterViewModel.ExecuteTaskWithProgressAsync(validationParams);
return adapterResponse.Success;
}

internal static ConnectionInfo CreateConnectionInfo(string organizationKey)
public void UpdateConnectionInfo(string organizationKey)
{
if(organizationKey is null)
{
throw new ArgumentException($"{nameof(organizationKey)} must be set.");
}
return new ConnectionInfo(organizationKey, ConnectionServerType.SonarCloud);
ConnectionInfo = ConnectionInfo with { Id = organizationKey };
}
}
18 changes: 9 additions & 9 deletions src/ConnectedMode/UI/Resources/UiResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/ConnectedMode/UI/Resources/UiResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@
<data name="LoadingOrganizationsProgressText" xml:space="preserve">
<value>Loading organizations...</value>
</data>
<data name="ManualOrganziationSelectionFailedText" xml:space="preserve">
<data name="ValidatingOrganziationSelectionFailedText" xml:space="preserve">
<value>The connection is not valid for the chosen organization "{0}". Make sure the entered key matches exactly your organization's key.</value>
</data>
</root>