New feature: Remote download on content pages #10558

MisRob · 2023-04-25T14:40:27Z

Summary

Fix: a remote content page now loads
Fix: a new download request is now saved where it was meant to in useDownloadRequests
New feature: Adds frontend part of download logic to content pages when browsing remote content. Note that the download action is mocked (see follow-ups and code comments)

(1) All actions as icons	(2) Actions hidden in the menu	(3) Actions partially hidden in the menu

Notice somewhat strange UX in (3) as the download option disappears and later the bookmark option appears. We may need to re-think the logic around displaying icons/menu items in the LearningActivityBar to be able to improve this. Unless someone has a better idea, I can open an issue for that.

References

Resolves a large portion of Remote Content Browsing: Update Content Renderer for downloading and streaming #9856 but contains some mocks. Remote Content Browsing: Update useDownloadRequests to provide remote content download progress (complete/incomplete) #10564 needs to be resolved for it to be complete.
Related KDS PR: Add an option to freeze loader kolibri-design-system#433

Follow-ups and related issues

Reviewer guidance

Taking into account the number of related issues, some of them in need of implementation before this feature is complete, I think it'd make more sense to do a more thorough QA of download as a whole as soon we have everything in place. In this PR, we can focus on:

To test remote content, you can navigate to Library -> Mr Tibbles' Classroom Server -> Kolibri QA Channel as a learner

Comments

After the referenced KDS PR is merged and before this PR is merged, I will replace the package.json reference
The original draft PR contains a comment with the summary of some decisions that I implemented here

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

To fix the topic content page not being properly loaded. This is most likely a temporary fix since the composable is work in progress. In the long term, it seems that pagination parameters will be optional.

for smoother user experience when showing/hiding download/bookmark button and the download loader

github-actions · 2023-04-25T15:03:09Z

Build Artifacts

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

- remove obsolete code - improve test cases independence - refactor redundant code - re-organize the test suite to be better readable

MisRob · 2023-04-26T06:15:55Z

kolibri/plugins/learn/assets/src/composables/useDownloadRequests.js

@@ -119,11 +119,18 @@ export default function useDownloadRequests(store) {
      status: 'QUEUED',
      date_added: new Date(),
    };
+    // TODO: Remove and replace by real progress implementation


Follow-up issue #10564

MisRob · 2023-04-26T06:16:08Z

kolibri/plugins/learn/assets/src/composables/useDownloadRequests.js

+      return false;
+    }
+    const downloadRequest = downloadRequestMap.downloads[this.content.id];
+    // TODO: Get real progress from `downloadRequest` as soon as backend is ready


Follow-up issue #10564

MisRob · 2023-04-26T06:16:26Z

kolibri/plugins/learn/assets/src/composables/useDownloadRequests.js

+      return false;
+    }
+    const downloadRequest = downloadRequestMap.downloads[this.content.id];
+    // TODO: Get real progress from `downloadRequest` as soon as backend is ready


Follow-up issue #10564

to prevent from 'Cannot read properties of undefined' occuring in some scenarios

MisRob · 2023-04-28T04:54:02Z

@jtamiace If you could look at the summary with the three recordings in the PR description, that'd be appreciated. Also note three related issues that involve UX

Remote Content Browsing: Improve the downloading string on a content page #10575
Remote Content Browsing: Show download completed snackbar notification #10576
Remote Content Browsing: Replace indeterminate downloading loader by determinate on a content page #10565

jtamiace · 2023-05-02T22:37:32Z

@MisRob for (1) and (3) the icons shifting around feels a bit unexpected and distracting. For (1), is it possible to make the spinner replace the location of the download icon so that either the loading spinner or the bookmark icon is showing but not both? So something like:

For how this adapts on mobile displays (2) and (3), I am not sure what technical solution makes sense, but here is an option that would require some different logic than full width displays but could eliminate the icon shifts. Keeping the download status outside of the menu seems like a nice way to communicate the system status:

^Although I guess disabling could also be used for the desktop view?

rather then disappearing it.

MisRob · 2023-05-08T12:20:25Z

Thank you, @jtamiace, that's right. I used the "disabled" state for both mobile and desktop as it was a bit more straightforward to implement. Looking much better now:

