-
Notifications
You must be signed in to change notification settings - Fork 32
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
add support for optional salesforce ids in csv upload #1034
Conversation
bdd0ae0
to
1c20eb8
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1034 +/- ##
=========================================
Coverage ? 82.90%
=========================================
Files ? 412
Lines ? 8902
Branches ? 1825
=========================================
Hits ? 7380
Misses ? 1482
Partials ? 40
☔ View full report in Codecov by Sentry. |
26b8765
to
894ff12
Compare
894ff12
to
81051cf
Compare
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.
Seems reasonable, couple of small suggestions!
src/data/validation/email.js
Outdated
}; | ||
|
||
// Combine all the rows from textarea and CSV and then make a map of email to salesforce id | ||
const parseData = (formData) => { |
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: maybe name this getSalesforceIdsByEmail
'[email protected],000000000000YYYYYY', | ||
'[email protected],000000000000ZZZZZZ', | ||
'[email protected],000000000000ABCDDD', | ||
'[email protected],', |
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: consider adding one more test email in here without a trailing comma, like "[email protected]" to make sure the splitting/parsing still works as expected.
src/data/validation/email.js
Outdated
const allFalse = ids.every(item => !item); | ||
return allFalse ? undefined : ids; |
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: perhaps add a comment and/or rename this variable to explain this logic a bit more explicitly? It's not clear to me what allFalse
really means in this context.
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 check ensure that if there are no salesforce ids present then do not send an empty array to backend.
src/data/validation/email.js
Outdated
return validateEmailAddresses(emails).validEmails; | ||
|
||
rows.forEach((row) => { | ||
const [email, id] = row.split(',').map(item => item.trim()); |
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: is it perhaps worth renaming id
to salesforceId
or similar, so reading the code has a bit more context added to what type of id
it is (Salesforce).
src/data/validation/email.js
Outdated
let emails = extractEmails(formData).allEmails; | ||
emails = _.union(emails); // Dedup emails |
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: would const emails = _.union(extractEmails(formData.allEmails)) // Extract and dedup emails
be logically equivalent without needing to define and then re-define emails
?
src/data/validation/email.js
Outdated
// allEmails: Concatenation of `textEmails` and `CSVEmails` | ||
const extractEmails = (formData) => { | ||
let textEmails = []; | ||
let CSVEmails = []; |
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: csvEmails
return { | ||
textEmails: textEmails.length ? textEmails : undefined, | ||
CSVEmails: CSVEmails.length ? CSVEmails : undefined, | ||
allEmails, |
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] Given allEmails
will always return an array, even when empty, I'm wondering if all the properties should follow that convention for consistency, which might simplify this logic a bit.
return {
textEmails,
csvEmails,
allEmails,
}
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.
Initially I was returning the exact same object as you suggested but this line fails when textEmails is [] and csvEmails have some emails. For example the below code evaluates to []
which is not what is expected.
[] || ['[email protected]']
This test fails if we just return the below object
return {
textEmails,
csvEmails,
allEmails,
}
src/data/validation/email.js
Outdated
const csvEmails = formData[EMAIL_ADDRESS_CSV_FORM_DATA]; | ||
const extractedEmails = extractEmails(formData); | ||
const textAreaEmails = extractedEmails.textEmails; | ||
const csvEmails = extractedEmails.CSVEmails; | ||
let { | ||
invalidEmailIndices, | ||
} = validateEmailAddresses(textAreaEmails || csvEmails); |
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.
[curious/clarification] Do we only support textarea OR CSV upload? Aka, you can't provide textarea input AND CSV upload together? This logic here would imply that it's one or the other, but I wonder if that's actually what we should be doing as a product requirement. Might be worth clarifying (not blocking/related to this PR, though!).
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.
In existing logic, On the UI we say that either input emails in textarea or upload csv but in reality we pick all the emails from both textarea and csv and then send them to backend for license assignment.
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.
Thanks for clarifying!
f8b33ad
to
a00c3ec
Compare
a00c3ec
to
9ed5ab3
Compare
JIRA: https://2u-internal.atlassian.net/browse/ENT-7642
For all changes
Only if submitting a visual change