feat: select group learners from table individually #1438
Conversation
marlonkeating
force-pushed
the
mkeating/ENT-10065
branch
from
2039fcc to
cacd981
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1438 +/- ##
==========================================
+ Coverage 86.41% 86.62% +0.21%
==========================================
Files 660 661 +1
Lines 14934 14862 -72
Branches 3109 3164 +55
==========================================
- Hits 12905 12874 -31
+ Misses 1965 1917 -48
- Partials 64 71 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
marlonkeating
force-pushed
the
mkeating/ENT-10065
branch
from
cacd981 to
4e8e1c4
Compare
fix: ValidateEmailsContextProvider file reference fix: unit tests and analytics fix: console warning chore: remove unused code
marlonkeating
force-pushed
the
mkeating/ENT-10065
branch
from
4e8e1c4 to
a26edb4
Compare
| allEnterpriseLearners?: string[], | ||
| groupEnterpriseLearners?: string[], |
There was a problem hiding this comment.
[question] can we set this to default to an empty array [] rather than undefined if the fields are always expected to be arrays even if they're empty?
| ValidatedEmailsReducer, | ||
| { ...initialContext, ...initialContextOverride }, | ||
| ); | ||
| validatedEmailsState.dispatch = dispatch; |
There was a problem hiding this comment.
[question (non-blocking)] is there a reason why we are manually adding dispatch? this seems a bit unconventional. could we instead manage dispatch outside of the context to something like this?
const value = {
...validatedEmailsState,
dispatch,
};
| const emails = newState.lowerCasedEmails as string[]; | ||
| newState.lowerCasedEmails = removeStringsFromList(emails, removedEmails); | ||
| return { ...newState }; | ||
| } default: { |
There was a problem hiding this comment.
[suggestion (non-blocking)]: looks like this reducer assumes that all inputs/data are valid but can we consider adding a log in the default to catch any unexpected actions/errors?
| if (actionType === 'CLICK_ACTION') { | ||
| newState.isCreateGroupListSelection = true; | ||
| } | ||
| if (actionType === 'UPLOAD_CSV_ACTION') { | ||
| newState.isCreateGroupFileUpload = true; | ||
| } |
There was a problem hiding this comment.
[suggestion (non-blocking)]:
const updatedState = {
...newState,
isCreateGroupListSelection: actionType === 'CLICK_ACTION',
isCreateGroupFileUpload: actionType === 'UPLOAD_CSV_ACTION',
};
| if (clearErroredEmails) { | ||
| newState.duplicateEmails = []; | ||
| newState.emailsNotInOrg = []; | ||
| newState.invalidEmails = []; | ||
| emails = [...(newState.lowerCasedEmails || []), ...addedEmails]; | ||
| } else { | ||
| emails = [...allEmails(newState), ...addedEmails]; | ||
| } |
There was a problem hiding this comment.
[suggestion (non-blocking)]: could we extract this into a helper function to merge all the emails?
const getUpdatedEmails = (newState, addedEmails, clearErroredEmails) => {
if (clearErroredEmails) {
newState.duplicateEmails = [];
newState.emailsNotInOrg = [];
newState.invalidEmails = [];
emails = [...(newState.lowerCasedEmails || []), ...addedEmails];
}
return [...allEmails(newState), ...addedEmails];
}
const emails = getUpdatedEmails(updatedState, addedEmails, clearErroredEmails)
| } | ||
| const emailValidation = isInviteEmailAddressesInputValueValid({ | ||
| learnerEmails: emails, | ||
| allEnterpriseLearners: newState.allEnterpriseLearners as string[], |
There was a problem hiding this comment.
this can be updated to allEnterpriseLearners: updatedState.allEnterpriseLearners
| learnerEmails: emails, | ||
| allEnterpriseLearners: newState.allEnterpriseLearners as string[], | ||
| }); | ||
| return { ...newState, ...emailValidation }; |
There was a problem hiding this comment.
updated to return { ...updatedState, ...emailValidation }
Jira Ticket
Functionality-wise, this change modifies the group member management modals to add/remove group members immediately when checkbox is clicked, rather than having to check a batch and click the Add/Remove bulk action buttons.
As part of this work, there is an extensive refactoring of the email validation logic out of individual components and into a central Context store.
Testing
Setup
npm run start:stageCreate Group
Add/Remove Group Members
For all changes
Only if submitting a visual change