-
Notifications
You must be signed in to change notification settings - Fork 33
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: enable allocation of restricted runs #1359
base: master
Are you sure you want to change the base?
Conversation
ab2a50c
to
23415e4
Compare
...onents/learner-credit-management/data/hooks/useCatalogContainsContentItemsMultipleQueries.js
Outdated
Show resolved
Hide resolved
}) => { | ||
// First, whittle down the runs to just the ones that are either completely | ||
// unrestricted, or restricted but allowed for the current subsidy. | ||
const unrestrictedCourseRuns = courseRuns.filter(courseRun => { |
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: just a naming consideration; given this list technically contains restricted runs (conditionally included by catalog inclusion), I wonder if a name like allowedCourseRuns
, unrestrictedCourseRunsForEnterpriseCustomer
, or similar might help mitigate any confusion around whether this list only contains truly unrestricted runs.
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.
Yea, I constantly face this naming issue. I hesitated to use "allowed" in this case because the purpose of the surrounding function is to determine which runs are "assignable" which could be mistaken for "allowed". Open to suggestions, or if you think "allowed" works with sufficient code comments.
src/components/learner-credit-management/cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
...onents/learner-credit-management/data/hooks/useCatalogContainsContentItemsMultipleQueries.js
Outdated
Show resolved
Hide resolved
...onents/learner-credit-management/data/hooks/useCatalogContainsContentItemsMultipleQueries.js
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/assignment-modal/NewAssignmentModalButton.jsx
Outdated
Show resolved
Hide resolved
} = useCatalogContainsContentItemsMultipleQueries( | ||
subsidyAccessPolicy.catalogUuid, | ||
// Pass only restricted runs. | ||
courseRuns?.filter(run => run.restrictionType === 'custom-e2e-enterprise'), // TODO: replace with constant |
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.
Should this be passing the course run keys vs. the entire course run objects? It appears the entire course run object is added in the query key vs. just the course run key
.
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 also wonder if it's worth abstracting a utility function for this filtering for restricted runs, given the same filtering logic is used in 2 places?
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.
Should this be passing the course run keys vs. the entire course run objects? It appears the entire course run object is added in the query key vs. just the course run key.
Yep, that definitely seems like a mistake on my part
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.
leaving open to remind myself to implement more DRYness on filtering restricted runs.
508b4c9
to
6c6932f
Compare
6c6932f
to
7bccdfe
Compare
ENT-9411