-
Notifications
You must be signed in to change notification settings - Fork 56
Removed django-registration-redux #106
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
Removed django-registration-redux #106
Conversation
('Ibrahim Jarif','jarifibrahim@gmail.com'), | ||
('Tapasweni Pathak', 'tapaswenipathak@gmail.com'), | ||
('Nikhita Raghunath', 'nikitaraghunath@gmail.com'), | ||
('Ibrahim Jarif', 'jarifibrahim@gmail.com'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vaibhavsingh97 Please remove these spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anubhakushwaha According to PEP 8, we should have spaces. If you want i can remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vaibhavsingh97 Ooh that's interesting, we would love to learn more,Is this the place? https://www.python.org/dev/peps/pep-0008/#tabs-or-spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oshc/oshc/settings.py
Outdated
LOGIN_REDIRECT_URL = 'home' | ||
# and are trying to access pages requiring authentication | ||
LOGIN_URL = '/accounts/login/' | ||
LOGOUT_REDIRECT_URL = 'home' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vaibhavsingh97 Remove this extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the minor changes, else LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unit tests for now
@anubhakushwaha I had made suggested change. Please merge |
@vaibhavsingh97 Just mention the reason for the decrease in coverage for everyone and it's ready to be merged. |
@anubhakushwaha @tapasweni-pathak @nikhita Please find the detailed report 📓 Reason for decrease in coverageBefore removal of test case:
Statement covered: 102 After removal of test case:
Statement covered: 51 |
Merging, keeping the two commits separate just as a reminder that we have removed the test cases as well. |
Status:
Which issue does this PR fix?:
fixes #104
Brief description of what this PR does:
This PR is for removing django-registration-redux model.