Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat send email students course #1

Closed
wants to merge 11 commits into from
Closed

feat send email students course #1

wants to merge 11 commits into from

Conversation

johnvente
Copy link

@johnvente johnvente commented Sep 11, 2023 •
edited
Loading

This feature will allow sending emails to expected emails of students in the course

DEMO

Currently without typehead

mfe-communications

With typehead
emails-communications

How to test it

If you are using devstack

  1. Go to workspace and clone this branch
cd ~/workspace && git clone -b jv/feat-send-email-students-course [email protected]:eduNEXT/frontend-app-communications.git
  1. Go to the mfe repository, use node version in the repo and install dependencies
cd ~/workspace/frontend-app-communications && nvm use && npm install
  1. inside frontend-app-communications run
npm run start
  1. Go to http://localhost:1984/courses/course-v1:edX+DemoX+Demo_Course/bulk_email Or
    http://localhost:1984/courses/{{id_course}}/bulk_email

If you are getting problems check this:

  1. Change this line

For this

const { data } = await Promise.resolve({ data: { cohorts: [] } });

in : src/components/page-container/data/api.js

  1. And this one

Which is like this:

in src/components/bulk-email-tool/BulkEmailTool.jsx

  {(courseMetadata) => (courseMetadata.originalUserIsStaff ? (

Change it to this:

  {(courseMetadata) => ( true ? (

@johnvente johnvente changed the title Jv/feat send email students course feat send email students course Sep 12, 2023
@mariajgrimaldi
Copy link
Collaborator

mariajgrimaldi commented Sep 12, 2023 •
edited
Loading

I'm getting this error after running npm start:

npm run start

> @edx/[email protected] start
> fedx-scripts webpack-dev-server --progress

Running with resolved config:
/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/webpack.dev.config.js

No local module configuration file found. This is fine.
Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme
<i> [webpack-dev-server] Project is running at:
<i> [webpack-dev-server] Loopback: http://localhost:1984/
<i> [webpack-dev-server] On Your Network (IPv4): http://192.168.0.121:1984/
<i> [webpack-dev-server] Content not from webpack is served from '/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/public' directory
<i> [webpack-dev-server] 404s will fallback to '/index.html'
10% building 0/1 entries 0/0 dependencies 0/0 modulesnode:internal/crypto/hash:67
  this[kHandle] = new _Hash(algorithm, xofLen);
                  ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:67:19)
    at Object.createHash (node:crypto:133:10)
    at BulkUpdateDecorator.hashFactory (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/util/createHash.js:145:18)
    at BulkUpdateDecorator.update (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/util/createHash.js:46:50)
    at OriginalSource.updateHash (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/node_modules/webpack-sources/lib/OriginalSource.js:130:8)
    at NormalModule._initBuildHash (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/NormalModule.js:880:17)
    at handleParseResult (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/NormalModule.js:946:10)
    at /home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/NormalModule.js:1040:4
    at processResult (/home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/NormalModule.js:755:11)
    at /home/mgmdi/Work/Envs/Devstack/frontend-app-communications/node_modules/webpack/lib/NormalModule.js:819:5 {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v18.0.0

Am I missing a step?

Update: it worked after running npm audit fix --force

mariajgrimaldi
mariajgrimaldi reviewed Sep 12, 2023
View reviewed changes
src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx Outdated
if (action === FORM_ACTIONS.POST) {
const emailData = new FormData();
emailData.append('action', 'send');
emailData.append('send_to', JSON.stringify(editor.emailRecipients));
emailData.append('subject', editor.emailSubject);
emailData.append('message', editor.emailBody);
emailData.append('email_learners', JSON.stringify(emailsFormat));
Copy link
Collaborator

@mariajgrimaldi mariajgrimaldi Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use: individual-student-emails?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or if learners are used across the app: individual-learners-emails

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use it with underscore individual_learners_emails

mariajgrimaldi
mariajgrimaldi reviewed Sep 12, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
mariajgrimaldi
mariajgrimaldi reviewed Sep 12, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
mariajgrimaldi
mariajgrimaldi reviewed Sep 12, 2023
View reviewed changes
src/components/bulk-email-tool/bulk-email-form/bulk-email-recipient/test/EmailList.test.jsx
const errorMessage = screen.getByText('Invalid email address');
expect(errorMessage).toBeInTheDocument();
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about these test cases:

  • Removing an email chip works
  • Sending an email with empty recipients fails

Copy link
Author

@johnvente johnvente Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Sending an email with empty recipients fails" it's out this component

mariajgrimaldi
mariajgrimaldi reviewed Sep 12, 2023
View reviewed changes
...omponents/bulk-email-tool/bulk-email-form/bulk-email-recipient/components/EmailList/utils.js Outdated
@@ -0,0 +1,5 @@
/* eslint-disable import/prefer-default-export */
export const isValidEmail = (email) => {
const emailRegex = /^(([^<>()·=~ºªÇ¨?¿*^|#¢∞¬÷"$%"≠´}{![\]\\.,;:\s@"]+(\.[^<>·$%&/=~ºªÇ¨?¿*^|#¢∞¬÷""≠´}{!()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same validation that the https://github.com/openedx/frontend-app-authn runs when registering a user (without custom registration rules)?

Copy link
Author

@johnvente johnvente Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have a regex for that as well here
but is not the same, that one doesn't allow uppercase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use that one? So we know it's maintained elsewhere. We can add a comment where we got it from

@johnvente johnvente marked this pull request as ready for review September 12, 2023 17:45
@johnvente johnvente force-pushed the jv/feat-send-email-students-course branch from eead98a to 3b14c1f Compare October 9, 2023 22:33
@johnvente johnvente force-pushed the jv/feat-send-email-students-course branch from 57c04b1 to 0f667ad Compare October 10, 2023 15:36
@e0d
Copy link

e0d commented Oct 18, 2023

@johnvente
I notice there are some commit-lint failures. Please note that we use conventional commits across Open edX projects. You can read about the details here. Can you please amend your commit messages to follow our standard?

@johnvente johnvente force-pushed the jv/feat-send-email-students-course branch from 56059c2 to 86b1575 Compare November 30, 2023 03:45
@johnvente johnvente force-pushed the jv/feat-send-email-students-course branch from 86b1575 to a68368b Compare November 30, 2023 04:00
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@johnvente
Copy link
Author

This PR won't be more maintained so I'm closing it

@johnvente johnvente closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariajgrimaldi mariajgrimaldi mariajgrimaldi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnvente @mariajgrimaldi @e0d @codecov-commenter