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

Fixes #723 : Greeting mail on user registration #724

Merged
merged 1 commit into from May 12, 2018

Conversation

yashLadha
Copy link
Member

Fixes : #723

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • I have added necessary documentation (if appropriate)

Preview Link

Changes proposed in this pull request:

  • Greeting mail is sent when a new user is saved.

@@ -28,3 +28,14 @@ def save_to_db(self):
@classmethod
def getUser(cls, username):
return cls.query.filter_by(username=username).first()

@db.event.listens_for(User, "after_insert")

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@codecov
Copy link

codecov bot commented May 11, 2018

Codecov Report

Merging #724 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           development   #724   +/-   ##
==========================================
  Coverage          100%   100%           
==========================================
  Files                1      1           
  Lines               43     43           
==========================================
  Hits                43     43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 957ce4b...bc01b57. Read the comment docs.

msg['body'] = "It's good to have you onboard with Badgeyay. Welcome to " \
"FOSSASIA Family."
res = send_mail(msg)
print(res)
Copy link
Member

Choose a reason for hiding this comment

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

delete this maybe?

Copy link
Member

@gabru-md gabru-md left a comment

Choose a reason for hiding this comment

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

Looks Good, just see to the camelCase changes I've suggested

@@ -1,17 +1,11 @@
from flask_mail import Mail, Message
from flask import current_app as app
from flask import jsonify
from models.user import User
from .response import Response


def send_mail(message):
Copy link
Member

Choose a reason for hiding this comment

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

how about using camelCase and set the name to sendMail ?



@db.event.listens_for(User, "after_insert")
def send_verification(mapper, connection, target):
Copy link
Member

Choose a reason for hiding this comment

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

camelCase here as well, maybe?

msg['receipent'] = sav_user.username
msg['body'] = "It's good to have you onboard with Badgeyay. Welcome to " \
"FOSSASIA Family."
res = sendMail(msg)

Choose a reason for hiding this comment

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

local variable 'res' is assigned to but never used

@yashLadha
Copy link
Member Author

Done @gabru-md

Copy link
Member

@gabru-md gabru-md left a comment

Choose a reason for hiding this comment

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

LGTM 💯


@db.event.listens_for(User, "after_insert")
def sendVerification(mapper, connection, target):
sav_user = target
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not using target directly?

Copy link
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

Nice work, please view my comment inside.

Copy link
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@djmgit djmgit merged commit 888709d into fossasia:development May 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Greeting mail send on User registration
4 participants