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 multi user extension login for setup environment and create environment flows #3907

Merged

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented Sep 28, 2024

Summary of the pull request

Update setup target and create environment flows to allow an extension with multiple logged in developerIds to show their environments. Before this change both places use to use the first DeveloperId it find from the extension to query for environments. This caused an issue for extensions like the azure extension where the user might be logged in using a work or school account as well as a personal MSA account.

Changes:

  • noticed that we remove the event handlers in DevHomeChoiceSetWithDynamicRefresh.cs when the combo boxes unload. But we don't add them back on load which is probably how this issue [Environments reliability] Dev Box operations: When I switch between different Projects for Dev Box creation, the Pool selection is not updated #3638 occurred. So I updated it to prevent that from happening.
  • noticed the text were too small in the adaptive card. Updated the default text size in the host config to be size 14 which looks like its also the default for win ui in general.
  • For the setup environment flow:
    • In ComputeSystemsListViewModel we now filter out compute systems that don't support environments in its constructor.
    •  Updated SetupTargetViewModel.cs's UpdateListViewModelList method to create a ComputeSystemsListViewModel for each developerId that returns a list of compute systems.
    • Added parentheses over the alternate title for the compute system when it appears in the UI so it looks like it does in the environments page.
  • For the create environment flow:
    • I removed the booleans which were used to update the state of the page e.g IsAdaptiveCardSessionLoaded to be enums, now that there are multiple states the initial creation page that shows the adaptive card can be in. A converter was created to allow controls to be visible/collapsed depending on the state of the initial creation page.
      • Note: For background on how the create environment flow works. There is only one ExtensionAdaptiveCardSession2 object. The EnvironmentCreationOptionsViewModel that holds it sends the adaptive card to different views depending on the state of the flow.
    • Added a Combo box so the user can select an account

Video of me showing how the create environment flow works with multiple users logged into the Azure extension:

Video.showing.multi.user.flow.when.an.extension.has.multiple.developerIds.mp4

Video of me showing that users can now see all their dev boxes when multiple users are logged in. In this scenario I don't have access to DevBoxes on my secondary msa, but you can see that doesn't prevent us from showing others

video.showing.multi.account.behavior.for.setup.environment.flow.mp4

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

@@ -48,8 +48,6 @@ public partial class ComputeSystemsListViewModel : ObservableObject

public AdvancedCollectionView ComputeSystemCardAdvancedCollectionView { get; private set; }

public Dictionary<DeveloperIdWrapper, ComputeSystemsResult> DevIdToComputeSystemMap { get; set; }

public ComputeSystemProvider Provider { get; set; }

public DeveloperIdWrapper CurrentDeveloperId { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we handle logout already? If we're reloading the environments on page load, we're probably fine, since the user will have to move to a different page to logout. If not, we might need to listen to DevId logout event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good call out we'd probably need listen for the event and refresh the page accordingly. I'll create an issue for that since we probably don't want this PR getting too big

@bbonaby bbonaby merged commit 70b3d83 into main Oct 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants