Consistent jobstore, GC, montage refactoring #45
Conversation
| $(this).hide(); | ||
| var elem = $(this); | ||
| var redir_url = $(this).data("urllink"); | ||
| var ss_id = $(this).data("ss_id"); |
There was a problem hiding this comment.
It's better to use const instead of var whenever possible.
| var show_montage = elem.children('.show-montage'); | ||
| var start_montage = elem.children('.start-montage'); | ||
|
|
||
| $.ajax({ |
| elem.data('ss_locked', "False"); | ||
| elem.css("background", "#FFFFFF"); | ||
| elem.css("pointer-events", "auto"); | ||
| elem.data('ss_locked', "False"); |
There was a problem hiding this comment.
use jQuery chaining:
elem.data('ss_locked', "False")
.css("background", "#FFFFFF")
.css("pointer-events", "auto")
.data('ss_locked', "False")
or
elem.data('ss_locked', "False").css("background", "#FFFFFF").css("pointer-events", "auto").data('ss_locked', "False")
| start_montage.hide(); | ||
| show_montage.show(); | ||
| } else if (!data.isexists){ | ||
| elem.data('ss_locked', "False"); |
There was a problem hiding this comment.
use same type of quotes everywhere – either 'single' or "double"
| elem.data('ss_locked', "False"); | ||
| start_montage.hide(); | ||
| show_montage.show(); | ||
| } else if (!data.isexists){ |
There was a problem hiding this comment.
else would be enough here, because here it's always true that !data.isexists.
| @@ -90,11 +90,14 @@ def get_storage_capacity(self, path: str) -> (InternalOperationResult, int): | |||
| self.logger.warning('Can\'t get information about total disk capacity: %s', str(e)) | |||
| return InternalOperationResult(ExecutionStatus.FATAL_ERROR, str(e)), None | |||
|
|
|||
| def validate_file(self, file: str) -> bool: | |||
| def is_file_valid(self, file: str) -> bool: | |||
| return os.path.isfile(file) | |||
There was a problem hiding this comment.
Not sure we need this one-line function. The very straightforward os.path.isfile(file) is much more readable and common.
|
|
||
| for f in files: | ||
| size += self.get_file_size(path + '/' + f) | ||
| self.__sftp.remove(path + '/' + f) |
There was a problem hiding this comment.
Why can't we remove the whole directory without going through all the files in it one-by-one?
There was a problem hiding this comment.
Why can't we remove the whole directory without going through all the files in it one-by-one?
There is no way to remove the whole directory using SFTP. It is possible to remove directory just if it's empty - SFTP specific.
| @@ -522,7 +549,7 @@ def video_view(request, substep_id): | |||
| return response | |||
| except FileNotFoundError as e: | |||
| logger.warning('Missing file: %s', str(e)) | |||
| return error_description(request, 'File is missing.') | |||
| return error_description(request, 'File is missed.') | |||
There was a problem hiding this comment.
File is missing was correct 🤔
| if step_deleted: | ||
| for substep in substeps: | ||
| substep.delete() | ||
| step.delete() |
There was a problem hiding this comment.
Something is off with the variables naming here. The following code looks strange:
if step_deleted:
...
step.delete() # why we need to delete the deleted step?
There was a problem hiding this comment.
Something is off with the variables naming here. The following code looks strange:
if step_deleted: ... step.delete() # why we need to delete the deleted step?
Legacy naming. step_deleted indicates that step files successfully removed from FS. I'll fix it.
Consistent jobstore for task manager;
Tablet garbage collector (#37);
Asynchronously montage: using Ajax without opening new page, no need to refresh page to unlock steps(Ajax polling), ability to montage all subsets over step or lesson (party resolves #24).
Also contains some minor fixes:
Some synchronization tuning;
Fixed renaming bug - #9;
Paramiko update - #44.