-
Notifications
You must be signed in to change notification settings - Fork 20
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: remove duplicative course run card on allocation #1248
base: master
Are you sure you want to change the base?
Conversation
1b5bf88
to
49e8678
Compare
49e8678
to
001571c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1248 +/- ##
==========================================
+ Coverage 89.00% 89.02% +0.01%
==========================================
Files 403 404 +1
Lines 8733 8728 -5
Branches 2124 2116 -8
==========================================
- Hits 7773 7770 -3
+ Misses 918 916 -2
Partials 42 42 ☔ View full report in Codecov by Sentry. |
@@ -443,7 +443,7 @@ export function useContentAssignments() { | |||
|
|||
const upgradeableAuditEnrollmentCourseKeys = [...allEnrollmentsByStatus.inProgress] | |||
.filter(enrollment => isEnrollmentUpgradeable(enrollment)) | |||
.map((enrollment) => enrollment.courseKey); | |||
.map((enrollment) => enrollment.courseRunId); |
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.
Legacy course-based assignments will still have assignment.courseRunId
below representing a top-level course key (e.g., edX+DemoX
), not a course run, as described by the comment on lines 448-451 below. This would technically represent a change in existing behavior, as I believe legacy course-based assignments would be no longer be filtered if this list only has enrollment.courseRunId
🤔
Would we want to compare assignment.courseRunId
to either enrollment.courseKey
or enrollment.courseRunId
to handle both legacy assignments and run-based assignments?
Example:
const potentiallyUpgradeableAuditEnrollmentCourses = [...allEnrollmentsByStatus.inProgress]
.filter(enrollment => isEnrollmentUpgradeable(enrollment))
.map((enrollment) => ({
courseKey: enrollment.courseKey,
courseRunId: enrollment.courseRunId,
}));
// Filter out any assignments that have a corresponding potentially upgradeable
// audit enrollment. Note: all enrollment cards currently assume content key is
// a course run id despite the current assignment's content key representing either a
// course run key or, for legacy assignments, a course key.
const filteredAssignmentsForDisplay = assignmentsForDisplay.filter((assignment) => (
!potentiallyUpgradeableAuditEnrollmentCourses.some(({ courseKey, courseRunId }) => (
[courseKey, courseRunId].includes(assignment.courseRunId)
))
));
const mockRedeemableLearnerCreditPolicies = emptyRedeemableLearnerCreditPolicies; | ||
const mockSubsidyExpirationDateStr = dayjs().add(ENROLL_BY_DATE_WARNING_THRESHOLD_DAYS + 1, 'days').toISOString(); | ||
const mockAssignmentConfigurationId = 'test-assignment-configuration-id'; | ||
const mockAssignment = { | ||
contentKey: 'edX+DemoX', | ||
courseKey: 'edX+DemoX', | ||
courseRunId: 'course-v1:Best+course+2T2025', |
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.
Do assignments have a courseKey
or courseRunId
property? I believe useContentAssignments
is dealing with the raw assignments, not related to the transformed assignments to make them more like the enrollments data.
For instance, the below assignment within assignmentsForDisplay
only has a contentKey
, which can either be a course key for existing legacy assignments OR the course run key used today:
I believe this data structure is the one that mockAssignment
should be following, since it's the one expected by mockUseEnterpriseCourseEnrollments
, which subsequently calls transformLearnerContentAssignment
adding the courseRunId
attribute at that point. I don't believe an assignment returned by transformLearnerContentAssignment
would ever have the courseKey
attribute in practice, though.
tl;dr; I believe mockAssignment
and similar variables should define contentKey
and parentContentKey
, but not courseKey
and courseRunId
.
During the transition to run based assignments, a refactor was missed related to filtering out duplicative learner credit allocations on a potentially upgradable audit enrollment on the learner dashboard page. This did not affect the user's ability to upgrade their audit enrollment. It presented a confusing UI experience on which course card to proceed on (Upgrade or learner credit enrollment.)
This PR removes duplicative course cards by filtering on the course run vs the content key which may either be a top level course or a course run id.
Prior to fix:
After fix:
For all changes
Only if submitting a visual change