* fix: Use reselect’s defaultMemoize instead of lodash.memoize
Lodash memoize doesn’t examine all parameters when deciding to memoize, apparently, meaning it doesn’t re-call the function if any parameter except the first changes.
More here: https://dev.to/nioufe/you-should-not-use-lodash-for-memoization-3441
* Fixing test setup. Improper use of sequenceMetadata factory!
Two problems:
One, we weren’t properly passing the courseId into our sequenceMetadata factories, so it was differing than the one defined in courseMetadata. This didn’t manifest until now because we weren’t using the one from sequenceMetadata until this memoization fix!
Two, I’d updated the options for sequenceMetadata to have a “unitBlocks” option, but didn’t update all the usages of the old “unitBlock” option. This meant it was manually creating its own unit instead of inheriting the one from courseBlocks, resulting in a different ID.
I find it much more legible this way.
Some thoughts… as part of refactoring it, I made some of the redux selectors more formal, and made use of reselect more thoroughly. this resulted in a reduction in re-renders from 16 to 12 on your average page load. It’s also a bit more verbose, accounting for some of the increased line count.
I hadn’t tried it before, but found the memoize method of comparing previous props/state to current props/state to be very, very nice. Much easier than manually comparing props, and much clearer to me than using react hooks’ dependency arrays.
The lack of dependency arrays feels really freeing in general to me. They’ve been such a source of hard-to-track-down bugs, and the hooks linter does not always suggest the right solution for what belongs in and out of the array.
Function names are nice. We had a ton of custom hooks in there so that we could put names to otherwise anonymous bits of functionality.
Also note: this component has a test suite. It passed without any changes. 🥳
We were inconsistently using “position” - a 1-indexed value - in JS arrays which are 0-indexed. We had an existing, normalized property called “activeUnitIndex” which we now use everywhere instead. The value is modified back to 1-indexed before being returned to the server.
* Adding testing-library dependencies, and bumping frontend-build to be compatible with them.
* Adding a function to initialize the redux store
We need to use it in a few places. Seems worth not-repeating, since they can easily get out of sync. In general, tests should only test the parts of the store they care about, as well.
* Adding function to initialize a mock application.
Ultimately I’d like to move this to frontend-platform as an alternative to ‘initialize’ for tests. ‘initialize’ is an async function which complicates matters.
* Using more explicit assertions for courseware reducer fields.
This removes the need for the snapshot file, and ensures our test is more resilient to unrelated changes in the store.
Also added a few more stages of assertions to some of the tests, showing that they have the right values over time.
* Adding a helper to build a simple course blocks response.
We can use this in the courseware data tests, and shortly in the tests for CoursewareContainer.
* Modifying sequenceMetadata factory to allow multiple units.
This will help us test sequence navigation’s behavior more fully by having multiple units in a sequence.
* A little linting and cleanup.
* Adding first round of tests for CoursewareContainer.
Tests loading, sequence navigation/unit rendering, and ‘denied’ states.
Subsequent tests will add tests for handlers.
* Do not redirect when the sequenceId is not valid
That is, if firstSequenceId is null or undefined. This prevents the url
becoming bogus but does cause the course contents display to become stuck
with the loading message.
* Detect invalid sequence when loading
If the course has no sections or the first section has no sub-sections,
then sequence will be null. Before the redirection fix, this would cause an
error, but after, the sequenceStatus never leaves the loading state. Thus,
if still loading and the sequence is null, return the no.content message.
* Check sequenceId instead of sequence
From David Joy:
During initial page load, I expect there's a period of time before the
course blocks API and the sequence metadata API come back where the
sequenceStatus is loading but the sequence is still false, meaning that
we'll see a flash of this 'no content' messaging for a moment before the
data comes in.
If we instead check whether sequenceId is null here, that may give us a
more accurate condition. The sequenceId in redux is only populated when we
begin to request a sequence (fetchSequence thunk). If we have no sequence
ID in the URL route, then fetchSequence never happens and the sequenceId in
redux stays null.
* Fix up some additional errors Piotr found
This fixes errors caused by deleting units or subsections.
* Move test for unit validity to SequenceContent
After further conversation, we decide that
we shouldn't just treat HTML blocks as units.
We've been making the assumption that Verticals
occupy the Unit-level, and we shouldn't break that
assumption for this specific case.
Reverts part of e04f588d1f.
However, the error handling changes remain.
* Add test for fetchCourse
* Add tests for fetchDatesTab, fetchOutlineTab, fetchSequence and resetDeadlines
* Implement fetch tabs tests
* Add fail test case for fetchSequence
* Add success test for fetchSequence
* Add test for resetDeadlines
* Update test group name
* Add empty tests for courseware and bookmarks
* Fix wrong field in saveSequencePosition thunk
* Add tests for courseware data layer
* Temporary commit
* Split tests after rebase
* Revert "Fix wrong field in saveSequencePosition thunk"
This reverts commit 4394d363c58ad929f81e587ce4da2241528494b5.
* Fix test for position
* Move executeThunk into utils
* Add test for all reducers
* Add expect statements for logs
* Remove redundant snapshot tests and add some specific checks
* Polishing
* Remove redundant checks
* Fix bug in normalizer and update test
* Upgrade @edx/frontend-platform dependency
* Utilize MockAuthService instead of manual auth package mocking
* Update tests after breaking changes in master
* Remove redundant snapshot check
Fixes bug where courses with html-type XBlocks as children
of sequences would ignore those children instead of treating them
as units. This caused the app to later just give up and redirect
to the course home in the old experience.
Also, handle that scenario where we have sections/sequences
with children of unexpected block types more gracefully by
logging an error instead of crashing.
TNL-7305
* Moving model-store into “generic” sub-directory.
Also adding a README.md to explain what belongs in “generic”
* Moving user-messages into “generic” sub-directory.
* Moving PageLoading into “generic” sub-directory.
* Moving “tabs” module into “generic” sub-directory.
* Moving InstructorToolbar and MasqueradeWidget up to instructor-toolbar.
The masquerade widget is a sub-module of instructor-toolbar.
* Co-locating celebration APIs with celebration utils.
Also adding an ADR about thunk/API naming conventions and making some other areas of the code adhere to it.
* Moving courseware data (thunks, api) into the courseware module.
Note that cousre-home/data/api still uses normalizeBlocks - this should be fixed so it’s not reaching across. Maybe we pull that particular API up top.
This PR includes a few TODOs for things I saw, as well as a tiny bit of whitespace cleanup.
- Pulled Course Home specific components into `course-home`
- Created a courseHome reducer (and all necessary data files - api, thunks, slice)
- Removed Course Home logic from Courseware's data files (api, thunks, slice, etc.)
- Renamed Outline Tab URL to end in `/home` rather than `/outline` again (per Product)
Co-authored-by: Carla Duarte <cduarte@edx.org>
* BB-2569: Use faVideo instead of faFilm for video units; Set page title based on section, sequence, and course titles
* Add CourseLicense component with styling
* Reorder the pageTitleBreadCrumbs that are used for setting the page title
* Revert "Add CourseLicense component with styling"
This reverts commit 8d154998de.
* Fix package-lock.json so that only new changes for react-helmet are included
The sequence.lmsWebUrl variable is loaded as part of the course blocks API. The status of that API’s request is stored in courseStatus.
The useEffect hook in useExamRedirect didn’t ensure that courseStatus was equal to “loaded”. This meant that if the sequence loaded first, it might attempt to redirect to sequence.lmsWebUrl even though that variable is still undefined.
When global.location.assign() is given `undefined` as a value, it tacks it onto the end of the URL and calls it a day. After that, we’ve got a badly formed URL.
* Normalizing “courseInfo” back into “course”
Splitting it out denormalizes the data and introduces potential data inconsistencies.
* Name component JSX files with the name of the component.
* Normalizing some module exports/naming.
* Moving alerts into a sub-directory.
* DRYing up alert hook creation into reusable useAlert hook.
* Adding some comments about ‘courses’ hydration.
Specifically, make sure that the header, footer, and tabs are all
shared code so that they look the same and don't need to be
redefined as we add more tab pages.
TNL-7191 - We didn’t fully protect against sequences with no units. The next/previous buttons now check whether there is a unit ID and construct a URL without if one doesn’t exist. When we load a sequence without units, we now show a message to the user so the page doesn’t look broken.
This should fix intermittent bugs in checking block completions. Prior we were checking the completion only for the first unit loaded in a given sequence no matter the current unit.
Fixes TNL-7187 - Adds a no-op useLayoutEffect hook to Unit.jsx to prevent the unit iframe from pausing React’s rendering lifecycle. Very strange bug - see comments in that file for more detail.
* Adding an index.js file for user-messages.
Importing from the module, not its contents.
* Allowing customProps to be passed though AlertList to Alerts.
* UserMessagesProvider can create flash messages.
A flash message is one that will be displayed on the next reload of the page. UserMessagesProvider now provides a “addFlash” function. These messages are stored in localStorage and displayed the next time UserMessagesProvider is mounted, which is generally going to be on the next page refresh.
Once displayed, flash messages are cleared out of localStorage.
* Hooking up Enroll Now button and adding “success” alert.
Success alert is shown as a flash message on next page reload.
* Using ALERT_TYPES constants.
TNL-7171, TNL-7172, TNL-7173, TNL-7174: When a user is denied access to load courseware, redirect them to the appropriate location based upon the error code returned. If the error code is unknown they will be redirected to course home.
TNL-7170
lms_base_url becomes lmsBaseUrl in the app and is then used by the InstructorToolbar to link the user back to the LMS. If it isn’t present, the toolbar hides itself. This puts it back.
Fixes TNL-7175: Redirect to course home if a user is not unenrolled and the course is private.
- Require authentication to use the app while course blocks api requires it
- Gracefully handle course blocks api request failures allowing app to proceed to it redirection logic
Notable changes:
- selectors related to sequences are more resilient to missing models. In the case the course blocks api returns successfully but empty (in this case of enrolled but course not yet started).
- `fetchCourse` thunk handles failures for fetchCourseMeta and fetchCourseBlocks separately using `Promise.allSettled` instead of `Promise.all`
- `denied` is a new `courseStatus`
- Access denied redirect is done using a component at a new route `redirect/course-home/:courseId`
Now handles cases
- User is unauthenticated > redirect to login
- User is authenticated but not enrolled > redirects to lms course home
- When an enrolled user attempts to access courseware before the course start date they will load the sequence (but unable to load the vertical block). This behavior should be fixed in an update to edx-platform
This is a separate component because we have no mechanism for passing context/state into these alerts right now, and I’m not sure it’s worth building. Easier to just use different codes for different situations.
TNL-7129
This adds a third clause to our useAccessDeniedRedirect hook, which makes sure the user doesn’t have staff access (instead of normal, enrolled access) prior to redirecting.
As an aside, this redirection approach - irrespective of this PR - is not great. The UI mostly renders prior to this redirect happening. It would be better of this hook returned something that would help prevent the UI from rendering while the redirect is in progress. As it stands, a redirected user will see a flash of the page content prior to being booted. Not wonderful.
