add support for optional salesforce ids in csv upload #1034

muhammad-ammar · 2023-09-19T14:00:43Z

JIRA: https://2u-internal.atlassian.net/browse/ENT-7642

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

codecov · 2023-09-20T05:50:27Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5cb3eeb). Click here to learn what that means.
Patch coverage: 91.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1034   +/-   ##
=========================================
  Coverage          ?   82.90%           
=========================================
  Files             ?      412           
  Lines             ?     8902           
  Branches          ?     1825           
=========================================
  Hits              ?     7380           
  Misses            ?     1482           
  Partials          ?       40

Files Changed	Coverage Δ
src/components/InviteLearnersModal/index.jsx	`39.21% <0.00%> (ø)`
src/data/validation/email.js	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/components/InviteLearnersModal/index.jsx

src/data/validation/email.js

iloveagent57

Seems reasonable, couple of small suggestions!

iloveagent57 · 2023-09-22T15:19:55Z

src/data/validation/email.js

+};
+
+// Combine all the rows from textarea and CSV and then make a map of email to salesforce id
+const parseData = (formData) => {


nit: maybe name this getSalesforceIdsByEmail

iloveagent57 · 2023-09-22T15:23:15Z

src/data/validation/email.test.js

+        '[email protected],000000000000YYYYYY',
+        '[email protected],000000000000ZZZZZZ',
+        '[email protected],000000000000ABCDDD',
+        '[email protected],',


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.

adamstankiewicz · 2023-09-22T15:33:05Z

src/data/validation/email.js

+  const allFalse = ids.every(item => !item);
+  return allFalse ? undefined : ids;


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.

this check ensure that if there are no salesforce ids present then do not send an empty array to backend.

src/components/InviteLearnersModal/index.jsx

adamstankiewicz · 2023-09-22T15:37:47Z

src/data/validation/email.js

-  return validateEmailAddresses(emails).validEmails;
+
+  rows.forEach((row) => {
+    const [email, id] = row.split(',').map(item => item.trim());


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).

adamstankiewicz · 2023-09-22T15:39:42Z

src/data/validation/email.js

+  let emails = extractEmails(formData).allEmails;
+  emails = _.union(emails); // Dedup emails


nit: would const emails = _.union(extractEmails(formData.allEmails)) // Extract and dedup emails be logically equivalent without needing to define and then re-define emails?

adamstankiewicz · 2023-09-22T15:41:39Z

src/data/validation/email.js

+//      allEmails: Concatenation of `textEmails` and `CSVEmails`
+const extractEmails = (formData) => {
+  let textEmails = [];
+  let CSVEmails = [];


nit: csvEmails

adamstankiewicz · 2023-09-22T15:43:02Z

src/data/validation/email.js

+  return {
+    textEmails: textEmails.length ? textEmails : undefined,
+    CSVEmails: CSVEmails.length ? CSVEmails : undefined,
+    allEmails,


[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, }

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, }

adamstankiewicz · 2023-09-22T15:44:47Z

src/data/validation/email.js

-  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);


[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!).

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.

Thanks for clarifying!

muhammad-ammar marked this pull request as draft September 19, 2023 14:01

muhammad-ammar marked this pull request as ready for review September 20, 2023 05:30

muhammad-ammar force-pushed the ammar/extract-and-use-sfids branch 2 times, most recently from bdd0ae0 to 1c20eb8 Compare September 20, 2023 05:39

muhammad-ammar force-pushed the ammar/extract-and-use-sfids branch 7 times, most recently from 26b8765 to 894ff12 Compare September 22, 2023 10:22

zamanafzal reviewed Sep 22, 2023

View reviewed changes

src/components/InviteLearnersModal/index.jsx Outdated Show resolved Hide resolved

zamanafzal reviewed Sep 22, 2023

View reviewed changes

src/data/validation/email.js Outdated Show resolved Hide resolved

zamanafzal reviewed Sep 22, 2023

View reviewed changes

src/data/validation/email.js Outdated Show resolved Hide resolved

muhammad-ammar force-pushed the ammar/extract-and-use-sfids branch from 894ff12 to 81051cf Compare September 22, 2023 10:47

muhammad-ammar requested a review from adamstankiewicz September 22, 2023 14:54

muhammad-ammar assigned iloveagent57 and unassigned iloveagent57 Sep 22, 2023

muhammad-ammar requested a review from iloveagent57 September 22, 2023 14:55

iloveagent57 approved these changes Sep 22, 2023

View reviewed changes

adamstankiewicz reviewed Sep 22, 2023

View reviewed changes

adamstankiewicz mentioned this pull request Sep 22, 2023

feat: update license assign view to set source for assigned licenses openedx/license-manager#526

Merged

muhammad-ammar force-pushed the ammar/extract-and-use-sfids branch 2 times, most recently from f8b33ad to a00c3ec Compare September 25, 2023 07:05

adamstankiewicz approved these changes Sep 25, 2023

View reviewed changes

feat: add support for optional salesforce ids in csv upload

9ed5ab3

muhammad-ammar force-pushed the ammar/extract-and-use-sfids branch from a00c3ec to 9ed5ab3 Compare September 25, 2023 16:18

muhammad-ammar merged commit e12f09a into master Sep 25, 2023
6 checks passed

muhammad-ammar deleted the ammar/extract-and-use-sfids branch September 25, 2023 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for optional salesforce ids in csv upload #1034

add support for optional salesforce ids in csv upload #1034

muhammad-ammar commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading

iloveagent57 left a comment

iloveagent57 Sep 22, 2023

iloveagent57 Sep 22, 2023

adamstankiewicz Sep 22, 2023 •

edited

Loading

muhammad-ammar Sep 22, 2023

adamstankiewicz Sep 22, 2023

adamstankiewicz Sep 22, 2023

adamstankiewicz Sep 22, 2023

adamstankiewicz Sep 22, 2023

muhammad-ammar Sep 22, 2023 •

edited

Loading

adamstankiewicz Sep 22, 2023

muhammad-ammar Sep 22, 2023

adamstankiewicz Sep 25, 2023

		const allFalse = ids.every(item => !item);
		return allFalse ? undefined : ids;

		let emails = extractEmails(formData).allEmails;
		emails = _.union(emails); // Dedup emails

add support for optional salesforce ids in csv upload #1034

add support for optional salesforce ids in csv upload #1034

Conversation

muhammad-ammar commented Sep 19, 2023 • edited Loading

For all changes

Only if submitting a visual change

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

iloveagent57 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading

adamstankiewicz Sep 22, 2023 •

edited

Loading

muhammad-ammar Sep 22, 2023 •

edited

Loading