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.

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

Jump to bottom

Git extension integration in Repository Management. #3896

Merged
merged 63 commits into from
Oct 2, 2024
Merged

Git extension integration in Repository Management. #3896

merged 63 commits into from
Oct 2, 2024

Conversation

dhoehna
Copy link
Contributor

@dhoehna dhoehna commented Sep 23, 2024 •
edited
Loading

Summary of the pull request

2 main changes

  1. Adding Git Extension support to the Repository Management
  2. Fully implementing all actions on the Repository Management page.
  3. Added the ability to select a source control provider via a toggle only visible on the dev build.

Minor changes

  1. Localization
  2. Filtering
  3. Sorting
  4. Commit information
  5. Added another assert to a test.

Known Issues

  1. Columns are misaligned with the headers

A note about the TODOs.
All TODOs should be finished in my next PR. The next PR is about enabling database migration.

References and relevant issues

Detailed description of the pull request / Additional comments

A note about the massive number of commits.
I started development of this branch while #3836 was in review. Their commits are also in this one.

The commit 3c24c6a merges main into this branch, this includes the changes from #3836.

All file changes are from this PR.

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

Movies!
Repository Actions!
https://github.com/user-attachments/assets/9009d906-04ab-4bd9-b492-5a020d96416f

Filtering and sorting!

FilterAndSort.mp4

Picture!
Users can now select a source control provider and it is toggleable!
https://github.com/user-attachments/assets/695232ce-6d0c-45e6-b9f1-fe7b4cecc896
Pay no mind to the un aligned columns. They will be fixed in another PR.

Correct picture of the toggle name and description.
image

dhoehna and others added 30 commits August 13, 2024 12:23
@dhoehna @krschau
Refining Repository Management's UI. (#3557)
fc754c1 
* Adding the Repository tool

* WIP

* View is set up.  Test ata as well.

* Aligning columns.  Adding column spacing.

* Reverting this change

* Update tools/RepositoryManagement/DevHome.RepositoryManagement/Strings/en-us/Resources.resw

Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>

* Adressing comments

* Revert "Reverting this change"

This reverts commit 711dd78.

* Moving to experimental

* Disabling by default

* Removing from experimental.  Addressing comments.

* Reverting to main

* Update tools/RepositoryManagement/DevHome.RepositoryManagement/DevHome.RepositoryManagement.csproj

Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>

* Update tools/RepositoryManagement/DevHome.RepositoryManagement/Extensions/ServiceExtensions.cs

Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>

* Removing duplicate xaml code

* Adding to the mermaid diagram

* Got lost in the shuffle

* Removing a refrence

---------

Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>
@dhoehna
Merge branch 'main' into user/dhoehna/RepositoryManagementFeature
e52afbf
@dhoehna
Adding EF core, database project, and some usage. (#3674)
7d00a02 
* WIP

* EF works.  Can save data.

* Can read and write. :)

* Repos are added when cloned.

* Putting save/load into a different class

* Adding default values

* Cleaning up names and using statements

* More comments

* Removing refrence to the public nuget

* Restoring the nuget config

* Adding a test.  Better defining dates

* Adding some tests

* More comments.

* Better path comparison.

* Removing unused equals code

* Adding more comments.  Making new migrations
@dhoehna
Merge branch 'main' into user/dhoehna/RepositoryManagementFeature
cd2f925
@dhoehna
Merge branch 'user/dhoehna/RepositoryManagementFeature' of https://gi…
6775301 
…thub.com/microsoft/devhome into user/dhoehna/RepositoryManagementFeature
@dhoehna
Moving this to experimental
9d77f68
@dhoehna
WIP
17cc488
@dhoehna
Some telemetry and logging
66f6978
@dhoehna
THings build again.
8606981
@dhoehna
Returning this back
241bcee
@dhoehna
I believe I am understanding this.
234af67
@dhoehna
Can update
038c65b
@dhoehna
Almost done with open in FE
e45e669
@dhoehna
Try/Catching everything. Open in FE should be done.
0d39cd6
@dhoehna
Open in CMD works. :)
d6f4f44
@dhoehna
Merge branch 'main' into user/dhoehna/ImplementingTheRepositoryActions
e3b552e
@dhoehna
Re-adding changes lost due to merge
1d628c7
@dhoehna
Getting saving straightened out.
d23a431
@dhoehna
Merge branch 'main' into user/dhoehna/ImplementingTheRepositoryActions
6b7b72e
@dhoehna
WIP
d73857d
@dhoehna
Move Repository done.
87a1a38
@dhoehna
Removing 1:1. Finishing RemoveFromList
5395f4c
@dhoehna
Adding _items
022977a
@dhoehna
Adding configuration file information to the repository table
7fe619c
@dhoehna
Opening the configuration file location in FE
5655157
@dhoehna
Deleting a repository.
2ef3922
@dhoehna
Add to winget configuration file.
2c44c31
@dhoehna
Cleaning up the code
cfa8f9a
@dhoehna
Icon for a configuration file. UI updates when hiding a repo
0c00fca
@dhoehna
All the buttons do something.
4135ec4
jsidewhite
jsidewhite approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@jsidewhite jsidewhite left a comment

Choose a reason for hiding this comment

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

:shipit:

