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.

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

Merged
merged 15 commits into from
May 10, 2023
Merged
27 changes: 24 additions & 3 deletions kolibri/core/assets/src/views/CoreMenu/CoreMenuOption.vue
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@
required: false,
default: null,
},
disabled: {
type: Boolean,
required: false,
default: false,
},
},
data() {
return {
Expand All @@ -109,6 +114,12 @@
return window.location.pathname == this.link;
},
optionStyle() {
if (this.disabled) {
return {
color: this.$themeTokens.textDisabled,
margin: '8px',
};
}
if (this.isActive) {
return {
color: this.$themeTokens.primaryDark,
Expand All @@ -130,7 +141,13 @@
};
},
optionIconColor() {
return this.active ? this.$themeTokens.primary : null;
if (this.disabled) {
return this.$themeTokens.textDisabled;
} else if (this.active) {
return this.$themeTokens.primary;
} else {
return null;
}
},
iconAfter() {
return this.visibleSubMenu ? 'chevronUp' : 'chevronDown';
Expand Down Expand Up @@ -170,6 +187,9 @@
};
},
toggleAndroidMenu() {
if (this.disabled) {
return;
}
this.$emit('toggleAndroidMenu');
},
handleNav(route) {
Expand All @@ -180,9 +200,10 @@
return generateNavRoute(this.link, route, params);
},
conditionalEmit() {
if (!this.link) {
this.$emit('select');
if (this.disabled || this.link) {
return;
}
this.$emit('select');
},
},
};
Expand Down
2 changes: 1 addition & 1 deletion kolibri/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"js-cookie": "^3.0.5",
"knuth-shuffle-seeded": "^1.0.6",
"kolibri-constants": "0.1.42",
"kolibri-design-system": "https://github.com/learningequality/kolibri-design-system#a671f6f898eef07a3167dc74648ef329f8e3af44",
"kolibri-design-system": "https://github.com/learningequality/kolibri-design-system#2e5c1e8fc9a5239eaf3780639953619f34b6a2be",
"lockr": "0.8.5",
"lodash": "^4.17.21",
"loglevel": "^1.8.1",
Expand Down
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
Expand Up @@ -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',
Expand Down Expand Up @@ -119,11 +119,18 @@ export default function useDownloadRequests(store) {
status: 'QUEUED',
date_added: new Date(),
};
// TODO: Remove and replace by real progress implementation
Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up issue #10564

// 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,
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -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 });
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 fetchUserDownloadRequests is implemented so better to be ready for it. I will resolve it in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
});
Expand Down
Loading