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

added emails status across email calls taking place in views #216

Conversation

saisiddhant12
Copy link
Contributor

Commit wrt #215 for handling mail send and dont send feature

app/account/views.py Outdated Show resolved Hide resolved
PonteIneptique
PonteIneptique previously approved these changes Oct 6, 2020
Copy link
Member

@PonteIneptique PonteIneptique left a comment

Choose a reason for hiding this comment

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

Here you go :)

app/account/views.py Outdated Show resolved Hide resolved
app/account/views.py Outdated Show resolved Hide resolved
app/account/views.py Outdated Show resolved Hide resolved
app/account/views.py Outdated Show resolved Hide resolved
app/account/views.py Outdated Show resolved Hide resolved
@saisiddhant12
Copy link
Contributor Author

@PonteIneptique
Are we aware why the build is failing again and again

@PonteIneptique
Copy link
Member

@PonteIneptique
Are we aware why the build is failing again and again

Yup: #214 and #211 , unfortunately :/

invite_link=invite_link)
if not current_app.config["SEND_MAIL_STATUS"]:
Copy link
Member

Choose a reason for hiding this comment

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

@PonteIneptique
Copy link
Member

I cannot yet close the PR but I really want to thank you @saisiddhant12 for your work 👍

@PonteIneptique PonteIneptique self-assigned this Oct 6, 2020
@saisiddhant12
Copy link
Contributor Author

fixed the indentation issue, you can take a look now, let me know what more it takes to close the PR

Copy link
Member

@PonteIneptique PonteIneptique left a comment

Choose a reason for hiding this comment

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

If you can, a last commit: can you put back the secondary argument in flashes ? Just copy the original :)

'warning')
else:
flash('You are running in dev or test mode. Your account needs'
' to be confirmed via the command line interface or through the CLI')
Copy link
Member

Choose a reason for hiding this comment

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

I am sorry I did not catch this sooner: you are missing the secondary argument "warning" here:

            flash('You are running in dev or test mode. Your account needs'
            ' to be confirmed via the command line interface or through the CLI', 'warning')

@PonteIneptique
Copy link
Member

I am waiting for the CI but I tested it locally and it looks ok :) I'll keep it updated ;)

@PonteIneptique
Copy link
Member

I assume you're here for Hacktoberfest, do I need to something specific once the PR is merged ?

@saisiddhant12
Copy link
Contributor Author

Apart from the PR getting merged,
We need to add a label named as
hacktoberfest-accepted

@PonteIneptique
Copy link
Member

PonteIneptique commented Oct 6, 2020

There are two tests which do not pass anymore, I'll deal with them with a technical-debt :)

@PonteIneptique PonteIneptique merged commit 556e7af into hipster-philology:dev Oct 6, 2020
@PonteIneptique
Copy link
Member

Thank you very much @saisiddhant12 :)
Let me know if I can do more, and have a nice hacktoberfest !

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.

2 participants