Skip to content

Conversation

@dnenov
Copy link
Collaborator

@dnenov dnenov commented Oct 18, 2023

Purpose

Complete rehaul of the Package Manager publishing package experience. WIP, a lot of items are still moving on this PR!

UI Flow

WIP publishing

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

- now all colors are controlled by the COLORS object in confing.js
- correctly creates minimum depth folder structure for the files added
- updated browser to only show root items
- added pages for the wizard-like experience when publishing a package
- using Dispatcher fixes the async tree creation
- tests added to assert correct functionality of collecting files and folders
- updated files and folders icons
- now only allows LibraryNode checkboxes for Assembly dll files
- started the structure for preview build
- added tests for the core methods of PreviewPackageBuild
- changed `RemoveItem` method to account for folder items being removed
- fixed a bug where removing an Assembly file would generate an error. Assembly files can also be added to 'additional items'
- UI added to allow users to add/remove items from current Package selection
- preview contents now correctly display based on user choice (retain folder or not)
- fixed a bug where CustomDefinitions would read as root item
- Pages are disabled when not displayed, in order to stop handling of tasks that affect other Pages
- now sorts browser alphabetically
- fixed disabled behavior
- assemblies now will show up with their file path, but still get picked up from their assembly resource on disk
- we need the ability to display custom nodes as file with file paths during package creation. CustomDefinition does not have the attributes to address that, so we are adding a new 'preview' item type to server the purpose
- added tests for delete item
- fixed an issue where removing all items would not result in cleaning the RootContent items
- added detailed description for the customTreeView_SelectedItemChanged method
- fixed checkbox behavior to be consistent when interacting with the rest of the controls
- clearing data and ui after publishing
- clearing data and ui with Cancel button
- finished the main flow between the pages
- publish local retaining folder structure
- now warns the user of losing changes if using cancel or navigating away
- also clear custom definition filepaths
@reddyashish
Copy link
Contributor

reddyashish commented Nov 13, 2023

@dnenov The dynamo icon is added in multiple locations under the test folder. Any reason to do that? Better to reuse only 1 icon.

- added a detailed comment describing the new PublishRetainFolderStructure method
- removed duplicate icon.png files / replaced with empty text files instead
- updated tests that were failing since last changes
@dnenov
Copy link
Collaborator Author

dnenov commented Nov 14, 2023

@dnenov The dynamo icon is added in multiple locations under the test folder. Any reason to do that? Better to reuse only 1 icon.

I removed the icons, by replacing them with empty text files for the purposes of the Tests. I also had to rework a few tests that were now failing because of recent changes.

InitializeComponent();

this.DataContextChanged += PublishFinishPage_DataContextChanged;
this.Tag = "Publish Finished Page";
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this to the resources, so that it can be localized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just an internal Tag, it's not meant to be shown on UI, I don't think we should localize Tags.

@reddyashish
Copy link
Contributor

4 failures and 1 conlict
Screenshot 2023-11-14 at 3 51 44 PM

- the tests were failing because another test folder was starting with a preceding alphabet letter
@dnenov
Copy link
Collaborator Author

dnenov commented Nov 15, 2023

4 failures and 1 conlict Screenshot 2023-11-14 at 3 51 44 PM

I was able to fix the tests that were failing because of a test folder I introduced. The issue was that I had created a folder with a folder name starting with a preceding letter in alphabetical order, which was causing an error in the affected tests.
However, it's difficult to test the remaining ones as they are failing in my local setup. Let's rerun and see if this change might have fixed all of the failing regressions 🤞🏾
image

this.Package = null;
this.CustomNodeDefinitions = new List<CustomNodeDefinition>();
RaisePropertyChanged("PackageContents");
//System.Windows.Application.Current.Dispatcher.Invoke(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code if not using.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I will do that in the other PR.

SubmitCommand.RaiseCanExecuteChanged();
PublishLocallyCommand.RaiseCanExecuteChanged();
});
// Can we try commenting out the can execute?
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this commented code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I was hoping we discuss this before committing to this change.

@reddyashish
Copy link
Contributor

@dnenov Can you address the last 2 comments in the other package manager PR that you have open. Merging this for now as all tests have passed and want to get some manual testing on the package upload functionality.

@reddyashish reddyashish added this to the 3.0 milestone Nov 16, 2023
@reddyashish reddyashish merged commit 803d8e1 into DynamoDS:master Nov 16, 2023

/// <summary>
/// Build a new version of the package and upload retaining folder structure
/// TODO: Should that be a separate method or an override? Break API ok?
Copy link
Member

Choose a reason for hiding this comment

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

@dnenov @reddyashish there is still a TODO in here.

{
var packageUploadHandle = new PackageUploadHandle(PackageUploadBuilder.NewRequestBody(package));

Task.Factory.StartNew(() =>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the recommended style any longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants