-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: dismiss alert modal and cancelled assignments #900
Conversation
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/tests/CourseEnrollments.test.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/tests/CourseEnrollments.test.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/CourseSection.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/tests/CourseEnrollments.test.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/data/constants.js
Outdated
Show resolved
Hide resolved
export default function getActiveAssignments(assignments = []) { | ||
const activeAssignments = assignments.filter((assignment) => [ | ||
ASSIGNMENT_TYPES.CANCELLED, ASSIGNMENT_TYPES.ALLOCATED, | ||
ASSIGNMENT_TYPES.ALLOCATED, | ||
].includes(assignment.state)); |
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.
@adamstankiewicz any concerns with changing this definition of getActiveAssignments()
?
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.
@iloveagent57 @katrinan029 Yes, by removing ASSIGNMENT_TYPES.CANCELLED
here, the CourseEnrollments
components that check against "cancelled" assignment states would no longer be provided any canceled assignments to process (i.e., I don't believe the util functions (e.g., getIsActiveCancelledAssignment
) would be executed? 🤔 For example, trying to run this locally, I couldn't get the canceled alert to appear unless canceled assignments were considered here.
I might recommend modifying getActiveAssignments
to split out arrays for each assignment lifecycle state that's relevant, which would enable more granular control over which types of assignments you're working with in CourseEnrollments
, i.e.:
// Renamed `getActiveAssignments` to `getAssignmentsByState`
export function getAssignmentsByState(assignments = []) {
const allAssignments = [];
const allocatedAssignments = [];
const canceledAssignments = [];
const acceptedAssignments = [];
assignments.forEach((assignment) => {
allAssignments.push(assignment);
if (assignment.state === ASSIGNMENT_TYPES.ALLOCATED) {
allocatedAssignments.push(assignment);
}
if (assignment.state === ASSIGNMENT_TYPES.CANCELLED) {
canceledAssignments.push(assignment);
}
if (assignment.state === ASSIGNMENT_TYPES.ACCEPTED) {
acceptedAssignments.push(assignment);
}
});
const hasAssignments = allAssignments.length > 0;
const hasAllocatedAssignments = allocatedAssignments.length > 0;
const hasCanceledAssignments = canceledAssignments.length > 0;
const hasAcceptedAssignments = acceptedAssignments.length > 0;
return {
assignments,
hasAssignments,
allocatedAssignments,
hasAllocatedAssignments,
canceledAssignments,
hasCanceledAssignments,
acceptedAssignments,
hasAcceptedAssignments,
};
Note: This function should really only be called within getRedeemablePoliciesData
at this point (as it is on master
) as that logic has already been executed further up in the component tree, without needing to run again to get similar results.
src/components/dashboard/main-content/course-enrollments/CourseEnrollments.jsx
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #900 +/- ##
==========================================
+ Coverage 84.87% 85.00% +0.12%
==========================================
Files 320 337 +17
Lines 6399 7120 +721
Branches 1552 1743 +191
==========================================
+ Hits 5431 6052 +621
- Misses 941 1035 +94
- Partials 27 33 +6 ☔ View full report in Codecov by Sentry. |
|
||
const activeExpiredAssignments = assignments.filter((assignment) => ( | ||
assignment?.actions.some((action) => ( | ||
action.actionType === ASSIGNMENT_ACTION_TYPES.AUTOMATIC_CANCELLATION_NOTIFICATION |
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.
Note: the cron job that runs to automatically cancel assignments will result in the assignment(s) being marked as state=cancelled
, which means those assignment cards would actually render as the canceled assignment card variant instead of the expired variant for assignments where the cron job already ran.
I believe at this point, the handing of expired assignments is for the edge case where a learner is viewing an assignment in the UI that hasn't yet been canceled by the cron job. That said, I think the logic here may need to be based on the determined enrollment deadline as depicted by isAssignmentExpired
, i.e. are there expired assignments that haven't been dismissed with an assignment enrollment deadline that is more recent than the last dismissed expired assignments?
[idea] Refactor isAssignmentExpired
slightly to return both an isExpired
boolean but also the enrollment deadline for the assignment and then check the localStorage date against this returned enrollment deadline.
export const isAssignmentExpired = (assignment) => {
// Assignment is not in an allocated state, so it cannot be expired.
if (!assignment) {
return false;
}
const currentDate = dayjs();
const allocationDate = dayjs(assignment.created);
const enrollmentEndDate = dayjs(assignment.contentMetadata.enrollByDate);
const subsidyExpirationDate = dayjs(assignment.subsidyExpirationDate);
const isExpired = (
currentDate.diff(allocationDate, 'day') > 90
|| currentDate.isAfter(enrollmentEndDate)
|| currentDate.isAfter(subsidyExpirationDate)
);
const earliestAssignmentExpiryDate = [
dayjs(allocationDate).add(90, 'day'),
enrollmentEndDate,
subsidyExpirationDate,
].sort()[0]?.toDate();
return {
isExpired,
enrollByDeadline: earliestAssignmentExpiryDate,
};
};
export function getHasActiveExpiredAssignment(assignments) {
const lastExpiredAlertDismissedTime = global.localStorage.getItem(LEARNER_ACKNOWLEDGED_ASSIGNMENT_EXPIRATION_ALERT);
const activeExpiredAssignments = assignments.filter((assignment) => {
const {
isExpired,
enrollByDeadline,
} = isAssignmentExpired(assignment);
if (!isExpired) {
return false;
}
return dayjs(enrollByDeadline).isAfter(new Date(lastExpiredAlertDismissedTime));
});
return activeExpiredAssignments.length > 0;
}
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.
This is a good idea for now, but really we should introduce an expired
state on the backend - and then the frontend could determine if an assignment is expired by checking assignment.state === 'expired'
.
return activeExpiredAssignments.length > 0; | ||
} | ||
|
||
export function getIsActiveCancelledAssignment(assignments) { |
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: getHasActiveCancelledAssignments
as we're working with plural assignments here?
LEARNER_ACKNOWLEDGED_ASSIGNMENT_EXPIRATION_ALERT, | ||
} from '../main-content/course-enrollments/data/constants'; | ||
|
||
export function getIsActiveExpiredAssignment(assignments) { |
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: getHasActiveExpiredAssignments
as we're working with plural assignments here?
export default function getActiveAssignments(assignments = []) { | ||
const activeAssignments = assignments.filter((assignment) => [ | ||
ASSIGNMENT_TYPES.CANCELLED, ASSIGNMENT_TYPES.ALLOCATED, | ||
ASSIGNMENT_TYPES.ALLOCATED, | ||
].includes(assignment.state)); |
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.
@iloveagent57 @katrinan029 Yes, by removing ASSIGNMENT_TYPES.CANCELLED
here, the CourseEnrollments
components that check against "cancelled" assignment states would no longer be provided any canceled assignments to process (i.e., I don't believe the util functions (e.g., getIsActiveCancelledAssignment
) would be executed? 🤔 For example, trying to run this locally, I couldn't get the canceled alert to appear unless canceled assignments were considered here.
I might recommend modifying getActiveAssignments
to split out arrays for each assignment lifecycle state that's relevant, which would enable more granular control over which types of assignments you're working with in CourseEnrollments
, i.e.:
// Renamed `getActiveAssignments` to `getAssignmentsByState`
export function getAssignmentsByState(assignments = []) {
const allAssignments = [];
const allocatedAssignments = [];
const canceledAssignments = [];
const acceptedAssignments = [];
assignments.forEach((assignment) => {
allAssignments.push(assignment);
if (assignment.state === ASSIGNMENT_TYPES.ALLOCATED) {
allocatedAssignments.push(assignment);
}
if (assignment.state === ASSIGNMENT_TYPES.CANCELLED) {
canceledAssignments.push(assignment);
}
if (assignment.state === ASSIGNMENT_TYPES.ACCEPTED) {
acceptedAssignments.push(assignment);
}
});
const hasAssignments = allAssignments.length > 0;
const hasAllocatedAssignments = allocatedAssignments.length > 0;
const hasCanceledAssignments = canceledAssignments.length > 0;
const hasAcceptedAssignments = acceptedAssignments.length > 0;
return {
assignments,
hasAssignments,
allocatedAssignments,
hasAllocatedAssignments,
canceledAssignments,
hasCanceledAssignments,
acceptedAssignments,
hasAcceptedAssignments,
};
Note: This function should really only be called within getRedeemablePoliciesData
at this point (as it is on master
) as that logic has already been executed further up in the component tree, without needing to run again to get similar results.
@@ -6,6 +6,7 @@ import { Info } from '@edx/paragon/icons'; | |||
import { getContactEmail } from '../../../../utils/common'; | |||
|
|||
const CourseAssignmentAlert = ({ | |||
showAlert, |
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.
Good call to make this alert component act as a controlled component instead of uncontrolled.
const hasExpiredAssignments = assignmentsData?.some(assignment => isAssignmentExpired(assignment)) | ||
&& getIsActiveExpiredAssignment(assignmentsData); |
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.
Related to another comment, I believe you could move the call to the (refactored) isAssignmentExpired
into getIsActiveExpiredAssignment
itself such that it has access to the proposed enrollByDeadline
returned by isAssignmentExpired
to compare against the localStorage timestamp value.
@@ -53,12 +57,12 @@ const CourseEnrollments = ({ children }) => { | |||
); | |||
setAssignments(assignmentsData); |
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.
[suggestion] I might propose a bit of a further refactor to this useEffect
to clean it up a bit:
useEffect(() => {
if (!redeemableLearnerCreditPolicies) {
return;
}
const {
allocatedAssignments,
canceledAssignments,
hasCanceledAssignments,
} = redeemableLearnerCreditPolicies.learnerContentAssignments;
const sortedAllocatedAssignments = sortAssignmentsByAssignmentStatus(allocatedAssignments);
const transformedAllocatedAssignments = getTransformedAllocatedAssignments(
sortedAllocatedAssignments,
slug,
);
setAssignments(transformedAllocatedAssignments);
const hasActiveCancelledAssignments = (
hasCanceledAssignments && getHasActiveCancelledAssignments(canceledAssignments)
);
setShowCancelledAssignmentsAlert(hasActiveCancelledAssignments);
const hasActiveExpiredAssignments = getHasActiveExpiredAssignment(allocatedAssignments);
setShowExpiredAssignmentsAlert(hasActiveExpiredAssignments);
}, [redeemableLearnerCreditPolicies, slug]);
By doing so, we could rely on the state variable assignments
instead of assignedCourses
when passing them as the course runs to the assigned CourseSection
component (i.e., the call to getTransformedAllocatedAssignments
was moved within the useEffect
).
Closing this PR in favor of #910 (same changes, just extended / finished up on my own branch to not mess with this one). |
Description:
We want to hide cancelled assignment course cards and when a user dismisses an cancellation alert, they should not see the alert again. This is a temporary fix to hide the alert based on local storage.
Screen.Recording.2023-12-14.at.10.11.22.AM.mov
JIRA Ticket
For all changes
Only if submitting a visual change