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

NXDRIVE-2889: Display system notification for document review #4682

Conversation

swetayadav1
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 74.03846% with 27 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (wip-NXDRIVE-2873-Handle-workflow-in-drive@15d68a2). Click here to learn what that means.

Files Patch % Lines
nxdrive/gui/application.py 14.28% 12 Missing ⚠️
nxdrive/engine/engine.py 37.50% 5 Missing ⚠️
nxdrive/notification.py 42.85% 4 Missing ⚠️
nxdrive/poll_workers.py 78.94% 4 Missing ⚠️
nxdrive/client/workflow/__init__.py 97.43% 1 Missing ⚠️
nxdrive/gui/api.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@                             Coverage Diff                              @@
##             wip-NXDRIVE-2873-Handle-workflow-in-drive    #4682   +/-   ##
============================================================================
  Coverage                                             ?   37.63%           
============================================================================
  Files                                                ?       95           
  Lines                                                ?    15801           
  Branches                                             ?        0           
============================================================================
  Hits                                                 ?     5946           
  Misses                                               ?     9855           
  Partials                                             ?        0           
Flag Coverage Δ
unit 37.63% <74.03%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @swetayadav1 - I've reviewed your changes and they look great!

General suggestions:

  • Consider using a logging framework instead of print statements for better control over logging levels and outputs.
  • Implement the actual logic for fetching pending tasks to replace the placeholder return value.
  • Refactor to reduce complexity and improve modularity, possibly by encapsulating workflow-related logic into a separate class or module.
  • Ensure that the new feature is thoroughly tested, especially the interaction between the workflow management system and the GUI components.
Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 3 issues found
  • 🟢 Docstrings: all looks good

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -216,6 +218,10 @@
if MAC:
self._setup_notification_center()

# Initiate workflow when drive starts
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (complexity): The addition of workflow initiation logic directly into the class's startup sequence increases the complexity of the class by introducing new dependencies and tightly coupling the workflow management with the class's initialization process. This makes the code less modular and potentially harder to maintain or refactor in the future.

Consider encapsulating the workflow-related logic into a separate class or module, which can then be invoked in a more decoupled manner. For example, creating a WorkflowManager class responsible for all workflow operations could simplify the main class and adhere to the Single Responsibility Principle. This approach would make the codebase more maintainable and modular, allowing for easier future modifications and testing of the workflow logic independently from the class initialization.

@@ -165,3 +165,36 @@
self.manager.application.quit()

return True


class WorkflowWorker(PollWorker):
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (complexity): The introduction of the WorkflowWorker class adds significant complexity to the codebase for several reasons:

  1. Increased Length and Dependencies: The new class increases the overall length of the code and introduces additional dependencies on the manager and its properties. This makes the system more complex to understand and maintain.

  2. State Management: The use of _first_workflow_check to manage the initial workflow check introduces stateful behavior, which can complicate debugging and maintenance due to the object's behavior depending on its history.

  3. Complex Polling Logic: The logic within the _poll method, with its conditional checks and operations, adds complexity. Understanding this requires a deep knowledge of the application's state and the manager's state.

  4. Tight Coupling: The class is tightly coupled to the manager and indirectly to other components it manages. This tight coupling can make future modifications more challenging as changes in one part of the system might affect this class.

Consider simplifying the WorkflowWorker class by reducing its responsibilities or by streamlining its interactions with other components. Simplifying state management and reducing tight coupling where possible can make the code easier to maintain and understand.

swetayadav1 and others added 22 commits March 19, 2024 01:24
@swetayadav1 swetayadav1 changed the base branch from master to wip-NXDRIVE-2873-Handle-workflow-in-drive April 16, 2024 05:53
@swetayadav1 swetayadav1 deleted the wip-NXDRIVE-2889-Display-notification-for-document-review branch May 16, 2024 12:37
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.

1 participant