Add user profile page content #2445

jwalgran · 2017-10-30T22:44:09Z

Overview

This commit adds a form on the "profile" tab of /account to display and edit user profile details.

Connects #2365

Demo

Notes

I moved the saving of the profile to a Django REST Framework serializer to keep the persistence details out of the view.
I styled the text fields on the form to match the wireframes but I did not style the bootstrap-select dropdown menus. We will be restyling them across the app in a future commit.

Testing Instructions

Reset your user profile

update user_userprofile set was_skipped = 'false', is_complete = 'false', organization = '', user_type = '', country='', postal_code = '' where user_id = (select id from auth_user where username = 'YOUR_USERNAME');

Log out, log in, and fully complete the profile form
Select "My Account" from the user dropdown at the upper right of the app. Verify that all the profile details are correctly displayed.
Refresh the page and verity that all the profile details are correct.
Change the first name, organization, and country fields and click Save changed. Verify that a spinner briefly appears.
Refresh the page and verify that the changes persist.
Stop the gunicorn server with vagrant ssh app -c 'sudo stop mmw-app'
Click Save changes and verify that an error message is displayed.

arottersman · 2017-10-31T14:44:40Z

Sorry, I think me merging #2438 caused some conflicts. There's also some js lint causing the build to fail.

jwalgran · 2017-10-31T17:38:12Z

Rebased on the latest develop with lint fixes.

rajadain · 2017-10-31T19:05:57Z

I'm also taking a look at this.

rajadain · 2017-10-31T19:26:15Z

I reset the user_profile table with the defaults given above, but am getting this error when filling out the profile again:

