-
Notifications
You must be signed in to change notification settings - Fork 717
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.
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
New feature: Remote download on content pages #10558
Changes from all commits
26e584e
0c04877
e045a1d
28deada
ab30f1a
65a9faf
5fc9812
3bde935
6038527
5ae63b5
cf72ed7
5f041d3
af0e177
7f57bc1
d587c87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/** | ||
* `useDownloadRequests` composable function mock. | ||
* | ||
* If default values are sufficient for tests, | ||
* you only need call `jest.mock('<useDownloadRequests file path>')` | ||
* at the top of a test file. | ||
* | ||
* If you need to override some default values from some tests, | ||
* you can import a helper function `useDownloadRequestsMock` that accepts | ||
* an object with values to be overriden and use it together | ||
* with `mockImplementation` as follows: | ||
* | ||
* ``` | ||
* // eslint-disable-next-line import/named | ||
* import useDownloadRequests, { useDownloadRequestsMock } from '<useDownloadRequests file path>'; | ||
* | ||
* jest.mock('<useDownloadRequests file path>') | ||
* | ||
* it('test', () => { | ||
* useDownloadRequests.mockImplementation( | ||
* () => useDownloadRequestsMock({ downloadRequestMap: ... }) | ||
* ); | ||
* }) | ||
* ``` | ||
* | ||
* You can reset your mock implementation back to default values | ||
* for other tests by calling the following in `beforeEach`: | ||
* | ||
* ``` | ||
* useDownloadRequests.mockImplementation(() => useDownloadRequestsMock()) | ||
* ``` | ||
*/ | ||
|
||
const MOCK_DEFAULTS = { | ||
downloadRequestsTranslator: { | ||
$tr: jest.fn(), | ||
}, | ||
downloadRequestMap: { | ||
downloads: {}, | ||
}, | ||
isDownloadedByLearner: jest.fn(), | ||
isDownloadingByLearner: jest.fn(), | ||
addDownloadRequest: jest.fn(), | ||
}; | ||
|
||
export function useDownloadRequestsMock(overrides = {}) { | ||
return { | ||
...MOCK_DEFAULTS, | ||
...overrides, | ||
}; | ||
} | ||
|
||
export default jest.fn(() => useDownloadRequestsMock()); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import Vue from 'kolibri.lib.vue'; | |
import { createTranslator } from 'kolibri.utils.i18n'; | ||
import { set } from '@vueuse/core'; | ||
|
||
const translator = createTranslator('DownloadRequests', { | ||
const downloadRequestsTranslator = createTranslator('DownloadRequests', { | ||
downloadStartedLabel: { | ||
message: 'Download requested', | ||
context: 'A message shown to indicate that a download request has been created', | ||
|
@@ -119,11 +119,18 @@ export default function useDownloadRequests(store) { | |
status: 'QUEUED', | ||
date_added: new Date(), | ||
}; | ||
// TODO: Remove and replace by real progress implementation | ||
// as soon as backend can provide it. `complete` is just a mock field | ||
// that may not reflect precisely they way backend will inform fronted | ||
// about progress. Also see `isDownloadingByLearner` and `isDownloadedByLearner`. | ||
requestData.complete = false; | ||
setTimeout(() => (requestData.complete = true), 1000); | ||
|
||
console.log(requestData); | ||
set(downloadRequestMap, requestData.node_id, requestData); | ||
set(downloadRequestMap.downloads, requestData.node_id, requestData); | ||
store.commit('CORE_CREATE_SNACKBAR', { | ||
text: translator.$tr('downloadStartedLabel'), | ||
actionText: translator.$tr('goToDownloadsPage'), | ||
text: downloadRequestsTranslator.$tr('downloadStartedLabel'), | ||
actionText: downloadRequestsTranslator.$tr('goToDownloadsPage'), | ||
actionCallback: navigateToDownloads, | ||
backdrop: false, | ||
forceReuse: true, | ||
|
@@ -146,12 +153,48 @@ export default function useDownloadRequests(store) { | |
return Promise.resolve(); | ||
} | ||
|
||
function isDownloadingByLearner(content) { | ||
if (!content || !content.id) { | ||
return false; | ||
} | ||
const downloadRequest = downloadRequestMap.downloads[this.content.id]; | ||
// TODO: Get real progress from `downloadRequest` as soon as backend is ready | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up issue #10564 |
||
// and determine whether the content is downloading according to that instead | ||
// of using the `complete` mock. | ||
// `isDownloadingByLearner` is expected to be reactive by components that use it | ||
// and return accurate information as the progress changes. After updating to real | ||
// progress, check that related logic in components that use `isDownloadingByLearner` | ||
// still makes sense and all features are working as expected. | ||
return Boolean(downloadRequest && !downloadRequest.complete); | ||
} | ||
|
||
// Note that backend filters out download requests that correspond to content that was | ||
// removed after being downloaded, therefore the presence of a completed download request | ||
// means that the content is downloaded on the device. | ||
function isDownloadedByLearner(content) { | ||
if (!content || !content.id) { | ||
return false; | ||
} | ||
const downloadRequest = downloadRequestMap.downloads[this.content.id]; | ||
// TODO: Get real progress from `downloadRequest` as soon as backend is ready | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up issue #10564 |
||
// and determine whether the content finished downloading according to that instead | ||
// of using the `complete` mock. | ||
// `isDownloadedByLearner` is expected to be reactive by components that use it | ||
// and return accurate information as the progress changes. After updating to real | ||
// progress, check that related logic in components that use `isDownloadedByLearner` | ||
// still makes sense and all features are working as expected. | ||
return Boolean(downloadRequest && downloadRequest.complete); | ||
} | ||
|
||
return { | ||
fetchUserDownloadRequests, | ||
fetchDownloadsStorageInfo, | ||
downloadRequestMap, | ||
addDownloadRequest, | ||
removeDownloadRequest, | ||
removeDownloadsRequest, | ||
downloadRequestsTranslator, | ||
isDownloadingByLearner, | ||
isDownloadedByLearner, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. I don't know if we'll use pagination parameters here on this page, but it's currently how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up PR #10683 |
||
return fetchChannels({ baseurl }).then(() => { | ||
return _loadTopicsContent(store, id, baseurl); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up issue #10564