-
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
feat: modifies logic for setting cookies on dashboard page load #881
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #881 +/- ##
==========================================
+ Coverage 84.87% 85.02% +0.14%
==========================================
Files 320 328 +8
Lines 6399 6883 +484
Branches 1552 1685 +133
==========================================
+ Hits 5431 5852 +421
- Misses 941 1000 +59
- Partials 27 31 +4 ☔ View full report in Codecov by Sentry. |
const learnerContentAssignmentsArray = learnerCreditPolicies?.flatMap( | ||
item => item?.learnerContentAssignments || [], | ||
); |
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, non-blocking] Looks like logic to generate a flat map of assignments based on the learner credit policies happens in two places now. I'm wondering if it'd be worth abstracting this logic out to where we get the response about redeemable learner credit policies from the credits_available
API so components such as this one don't need to run their own .flatMap
to get a list of assignments.
E.g., useRedeemableLearnerCreditPolicies
and its query function getRedeemablePoliciesData
could return a data structure more along the lines of the following, where it splits out redeemablePolicies
vs. learnerContentAssignments
:
const getRedeemablePoliciesData = async ({ queryKey }) => {
const enterpriseId = queryKey[1];
const userID = queryKey[2];
const response = await fetchRedeemableLearnerCreditPolicies(enterpriseId, userID);
const redeemablePolicies = camelCaseObject(transformRedeemablePoliciesData(response.data));
const learnerContentAssignments = redeemablePolicies.flatMap(policy => policy.learnerContentAssignments);
return {
redeemablePolicies,
learnerContentAssignments,
};
};
export function useRedeemableLearnerCreditPolicies(enterpriseId, userID) {
return useQuery({
queryKey: ['redeemablePolicies', enterpriseId, userID],
queryFn: getRedeemablePoliciesData,
onError: (error) => {
logError(error);
},
});
}
Any component that relies on an assignments list could rely on learnerContentAssignments
returned by the hook instead of having to re-compute the .flatMap
each time. Then, the same line in the isDisableCourseSearch
function could be simplified to:
const assignments = redeemableLearnerCreditPolicies?.learnerContentAssignments;
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.
Opted to resolve this in a followup PR to focus on the MVP, but intended to resolve immediately following the validation of functionality of this PR in stage/prod environment. Updated ticket to document the decision.
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
src/components/enterprise-redirects/EnterpriseLearnerFirstVisitRedirect.jsx
Outdated
Show resolved
Hide resolved
{ | ||
learnerContentAssignments: { | ||
state: 'accepted', | ||
}, |
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: should we include assignment(s) with state='errored'
and state='cancelled'
as well?
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.
Deferred on the errored state for now since the repo doesn't handle the error state at the moment, but the following PR with the optimizations will handle the error state. Will leave comment open to reference. 👍🏽
src/components/enterprise-redirects/tests/EnterpriseLearnerFirstVisitRedirect.test.jsx
Outdated
Show resolved
Hide resolved
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 more nit-level comments. Great job!
Modifies the logic for redirecting the page to the search page based on whether the
has-user-visited-learner-dashboard
cookie has been set to take into account whether active course assignments exist.This involves hoisting some of the logic from
CourseEnrollments.jsx
intoDashboardPage.jsx
to avoid partial render before redirect.For all changes
Only if submitting a visual change