Skip to content

Commit

Permalink
feat: handle courseware paths more liberally (openedx#395)
Browse files Browse the repository at this point in the history
Valid courseware URLs currently include:
* /course/:courseId
* /course/:courseId/:sequenceId
* /course/:courseId/:sequenceId/:unitId

In this commit we add support for:
* /course/:courseId/:sectionId
* /course/:courseId/:sectionId/:unitId
* /course/:courseId/:unitId

All URL forms still redirect to:
  /course/:courseId/:sequenceId/:unitId

See ADR openedx#8 for more context.

All changes:
* refactor: allow courseBlocks factory to build multiple sections
* refactor: make CoursewareContainer tests less brittle & stateful
* feat: handle courseware paths more liberally
* refactor: reorder, rename, & comment redirection functions

TNL-7796
  • Loading branch information
kdmccormick authored Apr 1, 2021
1 parent 6a376b2 commit 353964e
Show file tree
Hide file tree
Showing 12 changed files with 621 additions and 237 deletions.
3 changes: 3 additions & 0 deletions docs/decisions/0002-courseware-page-decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ Today, if the URL only specifies the course ID, we need to pick a sequence to sh

Similarly, if the URL doesn't contain a unit ID, we use the `position` field of the sequence to determine which unit we want to display from that sequence. If the position isn't specified in the sequence, we choose the first unit of the sequence. After determining which unit to display, we update the URL to match. After the URL is updated, the application will attempt to load that unit via an iFrame.

_This URL scheme has been expanded upon in
[ADR #8: Liberal courseware path handling](./0008-liberal-courseware-path-handling.md)._

## "Container" components vs. display components

This application makes use of a few "container" components at the top level - CoursewareContainer and CourseHomeContainer.
Expand Down
90 changes: 90 additions & 0 deletions docs/decisions/0008-liberal-courseware-path-handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Liberal courseware path handling

## Status

Accepted

_This updates some of the content in [ADR #2: Courseware page decisions](./0002-courseware-page-decisions.md)._

## Context

The courseware container currently accepts three path forms:

1. `/course/:courseId`
2. `/course/:courseId/:sequenceId`
3. `/course/:courseId/:sequenceId/:unitId`

Forms #1 and #2 are always redirected to Form #3 via simple set of rules:

* If the sequenceId is not specified, choose the first sequence in the course.
* If the unitId is not specified, choose the active unit in the sequence,
or the first unit if none are active.

Thus, Form #3 is effectively the canonoical path;
all Learning MFE units should be served from it.
We acknowledge that the best user experience is to link directly to the canonoical
path when possible, since it skips the redirection steps.
Still, there are times when it is necessary or prudent to link just to a course or
a sequence.

Through recent work in the LMS, we are realizing that there are _also_ times where it
would be simpler or more performant to link a user to an
_entire section without specifying a squence_ or to a
_unit without including the sequence_.
Specifically, this capability would let as avoid further modulestore or
block transformer queries in order to discern the course structure when trying to
direct a learner to a section or unit.
Futhermore, we hypothesize that being able to build a Learning MFE courseware link
with just a unit ID or a section ID will be a nice simplifying quality for future
development or debugging.

## Decision

The courseware container will accept five total path forms:

1. `/course/:courseId`
2. `/course/:courseId/:sectionId`
3. `/course/:courseId/:sectionId/:unitId`
4. `/course/:courseId/:sequenceId`
5. `/course/:courseId/:unitId`
6. `/course/:courseId/:sequenceId/:unitId`

The redirection rules are as follows:

* Forms #1 redirects to Form #4 by selecting the first sequence in the course.
* Form #2 redirects to Form #4 by selecting to the first sequence in the section.
* Form #3 redirects to Form #5 by dropping the section ID.
* Form #4 redirects to Form #6 by choosing the active unit in the sequence
(or the first unit, if none are active).
* Form #5 redirects to Form #6 by filling in the ID of the sequence that the
specified unit belongs to (in the edge case where the unit belongs to multiple
sequences, the first sequence is selected).

As before, Form #5 is the canonocial courseware path, which is always redirected to
by any of the other courseware path forms.

## Consequences

The above decision is implemented.

## Further work

At some point, we may decide to further extend the URL scheme to be
more human-readable.

We can't make UsageKeys themselves more readable because they're tied to student state,
but we could introduce a new optional `slug` field on Sequences,
which would be captured and propagated to the learning_sequences API.
We could eventually do something similar to Units, since those slugs only have to be sequence-local.

So eventually, URLs could look less 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
```

And more like:
```
https://learning.edx.org/course/course-v1:edX+DemoX.1+2T2019/Being_Social/Teams
```
126 changes: 110 additions & 16 deletions src/courseware/CoursewareContainer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,6 @@ import { TabPage } from '../tab-page';
import Course from './course';
import { handleNextSectionCelebration } from './course/celebration';

const checkExamRedirect = memoize((sequenceStatus, sequence) => {
if (sequenceStatus === 'loaded') {
if (sequence.isTimeLimited && sequence.lmsWebUrl !== undefined) {
global.location.assign(sequence.lmsWebUrl);
}
}
});

const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSequenceId) => {
if (courseStatus === 'loaded' && !sequenceId) {
// Note that getResumeBlock is just an API call, not a redux thunk.
Expand All @@ -41,8 +33,42 @@ const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSe
}
});

const checkContentRedirect = memoize((courseId, sequenceStatus, sequenceId, sequence, unitId) => {
if (sequenceStatus === 'loaded' && sequenceId && !unitId) {
const checkSectionUnitToUnitRedirect = memoize((courseStatus, courseId, sequenceStatus, section, unitId) => {
if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && unitId) {
history.replace(`/course/${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]}`);
// Otherwise, just go to the course root, letting the resume redirect take care of things.
} else {
history.replace(`/course/${courseId}`);
}
}
});

const checkUnitToSequenceUnitRedirect = memoize((courseStatus, courseId, sequenceStatus, unit) => {
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}`);
}
});