MisRob · 2023-05-08T13:37:59Z

Related learningequality/kolibri-design-system#433 has been merged and KDS reference in this PR updated to point to the latest KDS develop

LianaHarris360 · 2023-05-08T21:43:37Z

@MisRob For some resources the KCircularLoader seems to be continuous and is displayed before I have hit download. On other resources, it functions as expected.

InfiniteLoading.mov

MisRob · 2023-05-09T10:35:38Z

@LianaHarris360 Ah, I should have mentioned this - resources with ids 2ea9bda8703241be89b5b9fd87f88815 and 9e53d545aaf44c3787a29a34b189c56a are currently hardcoded in useDownloadRequests so if you're seeing this behavior only for these two, that should be fine for now, because they're indefinitely being downloaded due to that mock. Did you observe it for any other resources (you could have a look in URL where a resource ID can be found and compare to those I mentioned)?

LianaHarris360 · 2023-05-09T13:58:47Z

Ok, this behavior does only happen for those two hardcoded resources, which makes complete sense. Thank you!

marcellamaki

Thanks @MisRob - I've added just a couple of non-blocking comments from the code read through, but all can be discussed/addressed later. I also think it makes sense to have more robust manual QA happen here after I finish the non-mocked version of the content download request viewset work, so I think we're good to go with @LianaHarris360's manual QA and the QA team can pick up the testing once more of the backend is connected up (cc @radinamatic)

marcellamaki · 2023-05-09T16:55:33Z

kolibri/plugins/learn/assets/src/modules/topicsTree/handlers.js

