SLVS-1394 Load organizations #5629
Conversation
terraform-techuser
changed the title
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| @@ -63,13 +68,39 @@ public async Task<ValidateConnectionResponse> ValidateConnectionAsync(Connection | |||
| return await ValidateConnectionAsync(validateConnectionParams); | |||
| } | |||
|
|
|||
| public async Task<AdapterResponse<List<OrganizationDisplay>>> GetOrganizationsAsync(ICredentialsModel credentialsModel) | |||
| { | |||
| var failedResponse = new AdapterResponse<List<OrganizationDisplay>>(false, []); | |||
There was a problem hiding this comment.
Can be static readonly
There was a problem hiding this comment.
What can be static readonly?
There was a problem hiding this comment.
The variable failedResponse, you mean?
There was a problem hiding this comment.
Done
| { | ||
| var failedResponse = new AdapterResponse<List<OrganizationDisplay>>(false, []); | ||
|
|
||
| return await threadHandling.RunOnBackgroundThread(async () => |
There was a problem hiding this comment.
Can return without async
There was a problem hiding this comment.
I think it is better to be explicit.
There was a problem hiding this comment.
Eh, not really. You have the async lambda inside and you could add the Async suffix to the method name. Not a big deal in the end, but it would technically be more efficient to return just the task and not await it's result here
There was a problem hiding this comment.
I removed the async
| @@ -85,6 +116,17 @@ private async Task<ValidateConnectionResponse> ValidateConnectionAsync(ValidateC | |||
| }); | |||
| } | |||
|
|
|||
| private IConnectionConfigurationSLCoreService TryGetConnectionConfigurationSlCoreService() | |||
There was a problem hiding this comment.
Try pattern should return bool and have an out parameters
|
|
||
| namespace SonarLint.VisualStudio.ConnectedMode.UI.OrganizationSelection; | ||
|
|
||
| [ExcludeFromCodeCoverage] | ||
| public partial class OrganizationSelectionDialog : Window | ||
| { | ||
| public OrganizationSelectionDialog(IReadOnlyList<OrganizationDisplay> organizations) | ||
| public OrganizationSelectionDialog(IConnectedModeServices connectedModeServices, ICredentialsModel credentialsModel) |
There was a problem hiding this comment.
I think it was simpler when it was just accepting a static list of connections, but I also get the point of moving the responsibility here
There was a problem hiding this comment.
Simpler, maybe. But that means we would have to load the connections from the dialog, which would violate the MVVM pattern: the view should have no knowledge about the Model (slCore in our case)
| @@ -52,7 +52,8 @@ private void EditConnection_Clicked(object sender, RoutedEventArgs e) | |||
|
|
|||
| private void NewConnection_Clicked(object sender, RoutedEventArgs e) | |||
| { | |||
| if (GetTransientConnection() is {} transientConnection && CredentialsDialogSucceeded(transientConnection) && GetCompleteConnection(transientConnection) is {} completeConnection) | |||
| if (GetTransientConnection() is {} partialConnection && GetCredentialsDialog(partialConnection) is {} credentialsDialog && | |||
There was a problem hiding this comment.
- formatting is incorrect here. 1 part of boolean expression per line, line breaks before operators.
- GetCredentialsDialog can never return null, so the condition feels a bit hacky. At this point I would convert this
if (a && b && c && d)into
if(!a) return;
var dialog = b();
if(!c) return;
if(connection.Type == SonarCloud && !d) return;
viewModel.add()
This PR targets the [load organizations PR](#5629) and shows the loaded in the UI
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| } | ||
|
|
||
| var credentialsDialog = GetCredentialsDialog(transientConnection); | ||
| if (!CredentialsDialogSucceeded(credentialsDialog) || GetCompleteConnection(transientConnection, credentialsDialog) is not { } completeConnection) |
There was a problem hiding this comment.
at this point I would extract the server type check from GetCompleteConnection and rename it to FinalizeSonarCloudConnection or smth like that
There was a problem hiding this comment.
I would rather leave it like this. I think it is more readable and less cognitive complexity. It is, in the end, related to the final connection.
Would you want me to rename the GetCompleteConnection to FinalizeConnection? I see that SlCore also has the Transient connection vs full connection. So for me seems fine to stay as it is.
There was a problem hiding this comment.
I renamed the method
There was a problem hiding this comment.
- it hides the fact that this method is only really needed to be called for SC connections, which just makes it less clear why we call it for SQ
- both of these connections are transitive, as they need to be saved in the repository first and then synced with SLCore to be non-transitive
There was a problem hiding this comment.
I don't think the caller needs to know that is only needed for SC. It simply needs the final connection. Adding a (a || (b&&c) ) makes the if more complex, therefore I think the logic should be part of another method
|
gabriela-trutan-sonarsource
merged commit f69a48e
into
feature/new-connected-mode
SLVS-1394
This PR updates the ISlCoreConnectionAdapter to load the organizations for SonarCloud when creating a new connection.
Showing it in the UI will be a different PR