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

feat: Make courseware URLs more human-readable #540

Open
wants to merge 65 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
8970352
Update path
Jul 13, 2021
b23a633
Add ADR
Jul 13, 2021
832107f
Add reference to ADR 009
Jul 13, 2021
3b2bbbd
Merge branch 'master' into KristinAoki/TNL-8511
KristinAoki Jul 13, 2021
d38c07a
Update 0009-courseware-url-shortening.md
KristinAoki Jul 13, 2021
19f3186
Add example url
KristinAoki Jul 13, 2021
3b2f91c
Update 0009-courseware-url-shortening.md
KristinAoki Jul 15, 2021
457dc4b
Update 0009-courseware-url-shortening.md
KristinAoki Jul 15, 2021
abac174
Update model to base storage off hash_key for sequence and unit
Jul 21, 2021
fcda485
Remove commented out code
Jul 21, 2021
eb70d37
Merge branch 'KristinAoki/TNL-8511' of github.com:edx/frontend-app-le…
Jul 21, 2021
e96d885
Update model to store sequence based on hash_key
Jul 26, 2021
e4ec845
Update link path to match new format
Jul 26, 2021
388b9df
Fix undefined error in sequence metadata
Jul 26, 2021
174be4a
Update iframeUrl to use decoded_id instead of id
Jul 26, 2021
c592753
Merge branch 'master' into KristinAoki/TNL-8511
Jul 28, 2021
4be725b
Update iframe url parameter variables
Jul 29, 2021
29a24aa
Fix broken api calls
Jul 29, 2021
aeca68f
Update block id variables
Jul 29, 2021
43ff07a
Update course exit url
Jul 29, 2021
d517f94
Add more information about the url changes
Jul 30, 2021
4baf78c
Update links to match the new pattern
Jul 30, 2021
f2d7e11
Update API call urls
Jul 30, 2021
7b45c8b
Fix broken redirect
Jul 30, 2021
96a5753
Merge branch 'master' into KristinAoki/TNL-8511
Aug 2, 2021
2296922
Fix broken sequence metadata URL
Aug 2, 2021
7242583
Fix variable in API call
Aug 2, 2021
9b33f20
Fix route path bug
Aug 2, 2021
7f2df8b
Update snapshot to reflect new storage of blocks by hash key
Aug 2, 2021
f0fab48
Fix broken urls
Aug 2, 2021
40ea419
Fix broken sequence URL variable
Aug 2, 2021
20935e7
Update snapshot to reflect new block hash keys
Aug 2, 2021
5f0968e
Update hash key to be consistently generated for snapshots
Aug 2, 2021
7fde146
Remove unused parameters
Aug 3, 2021
2d1a13a
Remove unused definitions
Aug 3, 2021
07042d9
Update object key for unit and sequence to store by id if no hash_key
Aug 4, 2021
75b195b
Fix typos
Aug 5, 2021
544e11b
Add new flag to courseMetadata
Aug 5, 2021
296607f
Add new flag declarations
Aug 5, 2021
4e136d9
Add dispatch for new feature flag
Aug 5, 2021
24de9d7
Add new feature flag
Aug 5, 2021
e05428e
Add new flag and function for old urls
Aug 5, 2021
ea93aea
Add check for routeUnitId
Aug 6, 2021
367c8ad
Add new feature flag
Aug 11, 2021
a32a580
Add id to hash key mapping for sequences and units
Aug 11, 2021
0ba9ed7
Add hash key and mapping model
Aug 11, 2021
5efc222
Update model to store by id instead of hash key
Aug 11, 2021
b4c83a3
Update conditional checks to be based on hash_key instead of id
Aug 11, 2021
9c21909
Add condition to know how to render sequence links
Aug 11, 2021
7b57b06
Update id generation
Aug 11, 2021
38db0eb
Add page route and redirect to handle old urls
Aug 12, 2021
847cdfa
Fix misplace semicolon
Aug 12, 2021
5538b48
Update prereq_id
Aug 12, 2021
6f2281c
Add hash_key to id translations
Aug 12, 2021
43eb589
Update url length function
Aug 12, 2021
569b628
Update sequenceMetadata response
Aug 12, 2021
5fa33e4
Update checkBlockCompletion function unitID
Aug 12, 2021
ea02b2f
Update links
Aug 12, 2021
a17e2a1
Update unitViaSequenceId to check if id is not block id
Aug 12, 2021
9ca5c61
Fix id references
Aug 12, 2021
c46da1d
Merge branch 'master' into KristinAoki/TNL-8511
Aug 12, 2021
c09ba48
Update sequence id for goto links
Aug 12, 2021
fe46806
Update modelsSequenceId and modelsUnitId definitions
Aug 12, 2021
c6578d4
Update url to take direct sequence id
Aug 12, 2021
8882026
Fix misspelled variable names
Aug 12, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/decisions/0008-liberal-courseware-path-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,6 @@ And more like:
```
https://learning.edx.org/course/course-v1:edX+DemoX.1+2T2019/Being_Social/Teams
```