@@ -41,6 +41,8 @@ export function showTopicsContent(store, id, deviceId = null) {
  if (deviceId) {
    return setCurrentDevice(deviceId).then(device => {
      const baseurl = device.base_url;
+      const { fetchUserDownloadRequests } = useDownloadRequests(store);
+      fetchUserDownloadRequests({ page: 1, pageSize: 100 });


Non-blocking, and can definitely be adjusted in follow up as needed, but do we want the default page size to be 100 here? That seems like a lot to me and like we want smaller pagination segments.

That's a good point. I don't know if we'll use pagination parameters here on this page, but it's currently how fetchUserDownloadRequests is implemented so better to be ready for it. I will resolve it in a follow-up PR.

Follow-up PR #10683

marcellamaki · 2023-05-09T16:56:25Z

kolibri/plugins/learn/assets/src/views/LearningActivityBar.vue

+        <Transition name="downloading-loader">
+          <!--
+            wrapping span needed here because
+            (1) tooltip doesn't display when `ref` is on `KCircularLoader`


tooltip doesn't display when ref is on `KCircularLoader

Today I learned!

I don't know if it's the case generally, it's possible that the new show prop I added to KCircularLoader plays a role in this but I didn't find time to investigate more closely. It's near the end of my TODOs list though :)

marcellamaki · 2023-05-09T16:58:27Z

kolibri/plugins/learn/assets/src/views/LearningActivityBar.vue

-            !this.$refs.timerButton.$el.contains(event.target) &&
-            !this.$refs.timer.$el.contains(event.target)
+            !this.$refs.timerButton?.$el.contains(event.target) &&
+            !this.$refs.timer?.$el.contains(event.target)


I am not clear on what this change, and the one above on line 500 and 501 is doing

That comes from this commit 5ae63b5. At some point, I noticed "Cannot read properties of undefined" console errors and found out it was caused by references not being present at the time of attempting to access them here in some situations. Most likely it was a little bug not directly related to the new code. I added safeguards to resolve that. If you were asking about the syntax, that's Optional chaining.

MisRob added 4 commits April 19, 2023 13:21

Add download to content pages

26e584e

Add pagination parameters

0c04877

To fix the topic content page not being properly loaded. This is most likely a temporary fix since the composable is work in progress. In the long term, it seems that pagination parameters will be optional.

Save the new download request to where it belongs

e045a1d

Add transitions

28deada

for smoother user experience when showing/hiding download/bookmark button and the download loader

MisRob mentioned this pull request Apr 25, 2023

[DO NOT MERGE] Remote download on a content page #10285

Closed

1 task

github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: large labels Apr 25, 2023

[tmp] Point to fork KDS and freeze the loader

ab30f1a

MisRob force-pushed the content-renderer-download-2 branch from b42c4c9 to f7a394d Compare April 26, 2023 04:28

Cleanup spec file

65a9faf

- remove obsolete code - improve test cases independence - refactor redundant code - re-organize the test suite to be better readable

MisRob force-pushed the content-renderer-download-2 branch from f7a394d to 65a9faf Compare April 26, 2023 04:40

MisRob mentioned this pull request Apr 26, 2023

Remote Content Browsing: Update useDownloadRequests to provide remote content download progress (complete/incomplete) #10564

Closed

3 tasks

MisRob commented Apr 26, 2023

View reviewed changes

MisRob mentioned this pull request Apr 26, 2023

Remote Content Browsing: Update Content Renderer for downloading and streaming #9856

Closed

2 tasks

MisRob added this to the Kolibri 0.16 Release: Remote Content Browsing milestone Apr 26, 2023

MisRob mentioned this pull request Apr 26, 2023

Remote Content Browsing: Replace indeterminate downloading loader by determinate on a content page #10565

Open

MisRob added 4 commits April 26, 2023 11:27

Fix tooltip not being displayed

5fc9812

Merge branch 'develop' into content-renderer-download-2

3bde935

Make sure boolean value is returned

6038527

Add safeguards

5ae63b5

to prevent from 'Cannot read properties of undefined' occuring in some scenarios

MisRob added the TODO: needs review Waiting for review label Apr 27, 2023

MisRob requested review from rtibbles, AlexVelezLl, akolson and LianaHarris360 April 27, 2023 11:58

MisRob mentioned this pull request Apr 27, 2023

Add an option to freeze loader learningequality/kolibri-design-system#433

Merged

8 tasks

MisRob marked this pull request as ready for review April 27, 2023 12:15

MisRob requested a review from marcellamaki April 27, 2023 12:15

MisRob mentioned this pull request Apr 28, 2023

Bookmark option for remote resources should be disabled #10444

Closed

MisRob requested a review from jtamiace April 28, 2023 04:53

MisRob added 2 commits May 8, 2023 14:11

Add option to disable menu item

cf72ed7

Disable the download button when downloading

5f041d3

rather then disappearing it.

MisRob added 3 commits May 8, 2023 14:27

Update tests to the latest changes

af0e177

Update KDS fork commit to the latest one

7f57bc1

Point to KDS develop isntead of fork

d587c87

marcellamaki approved these changes May 9, 2023

View reviewed changes

rtibbles merged commit 3e9c959 into learningequality:develop May 10, 2023

MisRob mentioned this pull request May 11, 2023

Decrease page size when fetching user download requests for topics content pages #10683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: Remote download on content pages #10558

New feature: Remote download on content pages #10558

MisRob commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

MisRob Apr 26, 2023

MisRob Apr 26, 2023

MisRob Apr 26, 2023

MisRob commented Apr 28, 2023 •

edited

Loading

jtamiace commented May 2, 2023 •

edited

Loading

MisRob commented May 8, 2023

MisRob commented May 8, 2023

LianaHarris360 commented May 8, 2023

MisRob commented May 9, 2023

LianaHarris360 commented May 9, 2023

marcellamaki left a comment

marcellamaki May 9, 2023

MisRob May 11, 2023

MisRob May 11, 2023

marcellamaki May 9, 2023

MisRob May 11, 2023

marcellamaki May 9, 2023

MisRob May 11, 2023

New feature: Remote download on content pages #10558

New feature: Remote download on content pages #10558

Conversation

MisRob commented Apr 25, 2023 • edited Loading

Summary

References

Reviewer guidance

Comments

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Apr 28, 2023 • edited Loading

jtamiace commented May 2, 2023 • edited Loading

MisRob commented May 8, 2023

MisRob commented May 8, 2023

LianaHarris360 commented May 8, 2023

MisRob commented May 9, 2023

LianaHarris360 commented May 9, 2023

marcellamaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

MisRob commented Apr 28, 2023 •

edited

Loading

jtamiace commented May 2, 2023 •

edited

Loading