Fixes saving unit position and unit redirection bugs - #128
Conversation
Thought there was a feature in 1.18.2 that I needed - turns out the feature hasn’t been released yet. Still fine to bump the dependency, though.
Fixes a few bugs in the courseware container: - Position was not being saved because we weren’t reading “saveUnitPosition” correctly. - We weren’t calling checkContentRedirect with the right arguments - it was using a non-existent unitId instead of the routeUnitId, meaning we would redirect to the active unit even if a unit was specified in the URL. Adds tests in CoursewareContainer for various URL and data states. Now explicitly tests: - Exam redirects - The resume block method when it has, and doesn’t have, a block to resume. - The content redirect when a unit isn’t present on the URL (uses sequence.position) - Loading a specific unit (not the first of a sequence!) by URL. Updated some of the factories to be more flexible/allow multiple units.
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
+ Coverage 51.51% 52.27% +0.76%
==========================================
Files 119 119
Lines 1355 1360 +5
Branches 279 279
==========================================
+ Hits 698 711 +13
+ Misses 635 627 -8
Partials 22 22
Continue to review full report at Codecov.
|
stvstnfrd
left a comment
stvstnfrd
left a comment
There was a problem hiding this comment.
Looks good and love the tests! Just a question or two, but tentative +1
| const forbiddenCourseBlocks = Factory.build('courseBlocks', { | ||
| courseId: forbiddenCourseMetadata.id, | ||
| }); | ||
| const { courseBlocks: forbiddenCourseBlocks } = buildSimpleCourseBlocks(forbiddenCourseMetadata.id, null); |
There was a problem hiding this comment.
Nit: Do we need to pass null? Or is that already the implied default behavior?
There was a problem hiding this comment.
Hm, I guess I could have just left the parameter out and it'd be undefined, but I figure it's better to obey the method signature. It just needs to be falsy so that the default behavior happens, in this case. ideally there'd be an options parameter here instead of this. Once #97 merges, I think we can use options for this stuff.
There was a problem hiding this comment.
I think the only reason it jumped out to me is that we're not passing in the title in all invocations ;)
|
|
||
| // This is an important line that ensures the spinner has been removed - and thus our main | ||
| // content has been loaded - prior to proceeding with our expectations. | ||
| await waitForElementToBeRemoved(screen.getByRole('status')); |
There was a problem hiding this comment.
Just a thought, is it worth extracting this into a helper?
I know it's just a single line of code, but the longer comment is useful and we seem to repeating this pattern a few times. I could imagine it being a bit easier to read if we were invoking, ensureMainContentLoaded(screen).
Not a big deal if it's a wonky refactor though.
There was a problem hiding this comment.
That's not a bad thought.
It's funny, I always find when I come into a test suite that has a lot of things extracted to helpers I find it very hard to find my way around, but when I'm writing a test suite I want to do it... hard to know when to bother. :)
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
…inal-fix EDUCATOR-4685 final fix for using label with course id
2 participants
Fixes a few bugs in the courseware container:
Adds tests in CoursewareContainer for various URL and data states.
Now explicitly tests:
Updated some of the factories to be more flexible/allow multiple units.