-
Notifications
You must be signed in to change notification settings - Fork 53
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
test Demo #4701
test Demo #4701
Changes from all commits
c9a9131
120052b
efbf3ad
1eeb31f
0fefcc5
a77f5f8
f0485e9
9d00ed3
57ecc7d
a460b3c
34332b3
aaefe0a
a5e1301
2f9cdc4
f47b4df
a5a74e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import QtQuick 2.15 | ||
import QtQuick.Controls 2.15 | ||
import QtQuick.Layouts 1.15 | ||
import QtQuick.Window 2.15 | ||
import "icon-font/Icon.js" as MdiFont | ||
|
||
|
||
|
||
Rectangle { | ||
id: taskManager | ||
anchors.fill: parent | ||
|
||
property string engineUid: "" | ||
property int activeSessionsCount: 0 | ||
property int completedSessionsCount: 0 | ||
property double startTime: 0.0 | ||
|
||
signal setEngine(string uid) | ||
|
||
onSetEngine: { | ||
engineUid = uid | ||
} | ||
|
||
TabBar { | ||
id: bar | ||
width: parent.width | ||
height: 50 | ||
spacing: 0 | ||
|
||
anchors.top: buttonzone.bottom | ||
|
||
SettingsTab { | ||
text: qsTr("Tasks") + tl.tr | ||
barIndex: bar.currentIndex; | ||
index: 0 | ||
anchors.top: parent.top | ||
} | ||
|
||
} | ||
|
||
Text { | ||
anchors.fill: parent | ||
horizontalAlignment: Qt.AlignHCenter | ||
verticalAlignment: Qt.AlignVCenter | ||
height: 100 | ||
/*textFormat: Text.RichText*/ | ||
text: api.get_title(engineUid) | ||
font.pointSize: point_size * 1.2 | ||
color: primaryText | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||||||
# flake8: noqa | ||||||||||
|
||||||||||
import json | ||||||||||
from dataclasses import asdict | ||||||||||
from logging import getLogger | ||||||||||
|
@@ -21,6 +23,7 @@ | |||||||||
TransferStatus, | ||||||||||
) | ||||||||||
from ..dao.engine import EngineDAO | ||||||||||
from ..engine.engine import Engine | ||||||||||
from ..exceptions import ( | ||||||||||
AddonForbiddenError, | ||||||||||
AddonNotInstalledError, | ||||||||||
|
@@ -305,6 +308,7 @@ | |||||||||
engine = self._manager.engines.get(uid) | ||||||||||
if engine: | ||||||||||
path = engine.local.abspath(Path(ref)) | ||||||||||
self.fetch_pending_tasks(engine) | ||||||||||
self.application.show_metadata(path) | ||||||||||
|
||||||||||
@pyqtSlot(str, result=list) | ||||||||||
|
@@ -457,6 +461,43 @@ | |||||||||
self.application.hide_systray() | ||||||||||
log.info(f"Show settings on section {section}") | ||||||||||
self.application.show_settings(section) | ||||||||||
|
||||||||||
@pyqtSlot(str) | ||||||||||
def show_tasks(self, section: str, /) -> None: | ||||||||||
self.application.hide_systray() | ||||||||||
log.info(f"Show Task Manager {section}") | ||||||||||
self.application.show_tasks(section) | ||||||||||
|
||||||||||
@pyqtSlot() | ||||||||||
def fetch_pending_tasks(self, engine: Engine, /) -> None: | ||||||||||
log.info("Check for pending approval tasks") | ||||||||||
endpoint = "/api/v1/task/" | ||||||||||
url = "http://localhost:8080/nuxeo" + endpoint | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Use f-string instead of string concatenation (
Suggested change
|
||||||||||
headers = { | ||||||||||
"Accept": "application/json", | ||||||||||
"Content-Type": "application/json", | ||||||||||
} | ||||||||||
try: | ||||||||||
# response = NuxeoClient.request(self,method="GET", path=endpoint, headers=headers, ssl_verify=Options.ssl_no_verify) | ||||||||||
response = requests.get( | ||||||||||
url=url, | ||||||||||
verify=True, | ||||||||||
timeout=3600, | ||||||||||
headers=headers, | ||||||||||
auth=("Administrator", "Administrator"), | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 issue (security): Hard-coded credentials detected. It's highly insecure to hard-code credentials within the codebase. Consider using environment variables or a secure vault service for storing credentials. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 issue (security): Hard-coded credentials detected. It's highly insecure to hard-code credentials within the codebase. Consider using environment variables or a secure vault service for storing credentials. |
||||||||||
) | ||||||||||
Comment on lines
+482
to
+488
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (performance): The |
||||||||||
log.info(f">>>>> response type: {type(response)}") | ||||||||||
data = response.json() | ||||||||||
log.info(f">>>> response: {data}") | ||||||||||
if data["resultsCount"] > 0: | ||||||||||
log.info("Send pending task notification") | ||||||||||
for task in data["entries"]: | ||||||||||
log.info(f">>>> task: {task}") | ||||||||||
log.info(f">>>> taskid: {task['id']}") | ||||||||||
engine.fetch_pending_task_list(task["id"]) | ||||||||||
Comment on lines
+492
to
+497
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (edge_case_not_handled): The |
||||||||||
|
||||||||||
except Exception: | ||||||||||
log.exception("Unable to fetch tasks") | ||||||||||
Comment on lines
+472
to
+500
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 suggestion (security): Hardcoding the URL and credentials within the
Comment on lines
+473
to
+500
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (code_refinement): The use of a hardcoded URL ( |
||||||||||
|
||||||||||
@pyqtSlot() | ||||||||||
def quit(self) -> None: | ||||||||||
|
@@ -576,7 +617,9 @@ | |||||||||
def _balance_percents(self, result: Dict[str, float], /) -> Dict[str, float]: | ||||||||||
"""Return an altered version of the dict in which no value is under a minimum threshold.""" | ||||||||||
|
||||||||||
result = {k: v for k, v in sorted(result.items(), key=lambda item: item[1])} | ||||||||||
result = { | ||||||||||
k: v for k, v in sorted(result.items(), key=lambda item: item[1]) | ||||||||||
} # noqa | ||||||||||
Comment on lines
+620
to
+622
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Replace identity comprehension with call to collection constructor (
Suggested change
ExplanationConvert list/set/tuple comprehensions that do not change the input elements into.Before# List comprehensions
[item for item in coll]
[item for item in friends.names()]
# Dict comprehensions
{k: v for k, v in coll}
{k: v for k, v in coll.items()} # Only if we know coll is a `dict`
# Unneeded call to `.items()`
dict(coll.items()) # Only if we know coll is a `dict`
# Set comprehensions
{item for item in coll} After# List comprehensions
list(iter(coll))
list(iter(friends.names()))
# Dict comprehensions
dict(coll)
dict(coll)
# Unneeded call to `.items()`
dict(coll)
# Set comprehensions
set(coll) All these comprehensions are just creating a copy of the original collection. |
||||||||||
keys = list(result) | ||||||||||
min_threshold = 10 | ||||||||||
data = 0.0 | ||||||||||
|
@@ -1099,8 +1142,47 @@ | |||||||||
except OSError: | ||||||||||
log.exception("Remote document cannot be opened") | ||||||||||
|
||||||||||
@pyqtSlot(str, str, str) | ||||||||||
def display_pending_task(self, uid: str, remote_ref: str, /) -> None: | ||||||||||
log.info(f"Should open remote document ({remote_ref!r})") | ||||||||||
try: | ||||||||||
engine = self._manager.engines.get(uid) | ||||||||||
if engine: | ||||||||||
Comment on lines
+1149
to
+1150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Use named expression to simplify assignment and conditional (
Suggested change
|
||||||||||
url = engine.get_task_url(remote_ref) | ||||||||||
log.info(f">>>> doc url: {url}") | ||||||||||
engine.open_remote(url=url) | ||||||||||
except OSError: | ||||||||||
log.exception("Remote document cannot be opened") | ||||||||||
|
||||||||||
@pyqtSlot(str, str, result=str) | ||||||||||
def get_remote_document_url(self, uid: str, remote_ref: str, /) -> str: | ||||||||||
"""Return the URL to a remote document based on its reference.""" | ||||||||||
engine = self._manager.engines.get(uid) | ||||||||||
return engine.get_metadata_url(remote_ref) if engine else "" | ||||||||||
|
||||||||||
@pyqtSlot(str, result=str) | ||||||||||
def get_title(self, uid: str, /) -> str: | ||||||||||
endpoint = "/api/v1/task/" | ||||||||||
url = "http://localhost:8080/nuxeo" + endpoint | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Use f-string instead of string concatenation (
Suggested change
|
||||||||||
headers = { | ||||||||||
"Accept": "application/json", | ||||||||||
"Content-Type": "application/json", | ||||||||||
} | ||||||||||
try: | ||||||||||
response = requests.get( | ||||||||||
url=url, | ||||||||||
verify=True, | ||||||||||
timeout=3600, | ||||||||||
headers=headers, | ||||||||||
auth=("Administrator", "Administrator"), | ||||||||||
) | ||||||||||
data = response.json() | ||||||||||
html = data["entries"][0] | ||||||||||
html = html["variables"] | ||||||||||
html = html["review_result"] | ||||||||||
return html if html else "No Results To Show" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Replace if-expression with
Suggested change
ExplanationHere we find ourselves setting a value if it evaluates toTrue , and otherwiseusing a default. The 'After' case is a bit easier to read and avoids the duplication of It works because the left-hand side is evaluated first. If it evaluates to |
||||||||||
|
||||||||||
except Exception: | ||||||||||
log.exception("Unable to fetch tasks") | ||||||||||
return "No Results Found" | ||||||||||
Comment on lines
+1164
to
+1187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code_refinement): The |
||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,6 +215,9 @@ | |
# Setup notification center for macOS | ||
if MAC: | ||
self._setup_notification_center() | ||
current_uid = self.engine_model.engines_uid[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): The addition of the task manager window and the direct fetching of pending tasks in the initialization and GUI setup methods introduces additional complexity to the codebase. This approach mixes UI management with the application's core logic and lacks abstraction for these new functionalities. To maintain code readability and ease future maintenance, consider encapsulating the task manager setup and task fetching logic into separate methods or classes. For instance, creating a |
||
engine = self.manager.engines[current_uid] | ||
self.api.fetch_pending_tasks(engine) | ||
|
||
# Application update | ||
self.manager.updater.appUpdated.connect(self.quit) | ||
|
@@ -259,6 +262,7 @@ | |
del self.settings_window | ||
del self.systray_window | ||
del self.direct_transfer_window | ||
del self.task_manager_window | ||
else: | ||
del self.app_engine | ||
|
||
|
@@ -327,6 +331,15 @@ | |
self.direct_transfer_window.setSource( | ||
QUrl.fromLocalFile(str(find_resource("qml", file="DirectTransfer.qml"))) | ||
) | ||
|
||
# Task Manager | ||
self.task_manager_window = CustomWindow() | ||
self.task_manager_window.setMinimumWidth(300) | ||
self.task_manager_window.setMinimumHeight(240) | ||
self._fill_qml_context(self.task_manager_window.rootContext()) | ||
self.task_manager_window.setSource( | ||
QUrl.fromLocalFile(str(find_resource("qml", file="TaskManager.qml"))) | ||
) | ||
|
||
flags |= qt.Popup | ||
else: | ||
|
@@ -342,6 +355,7 @@ | |
self.direct_transfer_window = root.findChild( | ||
CustomWindow, "directTransferWindow" | ||
) | ||
self.task_manager_window = root.findChild(CustomWindow, "taskManagerWindow") | ||
|
||
if LINUX: | ||
flags |= qt.Drawer | ||
|
@@ -360,6 +374,7 @@ | |
if self.manager.engines: | ||
current_uid = self.engine_model.engines_uid[0] | ||
engine = self.manager.engines[current_uid] | ||
# self.api.fetch_pending_tasks(engine) | ||
self.get_last_files(current_uid) | ||
self.refresh_transfers(engine.dao) | ||
self.update_status(engine) | ||
|
@@ -886,6 +901,14 @@ | |
} | ||
self._window_root(self.settings_window).setSection.emit(sections[section]) | ||
self._center_on_screen(self.settings_window) | ||
|
||
@pyqtSlot(str) | ||
def show_tasks(self, section: str, /) -> None: | ||
# Note: Keep synced with the TaskManager.qml file | ||
sections = { | ||
} | ||
self._window_root(self.task_manager_window) # .setSection.emit(sections[section]) | ||
self._center_on_screen(self.task_manager_window) | ||
|
||
@pyqtSlot() | ||
def show_systray(self) -> None: | ||
|
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.
issue (complexity): The new implementation introduces several areas that could be improved for better maintainability, security, and readability:
Increased Code Length and Complexity: The addition of multiple new methods and direct HTTP requests within these methods significantly increases the code's complexity. Consider abstracting the HTTP request logic into a separate method to reduce repetition and improve readability.
Direct HTTP Requests and Hardcoded Values: The code now directly performs HTTP requests with hardcoded URLs and credentials. This approach tightly couples your class with specific implementation details and reduces flexibility. It's also a security risk to have credentials hardcoded. Consider using environment variables or a configuration file for sensitive information and abstracting the HTTP logic to make the code more secure and adaptable.
Error Handling: The broad use of try-except blocks, especially catching a general
Exception
, can make debugging more challenging as it may obscure the source of errors. It's usually better to catch specific exceptions. Also, consider logging the errors or providing feedback in a way that aids in diagnosing issues without making the error handling overly complex.Repetition: There's noticeable repetition, especially in constructing HTTP request headers and handling responses. Adhering to the DRY (Don't Repeat Yourself) principle can make your code less verbose and easier to maintain. A single method to handle HTTP requests could simplify these operations and centralize changes needed in the future.
By addressing these points, the code can become more maintainable, secure, and easier to understand. Simplifying the error handling, removing hardcoded values, and reducing repetition are key steps in improving the overall quality of the implementation.