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.

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

Adding mailing service to the codebase #577

Merged
merged 22 commits into from
Apr 19, 2021

Conversation

endurance21
Copy link
Contributor

Problem

There was no mailing agent dedicated for the codebase.

Solution

Implement one such agent.
This pr deals with setting up node-mailer as our mailing agent.

Areas of Impact

server
global config file

@endurance21 endurance21 changed the title Feature/mailer Adding mailing service to the codebase Mar 17, 2021
src/server/helpers/mailer.js Outdated Show resolved Hide resolved
@endurance21
Copy link
Contributor Author

@MonkeyDo lint tests are failing , and one of them is due to
Async function 'sendEmail' has no 'await' expression
should i bypass this?
or should i remove async keyword cz anyway this function gonna return promise 🤔

@MonkeyDo
Copy link
Member

@MonkeyDo lint tests are failing , and one of them is due to
Async function 'sendEmail' has no 'await' expression
should i bypass this?
or should i remove async keyword cz anyway this function gonna return promise 🤔

Removing the async keyword is the way to go here.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

You'll also need to commit the package-lock.json file in order for the tests to pass.

@endurance21
Copy link
Contributor Author

done the changes , please have a look @MonkeyDo

@coveralls
Copy link

coveralls commented Mar 25, 2021

Coverage Status

Coverage decreased (-0.02%) to 60.9% when pulling f2aa07a on endurance21:feature/mailer into 95365c5 on bookbrainz:master.

@endurance21
Copy link
Contributor Author

i have run npm run lint locally but found no error !
how this test is not passing

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Not sure what's happening with the linter, but we'll see if the next push passes. If not we'll ignore them as the dependency is indeed in package.json

src/server/helpers/mailer.js Outdated Show resolved Hide resolved
@endurance21
Copy link
Contributor Author

@MonkeyDo
i have done the chnages , looking forward for the process :)

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Looking pretty good !

The file mailer.js needs to be removed, now that you've created the .ts file.

src/server/helpers/mailer.ts Outdated Show resolved Hide resolved
src/server/helpers/mailer.ts Outdated Show resolved Hide resolved
src/server/helpers/mailer.ts Outdated Show resolved Hide resolved
src/server/helpers/mailer.ts Outdated Show resolved Hide resolved
src/server/helpers/mailer.ts Outdated Show resolved Hide resolved
endurance21 and others added 2 commits April 7, 2021 04:54
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
endurance21 and others added 4 commits April 7, 2021 04:55
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
@endurance21
Copy link
Contributor Author

Have to lint it properly once !

@endurance21
Copy link
Contributor Author

@MonkeyDo i think this one is ready to go !

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Great, thanks !

@MonkeyDo MonkeyDo merged commit f80f0ae into metabrainz:master Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants