Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: dismiss alert modal and cancelled assignments #900
Changes from 4 commits
f8b5af5
7883bfd
0cb6750
4a95b36
fa3eff5
ab42490
53ba280
64c9e72
c8e10a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?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, theCourseEnrollments
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 inCourseEnrollments
, i.e.:Note: This function should really only be called within
getRedeemablePoliciesData
at this point (as it is onmaster
) as that logic has already been executed further up in the component tree, without needing to run again to get similar 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.
Good call to make this alert component act as a controlled component instead of uncontrolled.
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:By doing so, we could rely on the state variable
assignments
instead ofassignedCourses
when passing them as the course runs to the assignedCourseSection
component (i.e., the call togetTransformedAllocatedAssignments
was moved within theuseEffect
).