DefaultRyan
DefaultRyan previously requested changes Sep 28, 2024
View reviewed changes
Copy link
Member

@DefaultRyan DefaultRyan left a comment

Choose a reason for hiding this comment

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

The decision to associate the folder with the first LocalRepository provider we see is a non-starter. If there is only a single provider installed, it's not as crucial, but in all other cases the user needs the choice to select which provider. Leaving this up to chance was an extremely disriptive bug that was already fixed earlier. Please don't regress this part.

tools/RepositoryManagement/DevHome.RepositoryManagement/Services/RepositoryEnhancerService.cs Outdated Show resolved Hide resolved
tools/RepositoryManagement/DevHome.RepositoryManagement/Services/RepositoryEnhancerService.cs Outdated Show resolved Hide resolved
tools/RepositoryManagement/DevHome.RepositoryManagement/Services/RepositoryEnhancerService.cs Outdated Show resolved Hide resolved
tools/RepositoryManagement/DevHome.RepositoryManagement/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs Outdated Show resolved Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs Show resolved Hide resolved
...epositoryManagement/DevHome.RepositoryManagement/Views/RepositoryManagementMainPageView.xaml Outdated Show resolved Hide resolved
@dhoehna dhoehna dismissed DefaultRyan’s stale review October 2, 2024 19:59

Changes are in this PR.

@vineeththomasalex
Copy link
Contributor

vineeththomasalex commented Oct 2, 2024 •
edited
Loading

The decision to associate the folder with the first LocalRepository provider we see is a non-starter. If there is only a single provider installed, it's not as crucial, but in all other cases the user needs the choice to select which provider. Leaving this up to chance was an extremely disriptive bug that was already fixed earlier. Please don't regress this part.

@DefaultRyan Reminder to check that your concerns were addressed when you're back.

Your blocking comment seems to be addressed in fa22fce

vineeththomasalex
vineeththomasalex reviewed Oct 2, 2024
View reviewed changes
src/App.xaml.cs
@@ -100,9 +100,6 @@ public App()
}).
ConfigureServices((context, services) =>
{
// Add databse connection
services.AddDatabase(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a duplicate from somewhere else?

vineeththomasalex
vineeththomasalex reviewed Oct 2, 2024
View reviewed changes
test/Database/RepositoryTests.cs

foreach (var repositoryDirectory in Directory.GetDirectories(_repositoryCloneLocation, "*", SearchOption.AllDirectories).Reverse())
{
Directory.Delete(repositoryDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

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

This might benefit from a "Try-catch" since the deletion of each file/folder might fail, and shouldn't fail the reset() and should proceed to the next item. Maybe log failure and proceed is best, since any dependent tests will indicate if it's a problem.

vineeththomasalex
vineeththomasalex reviewed Oct 2, 2024
View reviewed changes
tools/RepositoryManagement/DevHome.RepositoryManagement/Strings/en-us/Resources.resw
<value>More options for repository {0}</value>
<comment>{Locked="{0}"} {0} name of the repository. Automation name for the ... menu</comment>
</data>
<data name="MinuteAbbreviation" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't fine where this is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just found it. Should it be "mins" ?

vineeththomasalex
vineeththomasalex reviewed Oct 2, 2024
View reviewed changes
settings/DevHome.Settings/Strings/en-us/Resources.resw
<value>Toggle to allow tester to change the source control provider.</value>
</data>
<data name="RepositoryManagementSourceControlSelector_Name" xml:space="preserve">
<value>Show Souce Control Provider</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>Show Souce Control Provider</value>
<value>Show Source Control Provider</value>

vineeththomasalex
vineeththomasalex approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@vineeththomasalex vineeththomasalex left a comment

Choose a reason for hiding this comment

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

Don't see anything blocking. LGTM!

@dhoehna dhoehna merged commit 3b8cba0 into main Oct 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krschau krschau krschau approved these changes

@vineeththomasalex vineeththomasalex vineeththomasalex approved these changes

@jsidewhite jsidewhite jsidewhite approved these changes

@DefaultRyan DefaultRyan DefaultRyan left review comments

@manodasanW manodasanW Awaiting requested review from manodasanW

@guimafelipe guimafelipe Awaiting requested review from guimafelipe

@huzaifa-d huzaifa-d Awaiting requested review from huzaifa-d

@dkbennett dkbennett Awaiting requested review from dkbennett

@jaholme jaholme Awaiting requested review from jaholme

@dabhattimsft dabhattimsft Awaiting requested review from dabhattimsft

@EricJohnson327 EricJohnson327 Awaiting requested review from EricJohnson327

@sshilov7 sshilov7 Awaiting requested review from sshilov7

@hart-wilson hart-wilson Awaiting requested review from hart-wilson

@zadesai zadesai Awaiting requested review from zadesai

@SohamDas2021 SohamDas2021 Awaiting requested review from SohamDas2021

@AmelBawa-msft AmelBawa-msft Awaiting requested review from AmelBawa-msft

@bbonaby bbonaby Awaiting requested review from bbonaby

@srupakula srupakula Awaiting requested review from srupakula

@ssparach ssparach Awaiting requested review from ssparach

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhoehna @vineeththomasalex @jsidewhite @DefaultRyan @krschau