Internal Server Error: /user/profile
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/csrf.py", line 58, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py", line 71, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 452, in dispatch
    response = self.handle_exception(exc)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 449, in dispatch
    response = handler(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/decorators.py", line 50, in handler
    return func(*args, **kwargs)
  File "/opt/app/apps/user/views.py", line 112, in profile
    serializer.save()
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/serializers.py", line 165, in save
    self.instance = self.create(validated_data)
  File "/opt/app/apps/user/serializers.py", line 43, in create
    UserProfile.objects.update(**validated_data)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 563, in update
    rows = query.get_compiler(self.db).execute_sql(CURSOR)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/compiler.py", line 1062, in execute_sql
    cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/compiler.py", line 840, in execute_sql
    cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 79, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/dist-packages/django/db/utils.py", line 98, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
IntegrityError: duplicate key value violates unique constraint "user_userprofile_pkey"
DETAIL:  Key (user_id)=(1) already exists.

I'll drop all rows and try again.

rajadain · 2017-10-31T19:29:21Z

Hmm after dropping the rows, I'm getting this:

Internal Server Error: /user/profile
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/csrf.py", line 58, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py", line 71, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 452, in dispatch
    response = self.handle_exception(exc)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 449, in dispatch
    response = handler(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/decorators.py", line 50, in handler
    return func(*args, **kwargs)
  File "/opt/app/apps/user/views.py", line 112, in profile
    serializer.save()
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/serializers.py", line 165, in save
    self.instance = self.create(validated_data)
  File "/opt/app/apps/user/serializers.py", line 41, in create
    profile = UserProfile.create(user=user, was_skipped=False,
AttributeError: type object 'UserProfile' has no attribute 'create'

I may have borked the database. Will flush and recreate.

jwalgran · 2017-10-31T19:33:49Z

Not a database thing. That code is just wrong. It should be UserProfile.objects.create. Will fix.

rajadain · 2017-10-31T20:05:27Z

Can we add more padding around the error message?

It currently looks a little crowded.

jwalgran · 2017-10-31T20:45:54Z

Fixed spacing around the error messages. It applies to both profile and account pages

rajadain

Tested, works well. Leaving some style comments on the code, but overall this looks really good.

rajadain · 2017-10-31T20:31:53Z

src/mmw/js/src/account/views.js

+    },
+
+    initialize: function(){
+        this.listenTo(this.model, 'change', this.render);


This can be replaced with the Marionette modelEvents hash, like so:

events: { 'click @ui.saveChanges': 'saveChanges', }, modelEvents: { 'change': 'render', },

Addressed in fixup 2259d26

rajadain · 2017-10-31T20:36:55Z

src/mmw/js/src/account/views.js

+
+    onRender: function() {
+        var self = this;
+        function addOptions(model, field, selectedValue) {


For stylistic consistency, prefer defining this as a variable:

var self = this, addOptions = function(model, field, selectedValue) { ... };

Addressed in fixup 2259d26

rajadain · 2017-10-31T20:44:25Z

src/mmw/js/src/account/views.js

+            saving: true
+        },
+        {
+            success: function() {


For stylistic consistency, prefer defining this using done, fail, and always:

var model = this.model; model.save({ ... }) .done(function() { App.user.set('profile_is_complete', true); }) .fail(function() { model.set('error', 'There was a problem saving your profile.'); }) .always(function() { model.set('saving', false); });

Addressed in fixup 2259d26

rajadain · 2017-10-31T20:46:07Z

src/mmw/js/src/account/views.js

@@ -125,14 +183,17 @@ var AccountContainerView = Marionette.LayoutView.extend({

    initialize: function() {
        this.tokenModel = new models.ApiTokenModel();
+        this.profileModel = new userModels.UserProfileModel(App.user.attributes.profile);


Prefer App.user.get('profile') rather than directly accessing the attributes.

Addressed in fixup 2259d26

rajadain · 2017-10-31T20:48:11Z

src/mmw/js/src/user/models.js

@@ -60,6 +60,14 @@ var UserModel = Backbone.Model.extend({
 var UserProfileModel = Backbone.Model.extend({
    defaults: {
        was_skipped: false,


Is this was_skipped still used anywhere?

Addressed in fixup db7e61d

rajadain · 2017-10-31T20:48:24Z

src/mmw/js/src/user/models.js

-        last_name: null,
-        organization: null,
-        user_type: 'Unspecified',
-        country: 'US',
        was_skipped: false


How about this one?

Addressed in fixup db7e61d

jwalgran · 2017-10-31T22:52:48Z

Thanks for the excellent feedback. I have implemented all the suggestions and I have simplified the serializer to avoid the 500 errors.

arottersman

This looks good to me, and is working great!

I think not having the empty value of the country selector as Afghanistan might be worth doing, but otherwise +1

arottersman · 2017-11-01T13:41:17Z

src/mmw/js/src/account/views.js

+    onRender: function() {
+        var self = this,
+            addOptions = function(model, field, selectedValue) {
+                $.each(window.clientSettings.choices[model][field], function(index, choice) {


Consider prepending an unselected value, or defaulting to the US. It currently displays "Afghanistan" if you haven't given a country.

Also, consider importing core/settings instead of pulling it off the window.

Do you have a specific set of steps that resulted in the country defaulting to Afghanistan? US is specified as the default in both the backend and frontend and when I registered a new account it did show US as the default.

model-my-watershed/src/mmw/apps/user/models.py

Line 73 in 4b24200

default=countries.US)

model-my-watershed/src/mmw/js/src/user/templates/userProfileModal.html

Line 14 in 4b24200

{{ select(country, 'country', 'UserProfile', 'country', 'US', 'Country') }}

I will switch to an import for client settings.

I ran the query in your testing instructions and refreshed /account. I think we're setting the default for the modal, but not the profile page:

model-my-watershed/src/mmw/js/src/account/templates/profile.html

Line 55 in db7e61d

{{ select(country, 'country', 'UserProfile', 'country', country, 'Country') }}

Thanks. I'll dig into that.

Addressed in fixup 098ea6f

This still uses window.clientSettings, but I think that's okay.

Sorry. Lost commit. Actually fixed in the last rebase.

arottersman · 2017-11-01T13:45:02Z

src/mmw/js/src/account/views.js

+                });
+                self.$el.find('#' + field).selectpicker();
+            };
+        addOptions('UserProfile', 'country', self.model.attributes.country);


Another spot where you might consider using .get('country') like we do for the rest of the project.

Addressed in fixup 098ea6f

jwalgran · 2017-11-01T20:24:31Z

Thanks again for all the help. I think this is ready to go.

arottersman · 2017-11-01T21:09:04Z

Tested this out again and am seeing United States as the default.

arottersman · 2017-11-01T21:37:51Z

Just realized something while trying to recreate #2460:

If you have a "fresh" user with was_skipped=false and no profile, and while logged out, go directly to /account, you'll be shown the login modal. If you complete the profile form, you'll be redirected to the account page when complete. The account page will not reflect the info you just filled out until you refresh.

This is an edge case if you'd like to spin it off to a new card.

This commit adds a form on the "profile" tab of `/account` to display and edit user profile details. - I moved the saving of the profile to a Django REST Framework serializer to keep the persistence details out of the view. - I styled the text fields on the form to match the wireframes but I did not style the bootstrap-select dropdown menus. We will be restyling them across the app in a future commit.

jwalgran requested a review from arottersman October 30, 2017 22:44

jwalgran force-pushed the jcw/add-profile-to-account branch from f1f649c to a13d6c1 Compare October 31, 2017 17:35

rajadain reviewed Oct 31, 2017

View reviewed changes

tnation14 mentioned this pull request Nov 1, 2017

Release 1.21.0 #2305

Closed

arottersman approved these changes Nov 1, 2017

View reviewed changes

jwalgran mentioned this pull request Nov 1, 2017

Restyle onboarding modals #2462

Merged

21 tasks

jwalgran force-pushed the jcw/add-profile-to-account branch from 098ea6f to b2c0a72 Compare November 1, 2017 21:18

jwalgran force-pushed the jcw/add-profile-to-account branch from b2c0a72 to 81db33f Compare November 1, 2017 23:19

jwalgran merged commit 2c85897 into develop Nov 2, 2017

jwalgran deleted the jcw/add-profile-to-account branch November 2, 2017 01:32

Add user profile page content #2445

Add user profile page content #2445

Conversation

jwalgran commented Oct 30, 2017

Overview

Demo

Notes

Testing Instructions

arottersman commented Oct 31, 2017

jwalgran commented Oct 31, 2017

rajadain commented Oct 31, 2017

rajadain commented Oct 31, 2017

rajadain commented Oct 31, 2017

jwalgran commented Oct 31, 2017 • edited

rajadain commented Oct 31, 2017

jwalgran commented Oct 31, 2017

rajadain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalgran commented Oct 31, 2017

arottersman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalgran Nov 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalgran commented Nov 1, 2017

arottersman commented Nov 1, 2017

arottersman commented Nov 1, 2017

jwalgran commented Oct 31, 2017 •

edited

jwalgran Nov 1, 2017 •

edited