Migrate core logic of importchannel to a utility function and update associated tasks #13099

thesujai · 2025-02-20T19:51:17Z

Summary

This refactors the importchannel command and its related tasks

Moved the channel transfer logic to a utility function(Which originally was inside the management function)
Updated the channel import tasks to use the utility function instead of calling the importchannel command
Moved import_channel_by_id to channel_import.py util where the function is suited to be

References

Fixes #13095
Subtask of #9266

Reviewer guidance

NA

…e module

…ing management commands

thesujai · 2025-02-20T19:54:31Z

kolibri/core/content/tasks.py

@@ -587,15 +581,11 @@ def diskimport(
    drive = get_mounted_drive_by_id(drive_id)


This line is duplicated at line 594. Is it a bug or is there any purpose for this?

I don't think it's a bug, but it definitely looks unnecessary the second time. Probably just copy pasted from somewhere else.

rtibbles · 2025-02-20T20:02:42Z

kolibri/core/content/utils/channel_transfer.py

+logger = logging.getLogger(__name__)
+
+
+def start_file_transfer(job, filetransfer, channel_id, dest, no_upgrade, contentfolder):


For both of these functions, I would like it if we did not pass in job as a first argument, and instead invoked get_current_job inside the function itself.

What you would then need is a dummy job in the case where the function is not running in a task runner context, that just does a noop for all the methods of the job class (this way you don't need to keep on checking if the job exists or not).

rtibbles · 2025-02-20T20:07:59Z

This makes sense so far, I'd want the test updated, and not just skipped, before we merge, and I think not passing in the job into the functions is preferable - because that will eventually allow us to phase out the AsyncCommand class (passing the self into the function as the job is depending on methods defined on the AsyncCommand).

thesujai · 2025-02-20T20:14:05Z

Thank you @rtibbles . I will update it by tomorrow:)

github-actions · 2025-02-20T20:23:51Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a2.dev0_git.47.g3c089e71.pex
Windows Installer (EXE)	kolibri-0.18.0a2.dev0+git.47.g3c089e71-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a2.dev0+git.47.g3c089e71-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a2.dev0+git.47.g3c089e71.dmg
Android Package (APK)	kolibri-0.18.0a2.dev0+git.47.g3c089e71-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.0a2.dev0+git.47.g3c089e71.zip
TAR file	kolibri-0.18.0a2.dev0+git.47.g3c089e71.tar.gz
WHL file	kolibri-0.18.0a2.dev0+git.47.g3c089e71-py2.py3-none-any.whl

thesujai · 2025-02-21T05:24:31Z

Updated the utils functions as well as importchannel tests

rtibbles

This is looking really excellent - great work on this. I'll ask our QA team to confirm the workflows are still working as expected, and we should be ready to merge!

rtibbles · 2025-02-27T00:56:19Z

@pcenov @radinamatic we would need to test reviewing a new channel listing from Studio, from disk, and also bulk importing a channel all at once, without viewing the listing.

pcenov

Hi @rtibbles and @thesujai - unfortunately it's not possible to import a channel or resources in any way. I'm getting a Uncaught (in promise) TypeError: An id must be specified error in the console:

es

2025-03-05_13-41-56.mp4

Logs: logs.zip

thesujai · 2025-03-05T13:43:08Z

@pcenov Thank you:)
@rtibbles tasks.job.Job is not inheriting JobProgressMixin like AsyncCommand is doing. Missed that for sure. So as a result methods like start_progress and is_cancelled are not accessible by the job object being created at the start of the new functions

rtibbles · 2025-03-05T15:09:37Z

Ah, yes, you would need to update the methods to those on the job object, not how they are proxied via the mixin. Sorry, I should probably have caught this sooner!

thesujai · 2025-03-05T16:08:04Z

kolibri/core/content/utils/channel_transfer.py

+logger = logging.getLogger(__name__)
+
+
+class JobProgress(JobProgressMixin):


Introducing a adapter class is fine? Or will there be problems now or in the future?

I would prefer to migrate to directly accessing the job object rather than continuing the indirection.

Most of the methods are just very thin wrappers around job object methods, so it would be better to remove the extra layer.

re-defining those methods inside this JobProgress would work so?

thesujai · 2025-03-06T20:12:34Z

Should work now!

pcenov · 2025-03-07T16:35:03Z

Hi @thesujai and @rtibbles - I confirm that now all of the import scenarios are functioning correctly.

@rtibbles while regression testing I noticed that for some reason only on my Windows 11 laptop the export to USB is not working (works fine on a Windows 10 laptop). Could you take a look at the logs to see whether the error is indicating whether this is something device specific or it's something else: win11-logs.zip