const checkSpecialExamRedirect = memoize((sequenceStatus, sequence) => {
if (sequenceStatus === 'loaded') {
if (sequence.isTimeLimited && sequence.lmsWebUrl !== undefined) {
global.location.assign(sequence.lmsWebUrl);
}
}
});

const checkSequenceToSequenceUnitRedirect = memoize((courseId, sequenceStatus, sequence, unitId) => {
if (sequenceStatus === 'loaded' && sequence.id && !unitId) {
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.
Expand Down Expand Up @@ -97,6 +123,8 @@ class CoursewareContainer extends Component {
sequenceStatus,
sequence,
firstSequenceId,
unitViaSequenceId,
sectionViaSequenceId,
match: {
params: {
courseId: routeCourseId,
Expand All @@ -110,15 +138,52 @@ class CoursewareContainer extends Component {
this.checkFetchCourse(routeCourseId);
this.checkFetchSequence(routeSequenceId);

// Redirect to the legacy experience for exams.
checkExamRedirect(sequenceStatus, sequence);

// Determine if we need to redirect because our URL is incomplete.
checkContentRedirect(courseId, sequenceStatus, sequenceId, sequence, routeUnitId);
// All courseware URLs should normalize to the format /course/:courseId/:sequenceId/:unitId
// via the series of redirection rules below.
// See docs/decisions/0008-liberal-courseware-path-handling.md for more context.
// (It would be ideal to move this logic into the thunks layer and perform
// all URL-changing checks at once. This should be done once the MFE is moved
// to the new Outlines API. See TNL-8182.)

// Determine if we can resume where we left off.
// Check resume redirect:
// /course/:courseId -> /course/:courseId/:sequenceId/:unitId
// based on sequence/unit where user was last active.
checkResumeRedirect(courseStatus, courseId, sequenceId, firstSequenceId);

// Check section-unit to unit redirect:
// /course/:courseId/:sectionId/:unitId -> /course/:courseId/:unitId
// by simply ignoring the :sectionId.
// (It may be desirable at some point to be smarter here; for example, we could replace
// :sectionId with the parent sequence of :unitId and/or check whether the :unitId
// is actually within :sectionId. However, the way our Redux store is currently factored,
// the unit's metadata is not available to us if the section isn't loadable.)
// Before performing this redirect, we *do* still check that a section is loadable;
// otherwise, we could get stuck in a redirect loop, since a sequence that failed to load
// would endlessly redirect to itself through `checkSectionUnitToUnitRedirect`
// and `checkUnitToSequenceUnitRedirect`.
checkSectionUnitToUnitRedirect(courseStatus, courseId, sequenceStatus, sectionViaSequenceId, routeUnitId);

// Check section to sequence redirect:
// /course/:courseId/:sectionId -> /course/:courseId/:sequenceId
// by redirecting to the first sequence within the section.
checkSectionToSequenceRedirect(courseStatus, courseId, sequenceStatus, sectionViaSequenceId, routeUnitId);

// Check unit to sequence-unit redirect:
// /course/:courseId/:unitId -> /course/:courseId/:sequenceId/:unitId
// by filling in the ID of the parent sequence of :unitId.
checkUnitToSequenceUnitRedirect(courseStatus, courseId, sequenceStatus, unitViaSequenceId);

// Check special exam redirect:
// /course/:courseId/:sequenceId(/:unitId) -> :legacyWebUrl
// because special exams are currently still served in the legacy LMS frontend.
checkSpecialExamRedirect(sequenceStatus, sequence);

// Check to sequence to sequence-unit redirect:
// /course/:courseId/:sequenceId -> /course/:courseId/:sequenceId/:unitId
// by filling in the ID the most-recently-active unit in the sequence, OR
// the ID of the first unit the sequence if none is active.
checkSequenceToSequenceUnitRedirect(courseId, sequenceStatus, sequence, routeUnitId);

// Check if we should save our sequence position. Only do this when the route unit ID changes.
this.checkSaveSequencePosition(routeUnitId);
}
Expand Down Expand Up @@ -249,13 +314,24 @@ class CoursewareContainer extends Component {
}
}

const unitShape = PropTypes.shape({
id: PropTypes.string.isRequired,
sequenceId: PropTypes.string.isRequired,
});

const sequenceShape = PropTypes.shape({
id: PropTypes.string.isRequired,
unitIds: PropTypes.arrayOf(PropTypes.string).isRequired,
sectionId: PropTypes.string.isRequired,
isTimeLimited: PropTypes.bool,
lmsWebUrl: PropTypes.string,
});

const sectionShape = PropTypes.shape({
id: PropTypes.string.isRequired,
sequenceIds: PropTypes.arrayOf(PropTypes.string).isRequired,
});

const courseShape = PropTypes.shape({
canLoadCourseware: PropTypes.shape({
errorCode: PropTypes.string,
Expand All @@ -278,6 +354,8 @@ CoursewareContainer.propTypes = {
sequenceStatus: PropTypes.oneOf(['loaded', 'loading', 'failed']).isRequired,
nextSequence: sequenceShape,
previousSequence: sequenceShape,
unitViaSequenceId: unitShape,
sectionViaSequenceId: sectionShape,
course: courseShape,
sequence: sequenceShape,
saveSequencePosition: PropTypes.func.isRequired,
Expand All @@ -292,6 +370,8 @@ CoursewareContainer.defaultProps = {
firstSequenceId: null,
nextSequence: null,
previousSequence: null,
unitViaSequenceId: null,
sectionViaSequenceId: null,
course: null,
sequence: null,
};
Expand Down Expand Up @@ -367,6 +447,18 @@ const firstSequenceIdSelector = createSelector(
},
);

const sectionViaSequenceIdSelector = createSelector(
(state) => state.models.sections || {},
(state) => state.courseware.sequenceId,
(sectionsById, sequenceId) => (sectionsById[sequenceId] ? sectionsById[sequenceId] : null),
);

const unitViaSequenceIdSelector = createSelector(
(state) => state.models.units || {},
(state) => state.courseware.sequenceId,
(unitsById, sequenceId) => (unitsById[sequenceId] ? unitsById[sequenceId] : null),
);

const mapStateToProps = (state) => {
const {
courseId, sequenceId, courseStatus, sequenceStatus,
Expand All @@ -382,6 +474,8 @@ const mapStateToProps = (state) => {
previousSequence: previousSequenceSelector(state),
nextSequence: nextSequenceSelector(state),
firstSequenceId: firstSequenceIdSelector(state),
sectionViaSequenceId: sectionViaSequenceIdSelector(state),
unitViaSequenceId: unitViaSequenceIdSelector(state),
};
};

Expand Down
Loading

0 comments on commit 353964e

Please sign in to comment.