From 73cb7f3228e2f923f4a7a115db2287047b58446b Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Tue, 26 Jun 2018 16:07:59 -0400 Subject: [PATCH] endpoints/api: Allow null fields in user metadata The user metadata fields are nullable in the database, but were not in the json sechema. This prevented users from updating some of their information on the site if they hadn't set the metadata fields. --- data/model/user.py | 16 +++++++++---- endpoints/api/test/test_user.py | 42 +++++++++++++++++++++++++++++++++ endpoints/api/user.py | 32 ++++++++++--------------- static/js/pages/user-view.js | 4 ++-- static/partials/user-view.html | 2 +- util/saas/useranalytics.py | 2 +- 6 files changed, 71 insertions(+), 27 deletions(-) create mode 100644 endpoints/api/test/test_user.py diff --git a/data/model/user.py b/data/model/user.py index b19623cb8..a4f52837f 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -436,10 +436,18 @@ def update_user_metadata(user, metadata=None): metadata = metadata if metadata is not None else {} with db_transaction(): - user.given_name = metadata.get('given_name') or user.given_name - user.family_name = metadata.get('family_name') or user.family_name - user.company = metadata.get('company') or user.company - user.location = metadata.get('location') or user.location + if 'given_name' in metadata: + user.given_name = metadata['given_name'] + + if 'family_name' in metadata: + user.family_name = metadata['family_name'] + + if 'company' in metadata: + user.company = metadata['company'] + + if 'location' in metadata: + user.location = metadata['location'] + user.save() # Remove any prompts associated with the user's metadata being needed. diff --git a/endpoints/api/test/test_user.py b/endpoints/api/test/test_user.py new file mode 100644 index 000000000..bf31b0b6d --- /dev/null +++ b/endpoints/api/test/test_user.py @@ -0,0 +1,42 @@ +import pytest + +from mock import patch + +from endpoints.api.test.shared import conduct_api_call +from endpoints.api.user import User +from endpoints.test.shared import client_with_identity +from features import FeatureNameValue + +from test.fixtures import * + + +def test_user_metadata_update(client): + with patch('features.USER_METADATA', FeatureNameValue('USER_METADATA', True)): + with client_with_identity('devtable', client) as cl: + metadata = { + 'given_name': 'Quay', + 'family_name': 'User', + 'location': 'NYC', + 'company': 'Red Hat', + } + + # Update all user metadata fields. + conduct_api_call(cl, User, 'PUT', None, body=metadata) + + # Test that they were successfully updated. + user = conduct_api_call(cl, User, 'GET', None).json + for field in metadata: + assert user.get(field) == metadata.get(field) + + # Now nullify one of the fields, and remove another. + metadata['company'] = None + location = metadata.pop('location') + + conduct_api_call(cl, User, 'PUT', None, body=metadata) + + user = conduct_api_call(cl, User, 'GET', None).json + for field in metadata: + assert user.get(field) == metadata.get(field) + + # The location field should be unchanged. + assert user.get('location') == location diff --git a/endpoints/api/user.py b/endpoints/api/user.py index dde7bb15f..b81021f78 100644 --- a/endpoints/api/user.py +++ b/endpoints/api/user.py @@ -236,19 +236,19 @@ class User(ApiResource): 'description': 'Custom email address for receiving invoices', }, 'given_name': { - 'type': 'string', + 'type': ['string', 'null'], 'description': 'The optional entered given name for the user', }, 'family_name': { - 'type': 'string', + 'type': ['string', 'null'], 'description': 'The optional entered family name for the user', }, 'company': { - 'type': 'string', + 'type': ['string', 'null'], 'description': 'The optional entered company for the user', }, 'location': { - 'type': 'string', + 'type': ['string', 'null'], 'description': 'The optional entered location for the user', }, }, @@ -367,22 +367,16 @@ class User(ApiResource): model.user.update_email(user, new_email, auto_verify=not features.MAILING) if features.USER_METADATA: - metadata_fields = ('given_name', 'family_name', 'company', 'location') - if any(field in user_data for field in metadata_fields): - model.user.update_user_metadata(user, { - 'given_name': user_data.get('given_name'), - 'family_name': user_data.get('family_name'), - 'company': user_data.get('company'), - 'location': user_data.get('location'), - }) + metadata = {} - ua_mdata_future = user_analytics.change_metadata( - user.email, - user_data.get('given_name'), - user_data.get('family_name'), - user_data.get('company'), - user_data.get('location'), - ) + for field in ('given_name', 'family_name', 'company', 'location'): + if field in user_data: + metadata[field] = user_data.get(field) + + if len(metadata) > 0: + model.user.update_user_metadata(user, metadata) + + ua_mdata_future = user_analytics.change_metadata(user.email, **metadata) ua_mdata_future.add_done_callback(build_error_callback('Change metadata failed')) # Check for username rename. A username can be renamed if the feature is enabled OR the user diff --git a/static/js/pages/user-view.js b/static/js/pages/user-view.js index d6521d885..f38211276 100644 --- a/static/js/pages/user-view.js +++ b/static/js/pages/user-view.js @@ -146,7 +146,7 @@ $scope.updateMetadataInfo = function(info, callback) { var details = {}; - details[info.field] = info.value; + details[info.field] = (info.value === '' ? null : info.value); var errorDisplay = ApiService.errorDisplay('Could not update ' + info.title, callback); @@ -248,4 +248,4 @@ }; } -})(); \ No newline at end of file +})(); diff --git a/static/partials/user-view.html b/static/partials/user-view.html index 9d9aaa550..590c29095 100644 --- a/static/partials/user-view.html +++ b/static/partials/user-view.html @@ -211,7 +211,7 @@ dialog-form="context.metadataform">
Please enter an updated {{ changeMetadataInfo.title }}: - +
diff --git a/util/saas/useranalytics.py b/util/saas/useranalytics.py index 2540ea171..049097739 100644 --- a/util/saas/useranalytics.py +++ b/util/saas/useranalytics.py @@ -97,7 +97,7 @@ class _MarketoAnalyticsClient(object): lookupField='id', ) - def change_metadata(self, email, given_name, family_name, company, location): + def change_metadata(self, email, given_name=None, family_name=None, company=None, location=None): lead_data = self._get_lead_metadata(given_name, family_name, company, location) if not lead_data: return