-
Notifications
You must be signed in to change notification settings - Fork 668
DYN-6409 [Approach 2]: Disable PM requests with no-network mode #16238
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6409
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I would vote for this approach, looks and works better IMO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements an offline (no-network) mode by propagating a NoNetworkMode flag through sessions, HTTP clients, core nodes, and UI view models to disable network operations when Dynamo is offline.
- Introduce a
NoNetworkModesession parameter and a helper/HTTP handler to intercept and block outgoing requests. - Inject the custom handler into the PackageManager client and pass the offline flag to disable PM operations.
- Update core nodes and view models to guard against network calls and disable related UI commands.
Reviewed Changes
Copilot reviewed 23 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/NodeServices/ExecutionSession.cs | Add NoNetworkMode key, ThrowIfNoNetworkMode helper, and a NoNetworkModeHandler |
| src/Libraries/CoreNodes/Web.cs | Guard WebRequestByUrl with ThrowIfNoNetworkMode |
| src/Libraries/CoreNodeModels/WebRequest.cs | Refactor NodeDescription attribute to use nameof |
| src/DynamoPackages/PackageManagerExtension.cs | Inject HttpClient with NoNetworkModeHandler, pass offline flag |
| src/DynamoPackages/PackageManagerClient.cs | Store NoNetworkMode, update constructor, clean up field usage |
| src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs | Disable publish commands when offline |
| src/DynamoCoreWpf/ViewModels/Menu/PreferencesViewModel.cs | Expose NoNetworkMode property |
| src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs | Disable package manager menu commands when offline |
| src/DynamoCore/Models/DynamoModel.cs | Update documentation for NoNetworkMode |
| src/DynamoCore/Configuration/ExecutionSession.cs | Initialize NoNetworkMode in session parameters |
Files not reviewed (10)
- src/DynamoCore/DynamoCore.csproj: Language not supported
- src/DynamoCoreWpf/DynamoCoreWpf.csproj: Language not supported
- src/DynamoCoreWpf/Views/Menu/PreferencesView.xaml: Language not supported
- src/DynamoMLDataPipeline/DynamoMLDataPipeline.csproj: Language not supported
- src/DynamoPackages/DynamoPackages.csproj: Language not supported
- src/DynamoPackages/Properties/Resources.Designer.cs: Language not supported
- src/DynamoPackages/Properties/Resources.en-US.resx: Language not supported
- src/DynamoPackages/Properties/Resources.resx: Language not supported
- src/NodeServices/DynamoServices.csproj: Language not supported
- src/NodeServices/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (5)
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs:455
- The previous check for
Selection.Count > 0was removed, causing this to return true when the selection is empty (sinceAllis vacuously true). Reintroduce the count check to prevent publishing with no selected nodes.
return DynamoSelection.Instance.Selection.All(x => x is Function) && AuthenticationManager.HasAuthProvider && !Model.NoNetworkMode;
src/NodeServices/ExecutionSession.cs:2
- [nitpick] Remove the unused
System.Collections.Genericusing directive to reduce clutter.
using System.Collections.Generic;
src/Libraries/CoreNodes/Web.cs:16
- Add a unit test to verify that
WebRequestByUrlthrows the expectedInvalidOperationExceptionwith the offline warning whenNoNetworkModeis enabled.
ExecutionSessionHelper.ThrowIfNoNetworkMode();
src/NodeServices/ExecutionSession.cs:132
- Add a unit test for
NoNetworkModeHandlerto confirm thatSendAsyncreturns HTTP 503 with the expected JSON payload and reason phrase.
var json = "{\"success\":false,\"message\":\"Application is in offline mode\",\"content\":null}";
src/Libraries/CoreNodes/Web.cs:13
- Add XML documentation to indicate that this method throws
InvalidOperationExceptionwhen running in offline mode viaThrowIfNoNetworkMode().
public static string WebRequestByUrl(string url)
reddyashish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Purpose
This approach is similar to #16191, except for initializing a custom HTTP client that returns a
service unavailableresponse and injecting that into the PM client for all requests.This depends on the version of PM client that has the changes in DynamoDS/PackageManagerClient#116.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. 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