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.

Sign up for GitHub

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

Jump to bottom

track progress for content download request #10830

Merged
merged 3 commits into from
Jun 14, 2023
Merged

track progress for content download request #10830

merged 3 commits into from
Jun 14, 2023

Conversation

Jaspreet-singh-1032
Copy link
Contributor

References

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Jun 11, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 11, 2023 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.16.0a15.dev0_git.30.gecf17504.pex
Windows Installer (EXE) kolibri-0.16.0a15.dev0+git.30.gecf17504-unsigned.exe
Debian Package kolibri_0.16.0a15.dev0+git.30.gecf17504-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.0a15.dev0+git.30.gecf17504-0.3.0.dmg
Source Tarball kolibri-0.16.0a15.dev0+git.30.gecf17504.tar.gz
WHL file kolibri-0.16.0a15.dev0+git.30.gecf17504-py2.py3-none-any.whl

rtibbles
rtibbles requested changes Jun 12, 2023
View reviewed changes
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Should subclass the import manager to add the download request progress updating, rather than modifying the global JobProgressMixin.

kolibri/core/tasks/utils.py Outdated
@@ -276,6 +277,11 @@ def update_progress(self, increment=1, message="", extra_data=None):
)
if extra_data and isinstance(extra_data, dict):
self.job.update_metadata(**extra_data)
if self.download_request:
self.download_request.update_progress(
Copy link
Member

Choose a reason for hiding this comment

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

This is a global mixin, not just used for downloading individual pieces of metadata - this seems like the wrong place to make this change.

It would be better instead to subclass the RemoteChannelResourceImportManager class specifically for a content request import, and add this logic there, invoking the super method for update_progress and then also applying this change.

kolibri/core/tasks/utils.py Outdated
@@ -289,6 +295,8 @@ def start_progress(self, total=100):
self.progresstracker = ProgressTracker(total=total)
if self.job:
self.job.update_progress(0, total)
if self.download_request:
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here.

kolibri/core/content/utils/content_request.py Outdated
@@ -480,6 +480,7 @@ def process_download_request(download_request):
baseurl=peer.base_url,
node_ids=[download_request.contentnode_id],
)
import_manager.download_request = download_request
Copy link
Member

Choose a reason for hiding this comment

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

In the subclassed import manager, we should pass in the download request in the __init__ function, rather than setting it externally.

kolibri/core/content/models.py
@@ -466,6 +466,20 @@ def build_for_user(cls, user):
status=ContentRequestStatus.Pending,
)

def update_progress(self, progress, total_progress):
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the correct place for this logic to live.

@rtibbles
Copy link
Member

Hi @Jaspreet-singh-1032 - thanks for the hard work here, I think this is mostly done, just need to shuffle around where this logic is being added.

@Jaspreet-singh-1032
Copy link
Contributor Author

Sure @rtibbles, I will make these changes.

@Jaspreet-singh-1032
Copy link
Contributor Author

Jaspreet-singh-1032 commented Jun 14, 2023 •
edited
Loading

Hello @rtibbles, can we re-run the workflows? I think some of the checks canceled because of taking too long time.

rtibbles
rtibbles approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Beautiful - so clean!

@rtibbles rtibbles merged commit ea1e075 into learningequality:develop Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtibbles rtibbles rtibbles approved these changes

Assignees
No one assigned
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Content download request progress tracking
2 participants
@Jaspreet-singh-1032 @rtibbles