LIU-413: Add Past Sessions to DataIslandManager Graph UI #292
Conversation
…DIM. - Create the new class - Add basic table entries to see in the DIM. Currently not possible to do anything other than see the past sessions.
This comment was marked as duplicate.
This comment was marked as duplicate.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @myxie - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more unit tests for the PastSessionManager class to verify edge cases like invalid session directories and various file patterns. This will help ensure reliable behavior as the functionality expands.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -298,24 +298,95 @@ | |||
| }); | |||
| } | |||
|
|
|||
| function fillDmTable(sessions, tbodyEl, sessionLink, DimSessionLink, cancelBtnSessionId, deleteBtnSessionId, hashCode) { | |||
|
|
|||
| function loadPastSessions(serverUrl, tbodyEl, refreshBtn, selectedNode, delay) { | |||
There was a problem hiding this comment.
suggestion: loadPastSessions duplicates significant code from loadSessions
Consider extracting common setup code into a shared helper function to reduce duplication
function loadSessionsBase(serverUrl, tbodyEl, refreshBtn, selectedNode, delay, isPast = false) {
// Common setup code goes here
}
function loadPastSessions(serverUrl, tbodyEl, refreshBtn, selectedNode, delay) {
return loadSessionsBase(serverUrl, tbodyEl, refreshBtn, selectedNode, delay, true);
}
function loadSessions(serverUrl, tbodyEl, refreshBtn, selectedNode, delay) {
return loadSessionsBase(serverUrl, tbodyEl, refreshBtn, selectedNode, delay, false);
}
There was a problem hiding this comment.
This is a valid comment, but I think there is more risk in the refactor than duplicating the code, especially given our interest in moving way from a DALiuGE-based web interface.
There was a problem hiding this comment.
Thank you for your feedback, we will generate more comments like this in the future according to the following instructions:
- Include comments that identify code duplication and suggest refactoring to improve maintainability.
- Suggest extracting common code into helper functions when appropriate to reduce redundancy.
- Provide clear examples or code snippets to illustrate the suggested changes.
| past_sessions = [ | ||
| path for path in self._work_dir.iterdir() | ||
| if (path.is_dir() | ||
| and path.name not in excluded_sessions | ||
| and self._is_session_dir(path)) | ||
| ] |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| list of past sessions. | ||
| """ | ||
|
|
||
| return any([expected_ext in f.name for f in session.iterdir()]) |
There was a problem hiding this comment.
suggestion (code-quality): Replace unneeded comprehension with generator (comprehension-to-generator)
| return any([expected_ext in f.name for f in session.iterdir()]) | |
| return any(expected_ext in f.name for f in session.iterdir()) |
daliuge-engine/dlg/manager/rest.py
Outdated
| pastSessions = [] | ||
| for pastSession in self.dm.getPastSessionIds(): | ||
| pastSessions.append( | ||
| {"sessionId": pastSession} | ||
| ) | ||
| return pastSessions |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Convert for loop into list comprehension (
list-comprehension) - Inline variable that is immediately returned (
inline-immediately-returned-variable)
| pastSessions = [] | |
| for pastSession in self.dm.getPastSessionIds(): | |
| pastSessions.append( | |
| {"sessionId": pastSession} | |
| ) | |
| return pastSessions | |
| return [ | |
| {"sessionId": pastSession} | |
| for pastSession in self.dm.getPastSessionIds() | |
| ] |
myxie
changed the title
Note: This should not be merged until after #291 has been merged; make sure the branch this is targeting is changed to
masterwhen doing so.JIRA Ticket
LIU-413
Type
Problem/Issue
To progress towards being able to see past sessions and re-run them based on the Graph Configuration, we want to be able to provide an interface where users can see previous sessions that have run.
Solution
This PR aims to partially address this by updating the existing DataIslandManager web interface and REST APi, and adding the PastSessionManager.
This is an introduction of basic functionality to start off the work to managing past sessions. There is future work planned to introduce a sessions database, local/remote GitHub integration etc., which will expand on the PastSessionManager.
The work introduced in this PR:
Checklist
[ ] Documentation addedSummary by Sourcery
Add functionality to manage and display past sessions in the web interface and enhance session management by supporting the dumping of physical graphs.
New Features:
Enhancements: