Skip to content

Removing function 'send_email' #2499

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

Merged
merged 4 commits into from
Oct 11, 2019
Merged

Removing function 'send_email' #2499

merged 4 commits into from
Oct 11, 2019

Conversation

deniszh
Copy link
Member

@deniszh deniszh commented Oct 11, 2019

It's insecure, not used in the code and undocumented.

Fixing #2008

Please also note that sending email from Dashboard is implemented in completely different commit 'e2a70d8' and not use 'send_email' function at all.

@deniszh deniszh requested review from piotr1212 and DanCech October 11, 2019 19:41
@deniszh deniszh requested a review from cbowman0 October 11, 2019 19:44
Copy link
Member

@cbowman0 cbowman0 left a comment

Choose a reason for hiding this comment

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

I know no reason removing this would cause a problem.

@JLLeitschuh
Copy link

JLLeitschuh commented Oct 11, 2019

You also need to remove the call to this endpoint from inside of the UI, unless that UI component uses some newer logic.

@deniszh
Copy link
Member Author

deniszh commented Oct 11, 2019

You also need to remove the call to this endpoint from inside of the UI, unless that UI component uses some newer logic.

That's a point - there's no call from UI exist. Or any other call. It looks like some abandoned piece of code, which was never used.

@cbowman0
Copy link
Member

Which UI component? I find no callers of send_email.

@DanCech
Copy link
Member

DanCech commented Oct 11, 2019

It looks like we can also get rid of the SMTP_SERVER config setting, which isn't used anywhere else.

@deniszh
Copy link
Member Author

deniszh commented Oct 11, 2019

@DanCech : well... Dashboard have ability to send email, see e2a70d8

@deniszh
Copy link
Member Author

deniszh commented Oct 11, 2019

It's probably also worth to backport it to 0.9.x and 1.0.x
Not sure should we make new releases for this branches... Maybe fixed branch should be enough.

@DanCech
Copy link
Member

DanCech commented Oct 11, 2019

Yes but that doesn't use that config setting, it uses the django EmailMessage class which is configured via the django EMAIL_* settings.

@deniszh
Copy link
Member Author

deniszh commented Oct 11, 2019

Yes but that doesn't use that config setting, it uses the django EmailMessage class which is configured via the django EMAIL_* settings.

Ah, nice find then, @DanCech
Removed together with unused imports.

@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #2499 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2499      +/-   ##
==========================================
- Coverage   80.01%   79.96%   -0.05%     
==========================================
  Files          85       85              
  Lines        9131     9091      -40     
  Branches     1948     1948              
==========================================
- Hits         7306     7270      -36     
+ Misses       1544     1540       -4     
  Partials      281      281
Impacted Files Coverage Δ
webapp/graphite/settings.py 74.75% <ø> (-0.13%) ⬇️
webapp/graphite/composer/views.py 23.8% <ø> (-31.75%) ⬇️
webapp/graphite/composer/urls.py 100% <ø> (ø) ⬆️

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 021cfe6...5a5f231. Read the comment docs.

Copy link
Member

@DanCech DanCech 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!

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.

5 participants