rtibbles · 2025-03-07T20:33:26Z

Hi @pcenov looking at the logs, I do see an error, definitely nothing to do with the changes here, but it looks like the command we are trying to run to get the disk data from Windows is failing for Windows 11!

rtibbles

Excellent work - code looks good, and QA passes!

QA now confirmed as working! Reported issue unrelated to this PR, filed separately.

rtibbles · 2025-03-07T20:40:26Z

Thank you @thesujai for your diligence, hard work, flexibility and good communication to get this wrapped up swiftly!

thesujai added 3 commits February 21, 2025 01:08

move import_channel_by_id to channel_import utils

61a19b5

extract channel_transfer logic from imporchannel command to a seperat…

3a1785c

…e module

use the transfer_channel util for importchannel task instead of invok…

3d8666b

…ing management commands

github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Feb 20, 2025

thesujai commented Feb 20, 2025

View reviewed changes

rtibbles reviewed Feb 20, 2025

View reviewed changes

thesujai added 2 commits February 21, 2025 10:50

dont pass the job to channel_transfer

4d4beea

update the importchannel tests with new mock paths

b8baf27

rtibbles self-assigned this Feb 25, 2025

rtibbles reviewed Feb 27, 2025

View reviewed changes

pcenov self-requested a review March 5, 2025 11:28

pcenov previously requested changes Mar 5, 2025

View reviewed changes

thesujai commented Mar 5, 2025

View reviewed changes

use update_progress without worrying about CLI progress bars

4e7ce07

thesujai force-pushed the importchannel-refactor branch from 9fde1c8 to 4e7ce07 Compare March 6, 2025 20:10

rtibbles approved these changes Mar 7, 2025

View reviewed changes

rtibbles mentioned this pull request Mar 7, 2025

Disk enumeration fails on Windows 11 #13175

Open

rtibbles merged commit 03ed729 into learningequality:develop Mar 7, 2025
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate core logic of importchannel to a utility function and update associated tasks #13099

Migrate core logic of importchannel to a utility function and update associated tasks #13099

thesujai commented Feb 20, 2025

thesujai Feb 20, 2025 •

edited

Loading

rtibbles Feb 20, 2025

rtibbles Feb 20, 2025 •

edited

Loading

rtibbles commented Feb 20, 2025

thesujai commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 •

edited

Loading

thesujai commented Feb 21, 2025

rtibbles left a comment

rtibbles commented Feb 27, 2025

pcenov left a comment

thesujai commented Mar 5, 2025 •

edited

Loading

rtibbles commented Mar 5, 2025

thesujai Mar 5, 2025

rtibbles Mar 5, 2025

rtibbles Mar 5, 2025

thesujai Mar 5, 2025

thesujai commented Mar 6, 2025

pcenov commented Mar 7, 2025

rtibbles commented Mar 7, 2025

rtibbles left a comment

rtibbles commented Mar 7, 2025

		@@ -587,15 +581,11 @@ def diskimport(
		drive = get_mounted_drive_by_id(drive_id)

		logger = logging.getLogger(__name__)


		def start_file_transfer(job, filetransfer, channel_id, dest, no_upgrade, contentfolder):

		logger = logging.getLogger(__name__)


		class JobProgress(JobProgressMixin):

Migrate core logic of importchannel to a utility function and update associated tasks #13099

Migrate core logic of importchannel to a utility function and update associated tasks #13099

Conversation

thesujai commented Feb 20, 2025

Summary

References

Reviewer guidance

thesujai Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

rtibbles Feb 20, 2025

Choose a reason for hiding this comment

rtibbles Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

rtibbles commented Feb 20, 2025

thesujai commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 • edited Loading

Build Artifacts

thesujai commented Feb 21, 2025

rtibbles left a comment

Choose a reason for hiding this comment

rtibbles commented Feb 27, 2025

pcenov left a comment

Choose a reason for hiding this comment

thesujai commented Mar 5, 2025 • edited Loading

rtibbles commented Mar 5, 2025

thesujai Mar 5, 2025

Choose a reason for hiding this comment

rtibbles Mar 5, 2025

Choose a reason for hiding this comment

rtibbles Mar 5, 2025

Choose a reason for hiding this comment

thesujai Mar 5, 2025

Choose a reason for hiding this comment

thesujai commented Mar 6, 2025

pcenov commented Mar 7, 2025

rtibbles commented Mar 7, 2025

rtibbles left a comment

Choose a reason for hiding this comment

rtibbles commented Mar 7, 2025

thesujai Feb 20, 2025 •

edited

Loading

rtibbles Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited

Loading

thesujai commented Mar 5, 2025 •

edited

Loading