Migrated authentication and registration application from FBV to CBV #341

amruthasangeeth · 2016-06-15T20:32:25Z

Migrated authentication application from CBV using Django's auth views. It used function based views for login and logout process earlier. Registration application which allows administrator and volunteer signup is migrated to TemplateView which displays a signup form.

Referenced to #288

tapaswenipathak · 2016-06-16T07:39:51Z

@amruthasangeeth Tests are failing. :(

@smarshy Is this obvious that with this change tests will fail?

smarshy · 2016-06-16T09:56:18Z

@tapasweni-pathak There are failures across 80 tests, and most of them are showing unable to locate username and login panel. I think tests are accessing pages, which now no longer have the same content

@amruthasangeeth Have you changed urls?

amruthasangeeth · 2016-06-16T10:01:42Z

@smarshy Yes, The url syntax for CBV is different. There is no change in the url name or path. I had to change the function name to view name.

smarshy · 2016-06-16T14:53:52Z

@amruthasangeeth The functional tests are failing; they don't access the view names or function names. So any change you do there would not be a problem, unless you are making logical changes in the functionality. However, they do hit urls directly. Currently, login maps to authentication/login/ url. Can you please recheck?

smarshy · 2016-06-16T15:03:07Z

vms/vms/urls.py

@@ -16,4 +22,7 @@
    url(r'^registration/', include('registration.urls', namespace='registration')),
    url(r'^shift/', include('shift.urls', namespace='shift')),
    url(r'^volunteer/', include('volunteer.urls', namespace="volunteer")),
+    url(r'^portal', TemplateView.as_view(template_name='home/home.html'), name='home'),
+    url(r'^login/$', anonymous_required(auth_views.login), {'template_name': 'authentication/login.html'}, name='login_process'),


@amruthasangeeth I think urls are changing here. They have been shifted from the authentication app to the main urls.py file. Please recheck.

Yes, I have added the authentication urls to the main urls.py. Is it creating issues? Should I revert it?

@amruthasangeeth Any specific reason for this change?

I frequently come across this exception in the build. I think it is not getting directed to the correct login page due to change in url. If the url changes are not necessary, please revert them back to the initial ones. (link old urls itself to your new views) Also, I think there is a url already mapping to the home page in the urls.py. Isn't the portal/ url linking to the same template?

@smarshy Thats nice

tapaswenipathak · 2016-06-17T03:49:42Z

@smarshy @amruthasangeeth I think I messed up in the planning. Some changes will affect the test cases, no matter what. We need to reduce the rework.

@smarshy Ping me when you are free, on Slack. @amruthasangeeth is having her final year project presentation. We both will discuss and then we three will have a meeting.

amruthasangeeth · 2016-06-17T15:31:16Z

@smarshy @tapasweni-pathak Alright

tapaswenipathak · 2016-06-18T01:22:24Z

@amruthasangeeth I think I was not clear with my previous comment. You need to accomodate the changes that @smarshy is suggesting. You need to pass as many test cases as possible without affecting the functionality of the change you are doing.

Rework is for @smarshy, I remember she pointed out this in the start.

I have send both of you a mail describing the situation, please follow it. In case of confict or any help I will pitch in.

smarshy · 2016-06-18T02:56:28Z

@amruthasangeeth The ones failing are basically selenium tests. The way selenium webdriver is working is - it goes to the webpage, finds elements and triggers actions by interacting with them. Since the webdriver has no knowledge of the underlying views (what the names are etc.), so any changes done to refactor the view wouldn't cause problems.

But if there are changes made to the url, then the webdriver wouldn't be able to reach the page itself or if the template is changed, it might not be able to detect the elements. This is why there will be issues incase url/template is changed.
Another reason of failure might be if the logic is being affected/modified. In that case, if those functionality changes are desirable, i will make changes to the tests. Also, the same in case there are necessary changes needed in the template

I had a look at the entire build. This error keeps popping up -

from braces.views import LoginRequiredMixin, AnonymousRequiredMixin
ImportError: No module named braces.views

Are there packages which need to be installed to get this PR running? Please send me a list of the new ones then. I will need to update travis/requirements accordingly.
Once, these are fixed, build can be rerun to check if there are more errors.

amruthasangeeth · 2016-06-18T04:44:58Z

For the issue you mentioned about braces.views you need to install python-django-braces. I think that will work. And for the issue regarding the url change for authentication, can I make a PR to revert it back to the authentication/urls.py. I think we can have discussion on this.

smarshy · 2016-06-18T06:03:52Z

@amruthasangeeth Would there be a problem in changing the urls in this PR? Mapping the same views but to the original urls in urls.py. Because right now it is unconfirmed if these are the only reasons for failures. Once we complete changes, build needs to be re-run to see if issues are fixed.

tapaswenipathak · 2016-06-19T07:02:37Z

@amruthasangeeth I am fine with you making a new pull request. Creating a new pull request is the easiest thing to do but that also stops you from learning something new. Less rework is needed in updating this one. Try updating this pull request itself. You need to rebase the changes and then force push here.

Let me know if you need any help.

tapaswenipathak · 2016-06-19T07:06:31Z

@amruthasangeeth One more thing keep a note of the new packages that you are installing and write them in the description of the pull request. When you install it in your local machine, write it down so that you will not forget. There are many places where we are documenting them. Travis, our documentation, requirements file. Everything needs to updated.

Update the description of this pull request explaining why we need django-braces.

tapaswenipathak · 2016-06-29T14:45:47Z

@amruthasangeeth Did you get the time to fix this?

coveralls · 2016-07-11T18:34:59Z

Coverage decreased (-0.02%) to 94.131% when pulling 268a7d3298575cb2dd3843c679c919e0e03f19ea on amruthasangeeth:shiftRegToCBV into f3eda78 on systers:develop.

coveralls · 2016-07-11T19:07:09Z

Coverage decreased (-0.02%) to 94.131% when pulling 5c9d35e07e1bcd3878c16a1af0c05ce7ff6bcc2c on amruthasangeeth:shiftRegToCBV into f3eda78 on systers:develop.

coveralls · 2016-07-11T19:40:36Z

Coverage increased (+0.04%) to 94.195% when pulling 85d741849cf21da68bb934c8e52a156f65d0f5aa on amruthasangeeth:shiftRegToCBV into f3eda78 on systers:develop.

coveralls · 2016-07-11T19:44:34Z

Coverage decreased (-0.02%) to 94.131% when pulling 85d741849cf21da68bb934c8e52a156f65d0f5aa on amruthasangeeth:shiftRegToCBV into f3eda78 on systers:develop.

tapaswenipathak · 2016-07-12T14:26:12Z

vms/authentication/templates/authentication/login.html

@@ -62,6 +62,11 @@
                </div>
            </div>
        {% endif %}
+        {% if form.errors %}
+        <div class="alert alert-danger">
+            <i class="fa fa-warning"></i> "Your username and password didn't match. Please try again."


@amruthasangeeth Are we using fa fa icons anywhere in VMS? If not, it is better to show the message without this.

coveralls · 2016-07-12T20:13:36Z

Coverage decreased (-0.04%) to 94.135% when pulling 40ba7a56fc09c6280abe97d222fab09971ddaced on amruthasangeeth:shiftRegToCBV into 123f69b on systers:develop.

coveralls · 2016-07-15T17:51:24Z

Coverage decreased (-0.1%) to 82.631% when pulling 2dd8ac77b18e1b0efd90b3997baa4d711ccffcff on amruthasangeeth:shiftRegToCBV into 4436a63 on systers:develop.

amruthasangeeth · 2016-07-15T19:47:47Z

@tapasweni-pathak I have done the suggested changes. Can you please look at it.

tapaswenipathak · 2016-07-16T05:23:17Z

vms/authentication/templates/authentication/login.html

+        {% if form.errors %}
+        <div class="alert alert-danger">
+            "Your username and password didn't match. Please try again."
+        </div> <!-- alert alert-danger -->


@amruthasangeeth When I enter wrong username or/and password I get this. How can I get the above error on the UI?

@tapasweni-pathak I am getting the correct one. Let me check it. Or can you try it in incognito window

Migrated authentication application from CBV using Django's auth views. It used function based views for login and logout process earlier. Registration application is migrated to TemplateView based calls view which allows administrator and volunteer signup. Also implemented generating a message when username and password mismatches and phone number error. Updated requirements to avoid tests failure, and corrected URLs

coveralls · 2016-07-19T19:02:42Z

Coverage decreased (-0.1%) to 83.078% when pulling f6dc002 on amruthasangeeth:shiftRegToCBV into b69d88f on systers:develop.

amruthasangeeth · 2016-07-29T18:29:21Z

Closing down in favor of #367

smarshy reviewed Jun 16, 2016
View reviewed changes

This was referenced Jun 19, 2016

Added django-braces to requirements #343

Merged

Migrated administrator application from FBV to CBV #342

Merged

tapaswenipathak reviewed Jul 12, 2016
View reviewed changes

tapaswenipathak added the Needs changes label Jul 12, 2016

tapaswenipathak reviewed Jul 16, 2016
View reviewed changes

amruthasangeeth closed this Jul 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated authentication and registration application from FBV to CBV #341

Migrated authentication and registration application from FBV to CBV #341

amruthasangeeth commented Jun 15, 2016

tapaswenipathak commented Jun 16, 2016

smarshy commented Jun 16, 2016

amruthasangeeth commented Jun 16, 2016

smarshy commented Jun 16, 2016 •

edited

smarshy Jun 16, 2016

amruthasangeeth Jun 16, 2016 •

edited

tapaswenipathak Jun 18, 2016

smarshy Jun 18, 2016 •

edited

bryonglodencissp Jun 29, 2016

tapaswenipathak commented Jun 17, 2016

amruthasangeeth commented Jun 17, 2016

tapaswenipathak commented Jun 18, 2016

smarshy commented Jun 18, 2016 •

edited

amruthasangeeth commented Jun 18, 2016

smarshy commented Jun 18, 2016

tapaswenipathak commented Jun 19, 2016 •

edited

tapaswenipathak commented Jun 19, 2016 •

edited

tapaswenipathak commented Jun 29, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

tapaswenipathak Jul 12, 2016 •

edited

coveralls commented Jul 12, 2016

coveralls commented Jul 15, 2016

amruthasangeeth commented Jul 15, 2016

tapaswenipathak Jul 16, 2016

amruthasangeeth Jul 19, 2016

coveralls commented Jul 19, 2016

amruthasangeeth commented Jul 29, 2016

Migrated authentication and registration application from FBV to CBV #341

Migrated authentication and registration application from FBV to CBV #341

Conversation

amruthasangeeth commented Jun 15, 2016

tapaswenipathak commented Jun 16, 2016

smarshy commented Jun 16, 2016

amruthasangeeth commented Jun 16, 2016

smarshy commented Jun 16, 2016 • edited

smarshy Jun 16, 2016

Choose a reason for hiding this comment

amruthasangeeth Jun 16, 2016 • edited

Choose a reason for hiding this comment

tapaswenipathak Jun 18, 2016

Choose a reason for hiding this comment

smarshy Jun 18, 2016 • edited

Choose a reason for hiding this comment

bryonglodencissp Jun 29, 2016

Choose a reason for hiding this comment

tapaswenipathak commented Jun 17, 2016

amruthasangeeth commented Jun 17, 2016

tapaswenipathak commented Jun 18, 2016

smarshy commented Jun 18, 2016 • edited

amruthasangeeth commented Jun 18, 2016

smarshy commented Jun 18, 2016

tapaswenipathak commented Jun 19, 2016 • edited

tapaswenipathak commented Jun 19, 2016 • edited

tapaswenipathak commented Jun 29, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

tapaswenipathak Jul 12, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Jul 12, 2016

coveralls commented Jul 15, 2016

amruthasangeeth commented Jul 15, 2016

tapaswenipathak Jul 16, 2016

Choose a reason for hiding this comment

amruthasangeeth Jul 19, 2016

Choose a reason for hiding this comment

coveralls commented Jul 19, 2016

amruthasangeeth commented Jul 29, 2016

smarshy commented Jun 16, 2016 •

edited

amruthasangeeth Jun 16, 2016 •

edited

smarshy Jun 18, 2016 •

edited

smarshy commented Jun 18, 2016 •

edited

tapaswenipathak commented Jun 19, 2016 •

edited

tapaswenipathak commented Jun 19, 2016 •

edited

tapaswenipathak Jul 12, 2016 •

edited