-
Notifications
You must be signed in to change notification settings - Fork 221
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
fix: ignore failed outline API call when learner has no access #1259
Conversation
8dd60f1
to
ccdd37d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 88.29% 88.33% +0.03%
==========================================
Files 291 291
Lines 4923 4930 +7
Branches 1239 1242 +3
==========================================
+ Hits 4347 4355 +8
+ Misses 562 561 -1
Partials 14 14 ☔ View full report in Codecov by Sentry. |
src/course-home/data/thunks.js
Outdated
if (courseHomeCourseMetadataResult.status === 'rejected') { | ||
throw courseHomeCourseMetadataResult.reason; | ||
} else if (!courseHomeCourseMetadataResult.value.courseAccess.hasAccess) { | ||
dispatch(fetchTabDenied({ courseId })); | ||
} else if (tabDataResult || !getTabData) { | ||
} else if (tabDataResult && tabDataResult.status === 'rejected') { | ||
throw tabDataResult.reason; | ||
} else { | ||
dispatch(fetchTabSuccess({ | ||
courseId, | ||
targetUserId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notable changes:
- There's now a default case; one of [fetchTabDenied, fetchTabSuccess, fetchTabFailure] is always dispatched.
- A non-accessible course is now handled using fetchTabDenied regardless of the tab data query results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with a couple nits), deferring to any owning squad's review feedback.
src/course-home/data/thunks.js
Outdated
} else if (tabDataResult && tabDataResult.status === 'rejected') { | ||
throw tabDataResult.reason; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if it'd make sense to co-locate the conditions that result in throwing an exception?
if (courseHomeCourseMetadataResult.status === 'rejected') {
throw courseHomeCourseMetadataResult.reason;
} else if (tabDataResult?.status === 'rejected') {
throw tabDataResult.reason;
} else if (!courseHomeCourseMetadataResult.value.courseAccess.hasAccess) {
dispatch(fetchTabDenied({ courseId }));
} else {
dispatch(fetchTabSuccess({
courseId,
targetUserId,
}));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm pretty sure I want to dispatch fetchTabDenied as soon as possible, which is why the conditional is ordered like that. I added a comment to clarify this.
ccdd37d
to
997a524
Compare
If the learner doesn't even have access to the course (e.g. because the course starts in the future), don't worry about a 404 fetching the course outline since we're not planning to use it anyway. ENT-8078
997a524
to
1cea8cd
Compare
If the learner doesn't even have access to the course (e.g. because the course starts in the future), don't worry about a 404 fetching the course outline since we're not planning to use it anyway.
ENT-8078
matrix
A combination of factors contribute to this specific scenario with new behavior, but the vast majority of hits on this page won't trigger this scenario. It just so happens that it's likely to happen while testing on stage, which gets in the way of testing other features which rely on dispatching to fetchTabDenied.