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

Disable Network Traffice When Notification Center Disabled #13286

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Disable Network Traffice When Notification Center Disabled #13286

merged 2 commits into from
Sep 2, 2022

Conversation

QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Sep 2, 2022 •
edited
Loading

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Disable Network Traffice When Notification Center Disabled
image

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

Release Notes

Disable Network Traffice When Notification Center Disabled

Reviewers

@DynamoDS/dynamo

FYIs

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

QilongTang
QilongTang commented Sep 2, 2022
View reviewed changes
src/Notifications/NotificationCenterController.cs
@@ -23,11 +20,11 @@ namespace Dynamo.Notifications
{
[ClassInterface(ClassInterfaceType.AutoDual)]
[ComVisible(true)]
public class scriptObject
public class ScriptObject
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a name vialation, I think in C# public class are all capitalized

RobertGlobant20
RobertGlobant20 approved these changes Sep 2, 2022
View reviewed changes
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

src/Notifications/NotificationCenterController.cs
// This ensures no network traffic when Notification center feature is turned off
if (dynamoViewModel.PreferenceSettings.EnableNotificationCenter)
{
RequestNotifications();
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know the RequestNotifications(); only populates the number of notifications in the bell icon but this won't prevent that the webapp consume the REST API (not sure if that is the expected behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RobertGlobant20 This will prevent Dynamo making the REST API calls, do you mean the WebApp will still do it?

Copy link
Member

Choose a reason for hiding this comment

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

what about simply not loading the extension?

Copy link
Contributor

Choose a reason for hiding this comment

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

@QilongTang if the objective is to prevent just Dynamo to make REST API calls then is OK but if you want also to prevent those calls in the WebApp then is a different story.

Copy link
Contributor Author

@QilongTang QilongTang Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@RobertGlobant20 @mjkkirschner My understanding is that the latest version of Notification Center is using process.env to control the endpoint and REST API calls will be made depending on if the endpoint address exist or not. When consumed by Dynamo, the end point is empty. I believe @filipeotero made such change so when used in Dynamo, the WebApp itself should not pull anything but rely on info passed by Dynamo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@filipeotero Please let me know if my understanding is correct. If not, then happy to follow up

Copy link
Contributor

Choose a reason for hiding this comment

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

@QilongTang You are correct. The webApp itself will not request notifications if process.env.NOTIFICATION_URL is not set and it is empty when the app is consumed by dynamo. Dynamo calls the endpoint and passes notifications to window.setNotifications.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct. In Dynamo, the notification center is waiting to push the notifications. Executing the notification center in development mode will grab the notifications by itself.

@QilongTang QilongTang merged commit b01e46d into master Sep 2, 2022
@QilongTang QilongTang deleted the NotificationCenterDisable branch September 2, 2022 19:07
QilongTang added a commit that referenced this pull request Sep 2, 2022
@QilongTang
Disable Network Traffice When Notification Center Disabled (#13286)
56b0938 
* update

* Remain public
@QilongTang QilongTang mentioned this pull request Sep 2, 2022
Merged
8 tasks
QilongTang added a commit that referenced this pull request Sep 2, 2022
@QilongTang
Disable Network Traffice When Notification Center Disabled (#13286) (#…
86b9a03 
…13287)

* update

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

@RobertGlobant20 RobertGlobant20 RobertGlobant20 approved these changes

@avidit avidit avidit left review comments

@mjkkirschner mjkkirschner mjkkirschner left review comments

@filipeotero filipeotero Awaiting requested review from filipeotero

@reddyashish reddyashish Awaiting requested review from reddyashish

@zeusongit zeusongit Awaiting requested review from zeusongit

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
@QilongTang @mjkkirschner @avidit @RobertGlobant20 @filipeotero