_This further work has been expanded upon in
[ADR #9: Courseware URL shortening](./0009-courseware-url-shortening.md)._
58 changes: 58 additions & 0 deletions docs/decisions/0009-courseware-url-shortening.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Courseware URL shortening
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved

## Status

Accepted

_This updates some of the content in [ADR #8: Liberal courseware path handling](./0008-liberal-courseware-path-handling.md)._

## Context

The current URL is not human-readable. The URL is composed of the UsageKeys for the current sequence and unit. We can't make UsageKeys themselves more readable because they're tied to student state.

This is what the URLs currently look like:

```

https://learning.edx.org/course/course-v1:edX+DemoX.1+2T2019/block-v1:edX+DemoX.1+2T2019+type@sequential+block@e0a61b3d5a2046949e76d12cac5df493/block-v1:edX+DemoX.1+2T2019+type@vertical+block@52dbad5a28e140f291a476f92ce11996

```

After exploring different URL patterns and possible redundancies in the current URL format. The course, run, and organization is stated in every portion of the URL. We also do not need the URL to tell us the type of block since it has been determined that all URLs will follow the path` /course/:courseId/:sequenceId/:unitId`.
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved

## Decision

The courseware URL will format to the following structure:

```

https://learning.edx.org/c/:courseId/:sequenceHash/:unitHash/:sectionSlug/:sequenceSlug/:unitSlug/
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved

```

Example URL:

```

https://learning.edx.org/c/course-v1:edX+DemoX.1+2T2019/YmxvY2/njuRCq/optional-example-problem-types/stem-problems/code-grader

```

The fields definition and requirements ar as follows:

* :courseId (required) - same as the previous `courseId`.
* :sequenceHash (required) - a `urlsafe_b64encode` version of the `sequenceId`.
* :unitHash (required) - a `urlsafe_b64encode` version of the `unitId`.
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved
* :sectionSlug (optional) - `display_name` of the current sequence's parent section.
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved
* :sequenceSlug (optional) - `display_name` of the current sequence.
* :unitSlug (optional) - `display_name` of the current unit
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved

Partial paths will update with the required parameters as dicussed in [ADR #8: Liberal courseware path handling](./0008-liberal-courseware-path-handling.md). The `sequenceHash` and `unitHash` will shorten their respective ids using `hashlib.blake2b` with `digest_size` of 6 bytes. `Blake2b` will reduce the length of the id so the encoded version can also be short. Hashing will be handled by `blake2b` because it is the fastest hashing function in the `hashlib` library. The hash will be generated and mapped in LMS. The slugs based on `display_name` are optional because not all blocks have an associated `display_name` attributes, most likely to occur in OLX imports. The `display_name` will be pulled from the current section, sequence, and unit attribute, and if there is not an attribute `display`, the url will use the attribute `display_name_with_default`. The `display_name` will be formatted safely for a url using Django's [slugify](https://docs.djangoproject.com/en/3.2/ref/utils/#django.utils.text.slugify). Slugify allows unicode identifiers in the slug. If the slugs are omitted, it will redirect to the canonical version without the slugs.

## Consequences

If old URLs are not properly routed then the content and those links will no longer be accessible to the user. The old URLs could include, but not limited to, bookmarks and exams.
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved

KristinAoki marked this conversation as resolved.
Show resolved Hide resolved
## Further work

At some point, we may decide to further extend the URL shortening to the entire platform. At the moment, the hashes for the sequences and units are generated when the sequences and units are being called. In the future, it would be better if the hashes would be generated and stored when the sequences and units are originally created. This would require `learning_sequences` to include a class for unit storage, which is not being stored at the moment.
17 changes: 9 additions & 8 deletions src/course-home/data/__snapshots__/redux.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -395,38 +395,39 @@ Object {
},
"courseBlocks": Object {
"courses": Object {
"block-v1:edX+DemoX+Demo_Course+type@course+block@bcdabcdabcdabcdabcdabcdabcdabcd3": Object {
"abcdabcd3": Object {
"hasScheduledContent": false,
"id": "course-v1:edX+DemoX+Demo_Course_1",
"sectionIds": Array [
"block-v1:edX+DemoX+Demo_Course+type@chapter+block@bcdabcdabcdabcdabcdabcdabcdabcd2",
"abcdabcd2",
],
"title": "bcdabcdabcdabcdabcdabcdabcdabcd3",
},
},
"sections": Object {
"block-v1:edX+DemoX+Demo_Course+type@chapter+block@bcdabcdabcdabcdabcdabcdabcdabcd2": Object {
"abcdabcd2": Object {
"complete": false,
"courseId": "course-v1:edX+DemoX+Demo_Course_1",
"id": "block-v1:edX+DemoX+Demo_Course+type@chapter+block@bcdabcdabcdabcdabcdabcdabcdabcd2",
"id": "abcdabcd2",
"resumeBlock": false,
"sequenceIds": Array [
"block-v1:edX+DemoX+Demo_Course+type@sequential+block@bcdabcdabcdabcdabcdabcdabcdabcd1",
"abcdabcd1",
],
"title": "Title of Section",
},
},
"sequences": Object {
"block-v1:edX+DemoX+Demo_Course+type@sequential+block@bcdabcdabcdabcdabcdabcdabcdabcd1": Object {
"abcdabcd1": Object {
"complete": false,
"description": null,
"due": null,
"effortActivities": 2,
"effortTime": 15,
"hash_key": "abcdabcd1",
"icon": null,
"id": "block-v1:edX+DemoX+Demo_Course+type@sequential+block@bcdabcdabcdabcdabcdabcdabcdabcd1",
"id": "abcdabcd1",
"legacyWebUrl": "http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/jump_to/block-v1:edX+DemoX+Demo_Course+type@sequential+block@bcdabcdabcdabcdabcdabcdabcdabcd1?experience=legacy",
"sectionId": "block-v1:edX+DemoX+Demo_Course+type@chapter+block@bcdabcdabcdabcdabcdabcdabcdabcd2",
"sectionId": "abcdabcd2",
"showLink": true,
"title": "Title of Sequence",
},
Expand Down
5 changes: 3 additions & 2 deletions src/course-home/data/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export function normalizeOutlineBlocks(courseId, blocks) {
break;

case 'sequential':
models.sequences[block.id] = {
models.sequences[(block.hash_key || block.id)] = {
complete: block.complete,
description: block.description,
due: block.due,
Expand All @@ -144,6 +144,7 @@ export function normalizeOutlineBlocks(courseId, blocks) {
// link to the MFE ourselves).
showLink: !!block.legacy_web_url,
title: block.display_name,
hash_key: block.hash_key,
};
break;

Expand All @@ -167,7 +168,7 @@ export function normalizeOutlineBlocks(courseId, blocks) {
if (Array.isArray(section.sequenceIds)) {
section.sequenceIds.forEach(sequenceId => {
if (sequenceId in models.sequences) {
models.sequences[sequenceId].sectionId = section.id;
models.sequences.[sequenceId].sectionId = section.id;
KristinAoki marked this conversation as resolved.
Show resolved Hide resolved
} else {
logInfo(`Section ${section.id} has child block ${sequenceId}, but that block is not in the list of sequences.`);
}
Expand Down
8 changes: 4 additions & 4 deletions src/course-home/dates-tab/DatesTab.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('DatesTab', () => {
component = (
<AppProvider store={store}>
<UserMessagesProvider>
<Route path="/course/:courseId/dates">
<Route path="/c/:courseId/dates">
<TabContainer tab="dates" fetch={fetchDatesTab} slice="courseHome">
<DatesTab />
</TabContainer>
Expand Down Expand Up @@ -81,7 +81,7 @@ describe('DatesTab', () => {
beforeEach(() => {
axiosMock.onGet(courseMetadataUrl).reply(200, courseMetadata);
axiosMock.onGet(datesUrl).reply(200, datesTabData);
history.push(`/course/${courseId}/dates`); // so tab can pull course id from url
history.push(`/c/${courseId}/dates`); // so tab can pull course id from url

render(component);
});
Expand Down Expand Up @@ -147,7 +147,7 @@ describe('DatesTab', () => {
describe('Suggested schedule messaging', () => {
beforeEach(() => {
setMetadata({ is_self_paced: true, is_enrolled: true });
history.push(`/course/${courseId}/dates`);
history.push(`/c/${courseId}/dates`);
});

it('renders SuggestedScheduleHeader', async () => {
Expand Down Expand Up @@ -316,7 +316,7 @@ describe('DatesTab', () => {

beforeEach(() => {
axiosMock.onGet(datesUrl).reply(200, datesTabData);
history.push(`/course/${courseId}/dates`); // so tab can pull course id from url
history.push(`/c/${courseId}/dates`); // so tab can pull course id from url
});

it('redirects to course survey for a survey_required error code', async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/course-home/outline-tab/OutlineTab.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe('Outline Tab', () => {
await fetchAndRender();

const sequenceLink = screen.getByText('Title of Sequence');
expect(sequenceLink.getAttribute('href')).toContain(`/course/${courseId}`);
expect(sequenceLink.getAttribute('href')).toContain(`/c/${courseId}`);
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/course-home/outline-tab/SequenceLink.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function SequenceLink({
// canLoadCourseware is true if the Courseware MFE is enabled, false otherwise
const coursewareUrl = (
canLoadCourseware
? <Link to={`/course/${courseId}/${id}`}>{title}</Link>
? <Link to={`/c/${courseId}/${id}`}>{title}</Link>
: <Hyperlink destination={legacyWebUrl}>{title}</Hyperlink>
);
const displayTitle = showLink ? coursewareUrl : title;
Expand Down
33 changes: 16 additions & 17 deletions src/courseware/CoursewareContainer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,28 @@ const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSe
getResumeBlock(courseId).then((data) => {
// This is a replace because we don't want this change saved in the browser's history.
if (data.sectionId && data.unitId) {
history.replace(`/course/${courseId}/${data.sectionId}/${data.unitId}`);
history.replace(`/c/${courseId}/${data.sectionId}/${data.unitId}`);
} else if (firstSequenceId) {
history.replace(`/course/${courseId}/${firstSequenceId}`);
history.replace(`/c/${courseId}/${firstSequenceId}`);
}
});
}
});

const checkSectionUnitToUnitRedirect = memoize((courseStatus, courseId, sequenceStatus, section, unitId) => {
if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && unitId) {
history.replace(`/course/${courseId}/${unitId}`);
history.replace(`/c/${courseId}/${unitId}`);
}
});

const checkSectionToSequenceRedirect = memoize((courseStatus, courseId, sequenceStatus, section, unitId) => {
if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && !unitId) {
// If the section is non-empty, redirect to its first sequence.
if (section.sequenceIds && section.sequenceIds[0]) {
history.replace(`/course/${courseId}/${section.sequenceIds[0]}`);
history.replace(`/c/${courseId}/${section.sequenceIds[0]}`);
// Otherwise, just go to the course root, letting the resume redirect take care of things.
} else {
history.replace(`/course/${courseId}`);
history.replace(`/c/${courseId}`);
}
}
});
Expand All @@ -53,7 +53,7 @@ const checkUnitToSequenceUnitRedirect = memoize((courseStatus, courseId, sequenc
if (courseStatus === 'loaded' && sequenceStatus === 'failed' && unit) {
// If the sequence failed to load as a sequence, but it *did* load as a unit, then
// insert the unit's parent sequenceId into the URL.
history.replace(`/course/${courseId}/${unit.sequenceId}/${unit.id}`);
history.replace(`/c/${courseId}/${unit.sequenceId}/${unit.id}`);
}
});

Expand All @@ -72,7 +72,7 @@ const checkSequenceToSequenceUnitRedirect = memoize((courseId, sequenceStatus, s
if (sequence.unitIds !== undefined && sequence.unitIds.length > 0) {
const nextUnitId = sequence.unitIds[sequence.activeUnitIndex];
// This is a replace because we don't want this change saved in the browser's history.
history.replace(`/course/${courseId}/${sequence.id}/${nextUnitId}`);
history.replace(`/c/${courseId}/${sequence.id}/${nextUnitId}`);
}
}
});
Expand Down Expand Up @@ -106,13 +106,13 @@ class CoursewareContainer extends Component {
match: {
params: {
courseId: routeCourseId,
sequenceId: routeSequenceId,
sequenceId: routeSequenceHash,
},
},
} = this.props;
// Load data whenever the course or sequence ID changes.
this.checkFetchCourse(routeCourseId);
this.checkFetchSequence(routeSequenceId);
this.checkFetchSequence(routeSequenceHash);
}

componentDidUpdate() {
Expand All @@ -130,15 +130,14 @@ class CoursewareContainer extends Component {
match: {
params: {
courseId: routeCourseId,
sequenceId: routeSequenceId,
sequenceId: routeSequenceHash,
unitId: routeUnitId,
},
},
} = this.props;

// Load data whenever the course or sequence ID changes.
this.checkFetchCourse(routeCourseId);
this.checkFetchSequence(routeSequenceId);
this.checkFetchSequence(routeSequenceHash);

// All courseware URLs should normalize to the format /course/:courseId/:sequenceId/:unitId
// via the series of redirection rules below.
Expand Down Expand Up @@ -201,7 +200,7 @@ class CoursewareContainer extends Component {
} = this.props;

this.props.checkBlockCompletion(courseId, sequenceId, routeUnitId);
history.push(`/course/${courseId}/${sequenceId}/${nextUnitId}`);
history.push(`/c/${courseId}/${sequenceId}/${nextUnitId}`);
}

handleNextSequenceClick = () => {
Expand All @@ -217,10 +216,10 @@ class CoursewareContainer extends Component {
let nextUnitId = null;
if (nextSequence.unitIds.length > 0) {
[nextUnitId] = nextSequence.unitIds;
history.push(`/course/${courseId}/${nextSequence.id}/${nextUnitId}`);
history.push(`/c/${courseId}/${nextSequence.id}/${nextUnitId}`);
} else {
// Some sequences have no units. This will show a blank page with prev/next buttons.
history.push(`/course/${courseId}/${nextSequence.id}`);
history.push(`/c/${courseId}/${nextSequence.id}`);
}

const celebrateFirstSection = course && course.celebrations && course.celebrations.firstSection;
Expand All @@ -235,10 +234,10 @@ class CoursewareContainer extends Component {
if (previousSequence !== null) {
if (previousSequence.unitIds.length > 0) {
const previousUnitId = previousSequence.unitIds[previousSequence.unitIds.length - 1];
history.push(`/course/${courseId}/${previousSequence.id}/${previousUnitId}`);
history.push(`/c/${courseId}/${previousSequence.id}/${previousUnitId}`);
} else {
// Some sequences have no units. This will show a blank page with prev/next buttons.
history.push(`/course/${courseId}/${previousSequence.id}`);
history.push(`/c/${courseId}/${previousSequence.id}`);
}
}
}
Expand Down
Loading