Skip to content

Added ADMINS to settings.py #64

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 2 commits into from
Jul 9, 2017

Conversation

Utsal20
Copy link

@Utsal20 Utsal20 commented Jul 8, 2017

Fixes #49 completed. Admins are added to settings.py

@nikhita nikhita requested a review from jarifibrahim July 8, 2017 08:48
@nikhita
Copy link
Contributor

nikhita commented Jul 8, 2017

@Utsal20 Thanks! I'll let @jarifibrahim take a look at it too before merging it. 👍

@tapaswenipathak
Copy link
Member

tapaswenipathak commented Jul 8, 2017

@Utsal20 Can you please add @amarlearning ([email protected]) too? and then this can be merged.

@nikhita
Copy link
Contributor

nikhita commented Jul 8, 2017

@Utsal20 I modified the PR description to read Fixes #49 instead of Issue #49. When we write like this, it automatically closes the issue once this PR is merged! 😄

More details here: https://github.com/blog/1506-closing-issues-via-pull-requests, https://help.github.com/articles/closing-issues-via-commit-messages/.

@nikhita nikhita self-assigned this Jul 8, 2017
Copy link
Member

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

LGTM. I'll merge after you add Amar Prakash Pandey ([email protected]) to Admins.

@@ -24,6 +24,9 @@

ALLOWED_HOSTS = ['*']

# Tuple of people who get error notifications
ADMINS = [('Tapasweni Pathak','[email protected]'),('Nikhita Raghunath','[email protected]'),('Ibrahim Jarif','[email protected]')]

Copy link
Member

@amarlearning amarlearning Jul 8, 2017

Choose a reason for hiding this comment

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

I think we should split the list something like this for better readability. [more read]

ADMINS = [
    ('Tapasweni Pathak','[email protected]'),
    ('Nikhita Raghunath','[email protected]'),
    ('Ibrahim Jarif','[email protected]')
]

Extraneous change. Having two consecutive new lines is not a good practice. [more read]

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestions!
I just did:
git add oshc/oshc/settings.py
git push -u origin master
Will this be enough? Or is there something I am missing?

@tapaswenipathak
Copy link
Member

Can I please merge this one @amarlearning? or you would suggest @Utsal20 to squash commits? 😄

I see it's @Utsal20's first pull request. ✨

@tapaswenipathak
Copy link
Member

I am merging this sorry, can't wait. Congrats for your first pull request @Utsal20! 🎆

But @Utsal20 there is something as squashing commits in which if you have fixed your previous commit or you have many related commits separate you can squash them as one. I am sure these two would have suggested you to do this before merging, so surely have a read, for all next pull requests you have to. :)

@tapaswenipathak tapaswenipathak merged commit 7c0467e into OpenSourceHelpCommunity:master Jul 9, 2017
tapaswenipathak added a commit that referenced this pull request Jul 9, 2017
@Utsal20
Copy link
Author

Utsal20 commented Jul 9, 2017

Thank you so much for your support guys! Will do the reading.

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