From 510e465d8b01c3632aeb19612ca26483c7a3b611 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Fri, 13 Jul 2018 12:50:45 -0400 Subject: [PATCH 01/20] add allow jobs and schools attributes to be updated and validated --- api/users/serializers.py | 80 ++++++++++++++++++++++++++++++++++++++-- osf/models/validators.py | 2 +- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index 83bea628dab..7e995cc963a 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -1,3 +1,5 @@ +import datetime + from guardian.models import GroupObjectPermission from django.utils import timezone @@ -7,8 +9,8 @@ from api.base.serializers import ( BaseAPISerializer, JSONAPISerializer, JSONAPIRelationshipSerializer, VersionedDateTimeField, HideIfDisabled, IDField, - Link, LinksField, ListDictField, TypeField, RelationshipField, - WaterbutlerLink, ShowIfCurrentUser, DevOnly + Link, LinksField, ListDictField, TypeField, RelationshipField, JSONAPIListField, + WaterbutlerLink, ShowIfCurrentUser ) from api.base.utils import absolute_reverse, get_user_auth, waterbutler_api_url_for from api.files.serializers import QuickFilesSerializer @@ -57,8 +59,10 @@ class UserSerializer(JSONAPISerializer): timezone = HideIfDisabled(ser.CharField(required=False, help_text="User's timezone, e.g. 'Etc/UTC")) locale = HideIfDisabled(ser.CharField(required=False, help_text="User's locale, e.g. 'en_US'")) social = ListDictField(required=False) - employment = DevOnly(ser.ListField(child=ser.DictField(), source='jobs', read_only=True)) - education = DevOnly(ser.ListField(child=ser.DictField(), source='schools', read_only=True)) + employment = JSONAPIListField(required=False, source='jobs') + employment_required_keys = {'department', 'institution', 'ongoing', 'startMonth', 'startYear', 'title'} + education = JSONAPIListField(required=False, source='schools') + education_required_keys = {'department', 'institution', 'ongoing', 'startMonth', 'startYear', 'degree'} can_view_reviews = ShowIfCurrentUser(ser.SerializerMethodField(help_text='Whether the current user has the `view_submissions` permission to ANY reviews provider.')) accepted_terms_of_service = ShowIfCurrentUser(ser.SerializerMethodField()) @@ -128,6 +132,67 @@ def profile_image_url(self, user): size = self.context['request'].query_params.get('profile_image_size') return user.profile_image_url(size=size) + def _update_social(self, social_dict, user): + for key, val in social_dict.items(): + # currently only profileWebsites are a list, the rest of the social key only has one value + if key == 'profileWebsites': + user.social[key] = val + else: + if len(val) > 1: + raise InvalidModelValueError( + detail='{} only accept a list of one single value'.format(key) + ) + user.social[key] = val[0] + + def _validate_user_json(self, info_list, required_fields): + if type(info_list) != list or not info_list: + raise InvalidModelValueError(detail='The updated information must be in a list of dictionaries.') + + for info in info_list: + + if type(info) != dict: + raise InvalidModelValueError(detail='The updated information must be in a list of dictionaries.') + + fields = set(info.keys()) + + absent_fields = required_fields.difference(fields) + if absent_fields: + raise InvalidModelValueError( + detail='The updated employment fields must contain keys {}.'.format(', '.join(absent_fields)) + ) + + if type(info['ongoing']) != bool: + raise InvalidModelValueError(detail='The field "ongoing" must be boolean.') + + if not info['ongoing']: + required_fields = required_fields.union({'endYear', 'endMonth'}) + + extra_fields = required_fields.symmetric_difference(fields) + if extra_fields: + raise InvalidModelValueError( + detail='The updated employment fields can only contain keys {}.'.format(', '.join(required_fields)) + ) + + if not info.get('institution'): + raise InvalidModelValueError(detail='The institution field cannot be empty.') + + self._validate_dates(info) + + def _validate_dates(self, info): + try: + startDate = datetime.date(info['startYear'], info['startMonth'], 1) + except (ValueError, TypeError): + raise InvalidModelValueError(detail='Date values must be valid integers.') + + if not info['ongoing']: + try: + endDate = datetime.date(info['endYear'], info['endMonth'], 1) + except (ValueError, TypeError): + raise InvalidModelValueError(detail='Date values must be valid integers.') + + if (endDate - startDate).days <= 0: + raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') + def update(self, instance, validated_data): assert isinstance(instance, OSFUser), 'instance must be a User' for attr, value in validated_data.items(): @@ -142,6 +207,13 @@ def update(self, instance, validated_data): detail='{} only accept a list of one single value'. format(key) ) instance.social[key] = val[0] + elif 'schools' == attr: + self._validate_user_json(value, self.education_required_keys) + instance.schools = value + elif 'jobs' == attr: + self._validate_user_json(value, self.employment_required_keys) + instance.jobs = value + elif 'accepted_terms_of_service' == attr: if value and not instance.accepted_terms_of_service: instance.accepted_terms_of_service = timezone.now() diff --git a/osf/models/validators.py b/osf/models/validators.py index b5339632f9e..25cbb2349f5 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -38,7 +38,7 @@ def validate_year(item): except ValueError: raise ValidationValueError('Please enter a valid year.') else: - if len(item) != 4: + if type(item) == str and len(item) != 4: raise ValidationValueError('Please enter a valid year.') From b6cf74896489494fe62d0ef1182a8264bb85aaf0 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Fri, 13 Jul 2018 12:51:22 -0400 Subject: [PATCH 02/20] add tests for v2 updating of jobs and schools attributes --- api_tests/users/views/test_user_detail.py | 461 +++++++++++++++++++++- 1 file changed, 459 insertions(+), 2 deletions(-) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 2fa92aac568..37d2362b656 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -842,8 +842,465 @@ def test_partial_put_user_logged_in(self, app, user_one, url_user_one): assert user_one.suffix == 'The Millionth' assert user_one.social['github'] == 'even_newer_github' - def test_put_user_logged_in( - self, app, user_one, data_new_user_one, url_user_one): + def test_user_put_schools_200(self, app, user_one, url_user_one): + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth) + user_one.reload() + assert res.status_code == 200 + assert user_one.schools == [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + + def test_user_put_schools_401(self, app, user_one, url_user_one): + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, expect_errors=True) + assert res.status_code == 401 + assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' + + def test_user_put_schools_validation(self, app, user_one, url_user_one): + # Tests to make sure schools fields correct structure + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': {} + }, + } + }, auth=user_one.auth, expect_errors=True) + + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' + + # Tests to make sure structure is lists of dicts + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated information must be in a list of dictionaries.' + + # Tests to make sure structure is lists of dicts consisting of proper fields + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{}] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated employment fields must contain keys degree, startYear,' \ + ' startMonth, ongoing, department, institution.' + + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': '' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The institution field cannot be empty.' + + # Tests to make sure ongoing is bool + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'ongoing': '???', + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The field "ongoing" must be boolean.' + + def test_user_put_schools_date_validation(self, app, user_one, url_user_one): + + # Not valid datatypes for dates + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 'string', + 'startMonth': 9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + + # Not valid values for dates + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': -2, + 'startMonth': -9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + + # endDates for ongoing position + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated employment fields can only contain keys degree, ' \ + 'startYear, startMonth, ongoing, department, institution.' + + # End date is greater then start date + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1992, + 'startMonth': 9, + 'endYear': 1991, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' + + def test_user_put_jobs_200(self, app, user_one, url_user_one): + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth) + user_one.reload() + assert res.status_code == 200 + assert user_one.jobs == [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + + def test_user_put_jobs_401(self, app, user_one, url_user_one): + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, expect_errors=True) + assert res.status_code == 401 + assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' + + def test_user_put_jobs_validation(self, app, user_one, url_user_one): + # Tests to make sure schools fields correct structure + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': {} + }, + } + }, auth=user_one.auth, expect_errors=True) + + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' + + # Tests to make sure structure is lists of dicts + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated information must be in a list of dictionaries.' + + # Tests to make sure structure is lists of dicts consisting of proper fields + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{}] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated employment fields must contain keys startYear, title,' \ + ' startMonth, ongoing, department, institution.' + + # Tests to make sure institution is not empty string + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': '' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The institution field cannot be empty.' + + # Tests to make sure ongoing is bool + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'ongoing': '???', + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The field "ongoing" must be boolean.' + + def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): + + # Not valid datatypes for dates + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 'string', + 'startMonth': 9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + + # Not valid values for dates + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': -2, + 'startMonth': -9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + + # endDates for ongoing position + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': True, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'The updated employment fields can only contain keys startYear,' \ + ' title, startMonth, ongoing, department, institution.' + + # End date is greater then start date + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1992, + 'startMonth': 9, + 'endYear': 1991, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' + + def test_put_user_logged_in(self, app, user_one, data_new_user_one, url_user_one): # Logged in user updates their user information via put res = app.put_json_api( url_user_one, From 9c735bb63d0b70c862d3d97ea730d64280fffeeb Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 17 Jul 2018 12:01:42 -0400 Subject: [PATCH 03/20] add json schema validation and fix tests for that --- api/users/education-schema.json | 52 ++++++++++++++++++ api/users/employment-schema.json | 52 ++++++++++++++++++ api/users/schemas.py | 8 +++ api/users/serializers.py | 67 ++++++++--------------- api_tests/users/views/test_user_detail.py | 57 +++++-------------- 5 files changed, 148 insertions(+), 88 deletions(-) create mode 100644 api/users/education-schema.json create mode 100644 api/users/employment-schema.json create mode 100644 api/users/schemas.py diff --git a/api/users/education-schema.json b/api/users/education-schema.json new file mode 100644 index 00000000000..a1a1ddeeed2 --- /dev/null +++ b/api/users/education-schema.json @@ -0,0 +1,52 @@ +{ + "type": "array", + "items": { + "type": "object", + "properties": { + "degree": { + "type": "string" + }, + "startYear": { + "type": "integer", + "minimum": 1900 + }, + "startMonth": { + "type": "integer", + "minimum": 1, + "maximum": 12 + }, + "endMonth": { + "type": "integer", + "minimum": 1, + "maximum": 12 + }, + "endYear": { + "type": "integer", + "minimum": 1900 + }, + "ongoing": { + "type": "boolean" + }, + "department": { + "type": "string" + }, + "institution": { + "type": "string", + "minLength": 1 + } + }, + "required": [ + "startYear", + "startMonth", + "ongoing", + "department", + "institution", + "degree" + ], + "additionalProperties": false, + "dependencies": { + "endMonth": ["endYear"], + "endYear": ["endMonth"] + } + } +} \ No newline at end of file diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json new file mode 100644 index 00000000000..5b47116ed2e --- /dev/null +++ b/api/users/employment-schema.json @@ -0,0 +1,52 @@ +{ + "type": "array", + "items": { + "type": "object", + "properties": { + "title": { + "type": "string" + }, + "startYear": { + "type": "integer", + "minimum": 1900 + }, + "startMonth": { + "type": "integer", + "minimum": 1, + "maximum": 12 + }, + "endMonth": { + "type": "integer", + "minimum": 1, + "maximum": 12 + }, + "endYear": { + "type": "integer", + "minimum": 1900 + }, + "ongoing": { + "type": "boolean" + }, + "department": { + "type": "string" + }, + "institution": { + "type": "string", + "minLength": 1 + } + }, + "required": [ + "startYear", + "startMonth", + "ongoing", + "department", + "institution", + "title" + ], + "additionalProperties": false, + "dependencies": { + "endMonth": ["endYear"], + "endYear": ["endMonth"] + } + } +} \ No newline at end of file diff --git a/api/users/schemas.py b/api/users/schemas.py new file mode 100644 index 00000000000..3e9cb336f86 --- /dev/null +++ b/api/users/schemas.py @@ -0,0 +1,8 @@ +import os +import json + +here = os.path.split(os.path.abspath(__file__))[0] + +def from_json(fname): + with open(os.path.join(here, fname)) as f: + return json.load(f) diff --git a/api/users/serializers.py b/api/users/serializers.py index 7e995cc963a..e5717c30cfa 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -1,5 +1,6 @@ import datetime +import jsonschema from guardian.models import GroupObjectPermission from django.utils import timezone @@ -16,6 +17,7 @@ from api.files.serializers import QuickFilesSerializer from osf.exceptions import ValidationValueError, ValidationError from osf.models import OSFUser, QuickFilesNode +from api.users.schemas import from_json class QuickFilesRelationshipField(RelationshipField): @@ -144,54 +146,31 @@ def _update_social(self, social_dict, user): ) user.social[key] = val[0] - def _validate_user_json(self, info_list, required_fields): - if type(info_list) != list or not info_list: - raise InvalidModelValueError(detail='The updated information must be in a list of dictionaries.') - - for info in info_list: - - if type(info) != dict: - raise InvalidModelValueError(detail='The updated information must be in a list of dictionaries.') - - fields = set(info.keys()) - - absent_fields = required_fields.difference(fields) - if absent_fields: - raise InvalidModelValueError( - detail='The updated employment fields must contain keys {}.'.format(', '.join(absent_fields)) - ) - - if type(info['ongoing']) != bool: - raise InvalidModelValueError(detail='The field "ongoing" must be boolean.') - - if not info['ongoing']: - required_fields = required_fields.union({'endYear', 'endMonth'}) - - extra_fields = required_fields.symmetric_difference(fields) - if extra_fields: - raise InvalidModelValueError( - detail='The updated employment fields can only contain keys {}.'.format(', '.join(required_fields)) - ) + def _validate_user_json(self, value, json_schema): + try: + jsonschema.validate(value, from_json(json_schema)) + except jsonschema.ValidationError as e: + if len(e.path) > 1: + raise InvalidModelValueError("For '{}' the field value {}".format(e.path[-1], e.message)) - if not info.get('institution'): - raise InvalidModelValueError(detail='The institution field cannot be empty.') + raise InvalidModelValueError(e.message) + except jsonschema.SchemaError as e: + raise InvalidModelValueError(e.message) - self._validate_dates(info) + self._validate_dates(value) def _validate_dates(self, info): - try: - startDate = datetime.date(info['startYear'], info['startMonth'], 1) - except (ValueError, TypeError): - raise InvalidModelValueError(detail='Date values must be valid integers.') + for history in info: + + startDate = datetime.date(history['startYear'], history['startMonth'], 1) - if not info['ongoing']: - try: - endDate = datetime.date(info['endYear'], info['endMonth'], 1) - except (ValueError, TypeError): - raise InvalidModelValueError(detail='Date values must be valid integers.') + if not history['ongoing']: + endDate = datetime.date(history['endYear'], history['endMonth'], 1) - if (endDate - startDate).days <= 0: - raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') + if (endDate - startDate).days <= 0: + raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') + elif history.get('endYear') or history.get('endMonth'): + raise InvalidModelValueError(detail='Ongoing positions cannot have end dates.') def update(self, instance, validated_data): assert isinstance(instance, OSFUser), 'instance must be a User' @@ -208,10 +187,10 @@ def update(self, instance, validated_data): ) instance.social[key] = val[0] elif 'schools' == attr: - self._validate_user_json(value, self.education_required_keys) + self._validate_user_json(value, 'education-schema.json') instance.schools = value elif 'jobs' == attr: - self._validate_user_json(value, self.employment_required_keys) + self._validate_user_json(value, 'employment-schema.json') instance.jobs = value elif 'accepted_terms_of_service' == attr: diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 37d2362b656..e4887e69ddc 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -913,20 +913,6 @@ def test_user_put_schools_validation(self, app, user_one, url_user_one): assert res.status_code == 400 assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' - # Tests to make sure structure is lists of dicts - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated information must be in a list of dictionaries.' - # Tests to make sure structure is lists of dicts consisting of proper fields res = app.put_json_api(url_user_one, { 'data': { @@ -938,9 +924,9 @@ def test_user_put_schools_validation(self, app, user_one, url_user_one): }, } }, auth=user_one.auth, expect_errors=True) + print res.json assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated employment fields must contain keys degree, startYear,' \ - ' startMonth, ongoing, department, institution.' + assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { @@ -962,7 +948,7 @@ def test_user_put_schools_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The institution field cannot be empty.' + assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" # Tests to make sure ongoing is bool res = app.put_json_api(url_user_one, { @@ -982,7 +968,7 @@ def test_user_put_schools_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The field "ongoing" must be boolean.' + assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" def test_user_put_schools_date_validation(self, app, user_one, url_user_one): @@ -1004,7 +990,7 @@ def test_user_put_schools_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value u'string' is not of type u'integer'" # Not valid values for dates res = app.put_json_api(url_user_one, { @@ -1024,7 +1010,7 @@ def test_user_put_schools_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value -2 is less than the minimum of 1900" # endDates for ongoing position res = app.put_json_api(url_user_one, { @@ -1046,8 +1032,7 @@ def test_user_put_schools_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated employment fields can only contain keys degree, ' \ - 'startYear, startMonth, ongoing, department, institution.' + assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' # End date is greater then start date res = app.put_json_api(url_user_one, { @@ -1142,20 +1127,6 @@ def test_user_put_jobs_validation(self, app, user_one, url_user_one): assert res.status_code == 400 assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' - # Tests to make sure structure is lists of dicts - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated information must be in a list of dictionaries.' - # Tests to make sure structure is lists of dicts consisting of proper fields res = app.put_json_api(url_user_one, { 'data': { @@ -1168,8 +1139,7 @@ def test_user_put_jobs_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated employment fields must contain keys startYear, title,' \ - ' startMonth, ongoing, department, institution.' + assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { @@ -1191,7 +1161,7 @@ def test_user_put_jobs_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The institution field cannot be empty.' + assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" # Tests to make sure ongoing is bool res = app.put_json_api(url_user_one, { @@ -1211,7 +1181,7 @@ def test_user_put_jobs_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The field "ongoing" must be boolean.' + assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): @@ -1233,7 +1203,7 @@ def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value u'string' is not of type u'integer'" # Not valid values for dates res = app.put_json_api(url_user_one, { @@ -1253,7 +1223,7 @@ def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Date values must be valid integers.' + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value -2 is less than the minimum of 1900" # endDates for ongoing position res = app.put_json_api(url_user_one, { @@ -1275,8 +1245,7 @@ def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): } }, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'The updated employment fields can only contain keys startYear,' \ - ' title, startMonth, ongoing, department, institution.' + assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' # End date is greater then start date res = app.put_json_api(url_user_one, { From 8ae71f9ed65c3757a2f4344c0394d4bbd57a997c Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 17 Jul 2018 16:35:08 -0400 Subject: [PATCH 04/20] remove vestigial code --- api/users/serializers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index e5717c30cfa..82f76dd34bf 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -62,9 +62,7 @@ class UserSerializer(JSONAPISerializer): locale = HideIfDisabled(ser.CharField(required=False, help_text="User's locale, e.g. 'en_US'")) social = ListDictField(required=False) employment = JSONAPIListField(required=False, source='jobs') - employment_required_keys = {'department', 'institution', 'ongoing', 'startMonth', 'startYear', 'title'} education = JSONAPIListField(required=False, source='schools') - education_required_keys = {'department', 'institution', 'ongoing', 'startMonth', 'startYear', 'degree'} can_view_reviews = ShowIfCurrentUser(ser.SerializerMethodField(help_text='Whether the current user has the `view_submissions` permission to ANY reviews provider.')) accepted_terms_of_service = ShowIfCurrentUser(ser.SerializerMethodField()) From 130ff07a355dd780fd8f9391be8ea6887665c485 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 17 Jul 2018 16:36:24 -0400 Subject: [PATCH 05/20] remove _update_social unnecessary method --- api/users/serializers.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index 82f76dd34bf..2ba538e62fe 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -132,18 +132,6 @@ def profile_image_url(self, user): size = self.context['request'].query_params.get('profile_image_size') return user.profile_image_url(size=size) - def _update_social(self, social_dict, user): - for key, val in social_dict.items(): - # currently only profileWebsites are a list, the rest of the social key only has one value - if key == 'profileWebsites': - user.social[key] = val - else: - if len(val) > 1: - raise InvalidModelValueError( - detail='{} only accept a list of one single value'.format(key) - ) - user.social[key] = val[0] - def _validate_user_json(self, value, json_schema): try: jsonschema.validate(value, from_json(json_schema)) From 270702a4e1eab0cee6ff77f9724c3cdcb617e84b Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 17 Jul 2018 16:38:44 -0400 Subject: [PATCH 06/20] fix comments --- api_tests/users/views/test_user_detail.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index e4887e69ddc..20fd0ac1211 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -843,7 +843,6 @@ def test_partial_put_user_logged_in(self, app, user_one, url_user_one): assert user_one.social['github'] == 'even_newer_github' def test_user_put_schools_200(self, app, user_one, url_user_one): - # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { 'data': { 'id': user_one._id, @@ -875,7 +874,6 @@ def test_user_put_schools_200(self, app, user_one, url_user_one): }] def test_user_put_schools_401(self, app, user_one, url_user_one): - # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { 'data': { 'id': user_one._id, @@ -1057,7 +1055,6 @@ def test_user_put_schools_date_validation(self, app, user_one, url_user_one): assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' def test_user_put_jobs_200(self, app, user_one, url_user_one): - # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { 'data': { 'id': user_one._id, @@ -1089,7 +1086,6 @@ def test_user_put_jobs_200(self, app, user_one, url_user_one): }] def test_user_put_jobs_401(self, app, user_one, url_user_one): - # Tests to make sure institution is not empty string res = app.put_json_api(url_user_one, { 'data': { 'id': user_one._id, @@ -1112,7 +1108,7 @@ def test_user_put_jobs_401(self, app, user_one, url_user_one): assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' def test_user_put_jobs_validation(self, app, user_one, url_user_one): - # Tests to make sure schools fields correct structure + # Tests to make sure jobs fields correct structure res = app.put_json_api(url_user_one, { 'data': { 'id': user_one._id, From 3c6e4c3793777d825b17bba66065556c87369744 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 17 Jul 2018 16:43:47 -0400 Subject: [PATCH 07/20] add tests for 403s --- api_tests/users/views/test_user_detail.py | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 20fd0ac1211..ae8300d371b 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -895,6 +895,28 @@ def test_user_put_schools_401(self, app, user_one, url_user_one): assert res.status_code == 401 assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' + def test_user_put_schools_403(self, app, user_two, url_user_one): + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_two._id, + 'type': 'users', + 'attributes': { + 'full_name': user_two.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_two.auth, expect_errors=True) + assert res.status_code == 403 + assert res.json['errors'][0]['detail'] == 'You do not have permission to perform this action.' + def test_user_put_schools_validation(self, app, user_one, url_user_one): # Tests to make sure schools fields correct structure res = app.put_json_api(url_user_one, { @@ -1107,6 +1129,28 @@ def test_user_put_jobs_401(self, app, user_one, url_user_one): assert res.status_code == 401 assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' + def test_user_put_jobs_403(self, app, user_two, url_user_one): + res = app.put_json_api(url_user_one, { + 'data': { + 'id': user_two._id, + 'type': 'users', + 'attributes': { + 'full_name': user_two.fullname, + 'education': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + }, + } + }, auth=user_two.auth, expect_errors=True) + assert res.status_code == 403 + assert res.json['errors'][0]['detail'] == 'You do not have permission to perform this action.' + def test_user_put_jobs_validation(self, app, user_one, url_user_one): # Tests to make sure jobs fields correct structure res = app.put_json_api(url_user_one, { From 82ecae82cf3583011dbea7268601ce9b08570574 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 23 Jul 2018 15:21:15 -0400 Subject: [PATCH 08/20] add new line to json files --- api/users/education-schema.json | 2 +- api/users/employment-schema.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index a1a1ddeeed2..a1deb2d30b1 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -49,4 +49,4 @@ "endYear": ["endMonth"] } } -} \ No newline at end of file +} diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index 5b47116ed2e..4a5cb9b3726 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -49,4 +49,4 @@ "endYear": ["endMonth"] } } -} \ No newline at end of file +} From a37c058392e52f3b5b80c1a326aaf6f695debcff Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 23 Jul 2018 15:22:08 -0400 Subject: [PATCH 09/20] use Django builtin validation methods --- api/users/serializers.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index 2ba538e62fe..9de9037e1a5 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -158,6 +158,14 @@ def _validate_dates(self, info): elif history.get('endYear') or history.get('endMonth'): raise InvalidModelValueError(detail='Ongoing positions cannot have end dates.') + def validate_employment(self, value): + self._validate_user_json(value, 'employment-schema.json') + return value + + def validate_education(self, value): + self._validate_user_json(value, 'education-schema.json') + return value + def update(self, instance, validated_data): assert isinstance(instance, OSFUser), 'instance must be a User' for attr, value in validated_data.items(): @@ -173,12 +181,9 @@ def update(self, instance, validated_data): ) instance.social[key] = val[0] elif 'schools' == attr: - self._validate_user_json(value, 'education-schema.json') instance.schools = value elif 'jobs' == attr: - self._validate_user_json(value, 'employment-schema.json') instance.jobs = value - elif 'accepted_terms_of_service' == attr: if value and not instance.accepted_terms_of_service: instance.accepted_terms_of_service = timezone.now() From 57dabbffc25a140445debbe143e003a7c27402a4 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 23 Jul 2018 15:22:41 -0400 Subject: [PATCH 10/20] DRY up tests --- api_tests/users/views/test_user_detail.py | 631 ++++++---------------- 1 file changed, 164 insertions(+), 467 deletions(-) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index ae8300d371b..2958cfd27ec 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -842,473 +842,6 @@ def test_partial_put_user_logged_in(self, app, user_one, url_user_one): assert user_one.suffix == 'The Millionth' assert user_one.social['github'] == 'even_newer_github' - def test_user_put_schools_200(self, app, user_one, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth) - user_one.reload() - assert res.status_code == 200 - assert user_one.schools == [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - - def test_user_put_schools_401(self, app, user_one, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, expect_errors=True) - assert res.status_code == 401 - assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' - - def test_user_put_schools_403(self, app, user_two, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_two._id, - 'type': 'users', - 'attributes': { - 'full_name': user_two.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_two.auth, expect_errors=True) - assert res.status_code == 403 - assert res.json['errors'][0]['detail'] == 'You do not have permission to perform this action.' - - def test_user_put_schools_validation(self, app, user_one, url_user_one): - # Tests to make sure schools fields correct structure - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': {} - }, - } - }, auth=user_one.auth, expect_errors=True) - - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' - - # Tests to make sure structure is lists of dicts consisting of proper fields - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{}] - }, - } - }, auth=user_one.auth, expect_errors=True) - print res.json - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" - - # Tests to make sure institution is not empty string - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': '' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" - - # Tests to make sure ongoing is bool - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'ongoing': '???', - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" - - def test_user_put_schools_date_validation(self, app, user_one, url_user_one): - - # Not valid datatypes for dates - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 'string', - 'startMonth': 9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'startYear' the field value u'string' is not of type u'integer'" - - # Not valid values for dates - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': -2, - 'startMonth': -9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'startYear' the field value -2 is less than the minimum of 1900" - - # endDates for ongoing position - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' - - # End date is greater then start date - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'education': [{'degree': '', - 'startYear': 1992, - 'startMonth': 9, - 'endYear': 1991, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' - - def test_user_put_jobs_200(self, app, user_one, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth) - user_one.reload() - assert res.status_code == 200 - assert user_one.jobs == [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - - def test_user_put_jobs_401(self, app, user_one, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'degree': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, expect_errors=True) - assert res.status_code == 401 - assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' - - def test_user_put_jobs_403(self, app, user_two, url_user_one): - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_two._id, - 'type': 'users', - 'attributes': { - 'full_name': user_two.fullname, - 'education': [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_two.auth, expect_errors=True) - assert res.status_code == 403 - assert res.json['errors'][0]['detail'] == 'You do not have permission to perform this action.' - - def test_user_put_jobs_validation(self, app, user_one, url_user_one): - # Tests to make sure jobs fields correct structure - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': {} - }, - } - }, auth=user_one.auth, expect_errors=True) - - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' - - # Tests to make sure structure is lists of dicts consisting of proper fields - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{}] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" - - # Tests to make sure institution is not empty string - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': '' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" - - # Tests to make sure ongoing is bool - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'ongoing': '???', - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" - - def test_user_put_jobs_date_validation(self, app, user_one, url_user_one): - - # Not valid datatypes for dates - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 'string', - 'startMonth': 9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'startYear' the field value u'string' is not of type u'integer'" - - # Not valid values for dates - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': -2, - 'startMonth': -9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'startYear' the field value -2 is less than the minimum of 1900" - - # endDates for ongoing position - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 1991, - 'startMonth': 9, - 'endYear': 1992, - 'endMonth': 9, - 'ongoing': True, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' - - # End date is greater then start date - res = app.put_json_api(url_user_one, { - 'data': { - 'id': user_one._id, - 'type': 'users', - 'attributes': { - 'full_name': user_one.fullname, - 'employment': [{'title': '', - 'startYear': 1992, - 'startMonth': 9, - 'endYear': 1991, - 'endMonth': 9, - 'ongoing': False, - 'department': '', - 'institution': 'Fake U' - }] - }, - } - }, auth=user_one.auth, expect_errors=True) - assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' - def test_put_user_logged_in(self, app, user_one, data_new_user_one, url_user_one): # Logged in user updates their user information via put res = app.put_json_api( @@ -1456,3 +989,167 @@ def test_requesting_deactivated_user_returns_410_response_and_meta_info( assert urlparse.urlparse( res.json['errors'][0]['meta']['profile_image']).netloc == 'secure.gravatar.com' assert res.json['errors'][0]['detail'] == 'The requested user is no longer available.' + + +@pytest.mark.django_db +class UserProfileMixin(object): + + @pytest.fixture() + def request_payload(self): + raise NotImplementedError + + @pytest.fixture() + def request_key(self): + raise NotImplementedError + + @pytest.fixture() + def user_one(self): + return AuthUserFactory() + + @pytest.fixture() + def user_two(self): + return AuthUserFactory() + + @pytest.fixture() + def user_one_url(self, user_one): + return '/v2/users/{}/'.format(user_one._id) + + def test_user_put_profile_200(self, app, user_one, user_one_url, request_payload, request_key, user_attr): + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth) + user_one.reload() + assert res.status_code == 200 + assert getattr(user_one, user_attr) == request_payload['data']['attributes'][request_key] + + def test_user_put_profile_401(self, app, user_one, user_one_url, request_payload): + res = app.put_json_api(user_one_url, request_payload, expect_errors=True) + assert res.status_code == 401 + assert res.json['errors'][0]['detail'] == 'Authentication credentials were not provided.' + + def test_user_put_profile_403(self, app, user_two, user_one_url, request_payload): + res = app.put_json_api(user_one_url, request_payload, auth=user_two.auth, expect_errors=True) + assert res.status_code == 403 + assert res.json['errors'][0]['detail'] == 'You do not have permission to perform this action.' + + def test_user_put_profile_validate_dict(self, app, user_one, user_one_url, request_payload, request_key): + # Tests to make sure profile's fields have correct structure + request_payload['data']['attributes'][request_key] = {} + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Expected a list of items but got type "dict".' + + def test_user_put_profile_validation_list(self, app, user_one, user_one_url, request_payload, request_key): + # Tests to make sure structure is lists of dicts consisting of proper fields + request_payload['data']['attributes'][request_key] = [{}] + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" + + def test_user_put_profile_validation_empty_string(self, app, user_one, user_one_url, request_payload, request_key): + # Tests to make sure institution is not empty string + request_payload['data']['attributes'][request_key][0]['institution'] = '' + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" + + def test_user_put_profile_validation_ongoing_bool(self, app, user_one, user_one_url, request_payload, request_key): + # Tests to make sure ongoing is bool + request_payload['data']['attributes'][request_key][0]['ongoing'] = '???' + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" + + def test_user_put_profile_date_validate_int(self, app, user_one, user_one_url, request_payload, request_key): + # Not valid datatypes for dates + + request_payload['data']['attributes'][request_key][0]['startYear'] = 'string' + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + print res.json['errors'][0] + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value u'string' is not of type u'integer'" + + def test_user_put_profile_date_validate_positive(self, app, user_one, user_one_url, request_payload, request_key): + # Not valid values for dates + request_payload['data']['attributes'][request_key][0]['startYear'] = -2 + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "For 'startYear' the field value -2 is less than the minimum of 1900" + + def test_user_put_profile_date_validate_ongoing_position(self, app, user_one, user_one_url, request_payload, request_key): + # endDates for ongoing position + request_payload['data']['attributes'][request_key][0]['ongoing'] = True + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' + + def test_user_put_profile_date_validate_end_date(self, app, user_one, user_one_url, request_payload, request_key): + # End date is greater then start date + request_payload['data']['attributes'][request_key][0]['startYear'] = 2000 + res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' + + +@pytest.mark.django_db +class TestUserSchools(UserProfileMixin): + + @pytest.fixture() + def request_payload(self, user_one): + return { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'education': [{'degree': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + } + } + } + + @pytest.fixture() + def request_key(self): + return 'education' + + @pytest.fixture() + def user_attr(self): + return 'schools' + + +@pytest.mark.django_db +class TestUserJobs(UserProfileMixin): + + @pytest.fixture() + def request_payload(self, user_one): + return { + 'data': { + 'id': user_one._id, + 'type': 'users', + 'attributes': { + 'full_name': user_one.fullname, + 'employment': [{'title': '', + 'startYear': 1991, + 'startMonth': 9, + 'endYear': 1992, + 'endMonth': 9, + 'ongoing': False, + 'department': '', + 'institution': 'Fake U' + }] + } + } + } + + @pytest.fixture() + def request_key(self): + return 'employment' + + @pytest.fixture() + def user_attr(self): + return 'jobs' From d7338f3bbfe9b6fee631cab51150546e03e2fa33 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 23 Jul 2018 17:39:28 -0400 Subject: [PATCH 11/20] make start year optional --- api/users/education-schema.json | 6 +++--- api/users/employment-schema.json | 6 +++--- api_tests/users/views/test_user_detail.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index a1deb2d30b1..595ca84099f 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -36,8 +36,6 @@ } }, "required": [ - "startYear", - "startMonth", "ongoing", "department", "institution", @@ -46,7 +44,9 @@ "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "endYear": ["endMonth"] + "endYear": ["endMonth"], + "startMonth": ["startYear"], + "startYear": ["startMonth"] } } } diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index 4a5cb9b3726..8b68327421c 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -36,8 +36,6 @@ } }, "required": [ - "startYear", - "startMonth", "ongoing", "department", "institution", @@ -46,7 +44,9 @@ "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "endYear": ["endMonth"] + "endYear": ["endMonth"], + "startMonth": ["startYear"], + "startYear": ["startMonth"] } } } diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 2958cfd27ec..c46ab5f5f1f 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1042,7 +1042,7 @@ def test_user_put_profile_validation_list(self, app, user_one, user_one_url, req request_payload['data']['attributes'][request_key] = [{}] res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'startYear' is a required property" + assert res.json['errors'][0]['detail'] == "u'ongoing' is a required property" def test_user_put_profile_validation_empty_string(self, app, user_one, user_one_url, request_payload, request_key): # Tests to make sure institution is not empty string From 699ec810c859386d8895f6d7fca4c03da1ed1436 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 23 Jul 2018 17:39:57 -0400 Subject: [PATCH 12/20] remove extraneous code --- api/users/serializers.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index 9de9037e1a5..e23c83a4cb5 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -180,10 +180,6 @@ def update(self, instance, validated_data): detail='{} only accept a list of one single value'. format(key) ) instance.social[key] = val[0] - elif 'schools' == attr: - instance.schools = value - elif 'jobs' == attr: - instance.jobs = value elif 'accepted_terms_of_service' == attr: if value and not instance.accepted_terms_of_service: instance.accepted_terms_of_service = timezone.now() From 39e0c637d345b615c98940fadffcddd979bb5bdf Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 24 Jul 2018 17:37:10 -0400 Subject: [PATCH 13/20] unbreak tests after Travis speed up --- api_tests/users/views/test_user_detail.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 6512e35957c..8bd46a821cc 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -997,6 +997,7 @@ def test_requesting_deactivated_user_returns_410_response_and_meta_info( @pytest.mark.django_db +@pytest.mark.enable_quickfiles_creation class UserProfileMixin(object): @pytest.fixture() From 8fd6d669b03006d08b7c512bf59f942a7012d7f1 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Wed, 25 Jul 2018 17:43:09 -0400 Subject: [PATCH 14/20] update schemas and add more tests Signed-off-by: John Tordoff --- api/users/education-schema.json | 9 +--- api/users/employment-schema.json | 9 +--- api/users/serializers.py | 11 +++-- api_tests/users/views/test_user_detail.py | 60 ++++++++++++++++++++++- 4 files changed, 70 insertions(+), 19 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index 595ca84099f..e5361afebc0 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -36,17 +36,12 @@ } }, "required": [ - "ongoing", - "department", - "institution", - "degree" + "institution" ], "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "endYear": ["endMonth"], - "startMonth": ["startYear"], - "startYear": ["startMonth"] + "startMonth": ["startYear"] } } } diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index 8b68327421c..b5398309821 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -36,17 +36,12 @@ } }, "required": [ - "ongoing", - "department", - "institution", - "title" + "institution" ], "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "endYear": ["endMonth"], - "startMonth": ["startYear"], - "startYear": ["startMonth"] + "startMonth": ["startYear"] } } } diff --git a/api/users/serializers.py b/api/users/serializers.py index e23c83a4cb5..8abf4f82ede 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -148,13 +148,16 @@ def _validate_user_json(self, value, json_schema): def _validate_dates(self, info): for history in info: - startDate = datetime.date(history['startYear'], history['startMonth'], 1) + if history.get('startYear'): + startDate = datetime.date(history['startYear'], history['startMonth'], 1) if not history['ongoing']: - endDate = datetime.date(history['endYear'], history['endMonth'], 1) + if history.get('endYear'): + endDate = datetime.date(history['endYear'], history.get('endMonth', 1), 1) - if (endDate - startDate).days <= 0: - raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') + if history.get('startYear') and history.get('endYear'): + if (endDate - startDate).days <= 0: + raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') elif history.get('endYear') or history.get('endMonth'): raise InvalidModelValueError(detail='Ongoing positions cannot have end dates.') diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 8bd46a821cc..89298fd2ac9 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1004,6 +1004,33 @@ class UserProfileMixin(object): def request_payload(self): raise NotImplementedError + @pytest.fixture() + def bad_request_payload(self, request_payload, request_key): + request_payload['data']['attributes'][request_key][0]['bad_key'] = 'bad_value' + return request_payload + + @pytest.fixture() + def end_dates_no_start_dates_payload(self, request_payload, request_key): + del request_payload['data']['attributes'][request_key][0]['startYear'] + del request_payload['data']['attributes'][request_key][0]['startMonth'] + return request_payload + + @pytest.fixture() + def start_dates_no_end_dates_payload(self, request_payload, request_key): + del request_payload['data']['attributes'][request_key][0]['endYear'] + del request_payload['data']['attributes'][request_key][0]['endMonth'] + return request_payload + + @pytest.fixture() + def end_month_dependency_payload(self, request_payload, request_key): + del request_payload['data']['attributes'][request_key][0]['endYear'] + return request_payload + + @pytest.fixture() + def start_month_dependency_payload(self, request_payload, request_key): + del request_payload['data']['attributes'][request_key][0]['startYear'] + return request_payload + @pytest.fixture() def request_key(self): raise NotImplementedError @@ -1026,6 +1053,11 @@ def test_user_put_profile_200(self, app, user_one, user_one_url, request_payload assert res.status_code == 200 assert getattr(user_one, user_attr) == request_payload['data']['attributes'][request_key] + def test_user_put_profile_400(self, app, user_one, user_one_url, bad_request_payload): + res = app.put_json_api(user_one_url, bad_request_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "Additional properties are not allowed (u'bad_key' was unexpected)" + def test_user_put_profile_401(self, app, user_one, user_one_url, request_payload): res = app.put_json_api(user_one_url, request_payload, expect_errors=True) assert res.status_code == 401 @@ -1048,7 +1080,7 @@ def test_user_put_profile_validation_list(self, app, user_one, user_one_url, req request_payload['data']['attributes'][request_key] = [{}] res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'ongoing' is a required property" + assert res.json['errors'][0]['detail'] == "u'institution' is a required property" def test_user_put_profile_validation_empty_string(self, app, user_one, user_one_url, request_payload, request_key): # Tests to make sure institution is not empty string @@ -1094,6 +1126,32 @@ def test_user_put_profile_date_validate_end_date(self, app, user_one, user_one_u assert res.status_code == 400 assert res.json['errors'][0]['detail'] == 'End date must be greater than or equal to the start date.' + def test_user_put_profile_date_validate_end_month_dependency(self, app, user_one, user_one_url, end_month_dependency_payload): + # No endMonth with endYear + res = app.put_json_api(user_one_url, end_month_dependency_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "u'endYear' is a dependency of u'endMonth'" + + def test_user_put_profile_date_validate_start_month_dependency(self, app, user_one, user_one_url, start_month_dependency_payload): + # No endMonth with endYear + res = app.put_json_api(user_one_url, start_month_dependency_payload, auth=user_one.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "u'startYear' is a dependency of u'startMonth'" + + def test_user_put_profile_date_validate_start_date_no_end_date(self, app, user_one, user_attr, user_one_url, start_dates_no_end_dates_payload, request_key): + # End date is greater then start date + res = app.put_json_api(user_one_url, start_dates_no_end_dates_payload, auth=user_one.auth, expect_errors=True) + user_one.reload() + assert res.status_code == 200 + assert getattr(user_one, user_attr) == start_dates_no_end_dates_payload['data']['attributes'][request_key] + + def test_user_put_profile_date_validate_end_date_no_start_date(self, app, user_one, user_attr, user_one_url, end_dates_no_start_dates_payload, request_key): + # End dates, but no start dates + res = app.put_json_api(user_one_url, end_dates_no_start_dates_payload, auth=user_one.auth, expect_errors=True) + user_one.reload() + assert res.status_code == 200 + assert getattr(user_one, user_attr) == end_dates_no_start_dates_payload['data']['attributes'][request_key] + @pytest.mark.django_db class TestUserSchools(UserProfileMixin): From 344f50acdf8b5147aa0a9776da62938628e2ea6a Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Thu, 26 Jul 2018 14:51:59 -0400 Subject: [PATCH 15/20] handle startYear with no startMonth --- api/users/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/users/serializers.py b/api/users/serializers.py index 8abf4f82ede..9b38c124a9b 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -149,7 +149,7 @@ def _validate_dates(self, info): for history in info: if history.get('startYear'): - startDate = datetime.date(history['startYear'], history['startMonth'], 1) + startDate = datetime.date(history['startYear'], history.get('startMonth', 1), 1) if not history['ongoing']: if history.get('endYear'): From 811b7b74d7265e9f1141ea2a90907bf0f472e009 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Thu, 26 Jul 2018 17:10:44 -0400 Subject: [PATCH 16/20] add jsonschema validation for ongoing attribute. --- api/users/education-schema.json | 31 +++++++++++++++++++++++ api/users/employment-schema.json | 31 +++++++++++++++++++++++ api/users/serializers.py | 5 ++-- api_tests/users/views/test_user_detail.py | 10 +++++--- 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index e5361afebc0..45fb6e84ae5 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -42,6 +42,37 @@ "dependencies": { "endMonth": ["endYear"], "startMonth": ["startYear"] + }, + "anyOf": [ + { + "not": { + "properties": { + "ongoing": { + "enum": [true] + } + }, + "required": ["ongoing"] + } + }, + { "required": ["startYear"]} + ], + "anyOf": [ + { + "not": { + "properties": { + "ongoing": { "enum": [false] } + }, + "required": ["ongoing"] + } + }, + { "required": ["endYear"] } + ], + "not": { + "properties": { + "ongoing": {"enum": [true]} + }, + "required": ["endYear"], + "message": "Ongoing positions cannot have end dates." } } } diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index b5398309821..b591298e452 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -42,6 +42,37 @@ "dependencies": { "endMonth": ["endYear"], "startMonth": ["startYear"] + }, + "anyOf": [ + { + "not": { + "properties": { + "ongoing": { + "enum": [true] + } + }, + "required": ["ongoing"] + } + }, + { "required": ["startYear"]} + ], + "anyOf": [ + { + "not": { + "properties": { + "ongoing": { "enum": [false] } + }, + "required": ["ongoing"] + } + }, + { "required": ["endYear"] } + ], + "not": { + "properties": { + "ongoing": {"enum": [true]} + }, + "required": ["endYear"], + "message": "Ongoing positions cannot have end dates." } } } diff --git a/api/users/serializers.py b/api/users/serializers.py index 9b38c124a9b..588ea9e3f53 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -136,9 +136,10 @@ def _validate_user_json(self, value, json_schema): try: jsonschema.validate(value, from_json(json_schema)) except jsonschema.ValidationError as e: + if type(e.validator_value) == dict and e.validator_value.get('message'): + raise InvalidModelValueError(e.validator_value.get('message')) if len(e.path) > 1: raise InvalidModelValueError("For '{}' the field value {}".format(e.path[-1], e.message)) - raise InvalidModelValueError(e.message) except jsonschema.SchemaError as e: raise InvalidModelValueError(e.message) @@ -158,8 +159,6 @@ def _validate_dates(self, info): if history.get('startYear') and history.get('endYear'): if (endDate - startDate).days <= 0: raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') - elif history.get('endYear') or history.get('endMonth'): - raise InvalidModelValueError(detail='Ongoing positions cannot have end dates.') def validate_employment(self, value): self._validate_user_json(value, 'employment-schema.json') diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 89298fd2ac9..0b4d87b0117 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1138,16 +1138,18 @@ def test_user_put_profile_date_validate_start_month_dependency(self, app, user_o assert res.status_code == 400 assert res.json['errors'][0]['detail'] == "u'startYear' is a dependency of u'startMonth'" - def test_user_put_profile_date_validate_start_date_no_end_date(self, app, user_one, user_attr, user_one_url, start_dates_no_end_dates_payload, request_key): + def test_user_put_profile_date_validate_start_date_no_end_date_not_ongoing(self, app, user_one, user_attr, user_one_url, start_dates_no_end_dates_payload, request_key): # End date is greater then start date res = app.put_json_api(user_one_url, start_dates_no_end_dates_payload, auth=user_one.auth, expect_errors=True) user_one.reload() - assert res.status_code == 200 - assert getattr(user_one, user_attr) == start_dates_no_end_dates_payload['data']['attributes'][request_key] + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "{{u'startYear': 1991, u'{}': u'', u'startMonth': 9, u'ongoing':" \ + " False, u'department': u'', u'institution': u'Fake U'}}" \ + ' is not valid under any of the given schemas'.format('title' if user_attr == 'jobs' else 'degree') def test_user_put_profile_date_validate_end_date_no_start_date(self, app, user_one, user_attr, user_one_url, end_dates_no_start_dates_payload, request_key): # End dates, but no start dates - res = app.put_json_api(user_one_url, end_dates_no_start_dates_payload, auth=user_one.auth, expect_errors=True) + res = app.put_json_api(user_one_url, end_dates_no_start_dates_payload, auth=user_one.auth) user_one.reload() assert res.status_code == 200 assert getattr(user_one, user_attr) == end_dates_no_start_dates_payload['data']['attributes'][request_key] From b7d9324a88c4ffba58c215340f7f04fbadae7e9e Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Fri, 27 Jul 2018 13:07:29 -0400 Subject: [PATCH 17/20] make the employment/education schema lock tight and remove some custom error messages --- api/users/education-schema.json | 49 ++++++++--------------- api/users/employment-schema.json | 49 ++++++++--------------- api/users/serializers.py | 2 - api_tests/users/views/test_user_detail.py | 14 +++---- 4 files changed, 39 insertions(+), 75 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index 45fb6e84ae5..48273327e43 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -25,6 +25,18 @@ "minimum": 1900 }, "ongoing": { + "oneOf": [{ + "enum": [false], + "required": ["startYear", "endYear"] + }, + { + "enum": [true], + "required": ["startYear"], + "not": { + "required": ["endYear"] + } + } + ], "type": "boolean" }, "department": { @@ -41,38 +53,9 @@ "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "startMonth": ["startYear"] - }, - "anyOf": [ - { - "not": { - "properties": { - "ongoing": { - "enum": [true] - } - }, - "required": ["ongoing"] - } - }, - { "required": ["startYear"]} - ], - "anyOf": [ - { - "not": { - "properties": { - "ongoing": { "enum": [false] } - }, - "required": ["ongoing"] - } - }, - { "required": ["endYear"] } - ], - "not": { - "properties": { - "ongoing": {"enum": [true]} - }, - "required": ["endYear"], - "message": "Ongoing positions cannot have end dates." + "startMonth": ["startYear"], + "startYear": ["ongoing"], + "endYear": ["ongoing"] } } -} +} \ No newline at end of file diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index b591298e452..f7e294a2f54 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -25,6 +25,18 @@ "minimum": 1900 }, "ongoing": { + "oneOf": [{ + "enum": [false], + "required": ["startYear", "endYear"] + }, + { + "enum": [true], + "required": ["startYear"], + "not": { + "required": ["endYear"] + } + } + ], "type": "boolean" }, "department": { @@ -41,38 +53,9 @@ "additionalProperties": false, "dependencies": { "endMonth": ["endYear"], - "startMonth": ["startYear"] - }, - "anyOf": [ - { - "not": { - "properties": { - "ongoing": { - "enum": [true] - } - }, - "required": ["ongoing"] - } - }, - { "required": ["startYear"]} - ], - "anyOf": [ - { - "not": { - "properties": { - "ongoing": { "enum": [false] } - }, - "required": ["ongoing"] - } - }, - { "required": ["endYear"] } - ], - "not": { - "properties": { - "ongoing": {"enum": [true]} - }, - "required": ["endYear"], - "message": "Ongoing positions cannot have end dates." + "startMonth": ["startYear"], + "startYear": ["ongoing"], + "endYear": ["ongoing"] } } -} +} \ No newline at end of file diff --git a/api/users/serializers.py b/api/users/serializers.py index 588ea9e3f53..ce93278e881 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -136,8 +136,6 @@ def _validate_user_json(self, value, json_schema): try: jsonschema.validate(value, from_json(json_schema)) except jsonschema.ValidationError as e: - if type(e.validator_value) == dict and e.validator_value.get('message'): - raise InvalidModelValueError(e.validator_value.get('message')) if len(e.path) > 1: raise InvalidModelValueError("For '{}' the field value {}".format(e.path[-1], e.message)) raise InvalidModelValueError(e.message) diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 0b4d87b0117..2f1bfa5be2a 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1017,6 +1017,7 @@ def end_dates_no_start_dates_payload(self, request_payload, request_key): @pytest.fixture() def start_dates_no_end_dates_payload(self, request_payload, request_key): + request_payload['data']['attributes'][request_key][0]['ongoing'] = True del request_payload['data']['attributes'][request_key][0]['endYear'] del request_payload['data']['attributes'][request_key][0]['endMonth'] return request_payload @@ -1089,12 +1090,12 @@ def test_user_put_profile_validation_empty_string(self, app, user_one, user_one_ assert res.status_code == 400 assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" - def test_user_put_profile_validation_ongoing_bool(self, app, user_one, user_one_url, request_payload, request_key): + def test_user_put_profile_validation_ongoing_dependency(self, app, user_one, user_one_url, request_payload, request_key): # Tests to make sure ongoing is bool - request_payload['data']['attributes'][request_key][0]['ongoing'] = '???' + del request_payload['data']['attributes'][request_key][0]['ongoing'] res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value u'???' is not of type u'boolean'" + assert res.json['errors'][0]['detail'] == "u'ongoing' is a dependency of u'endYear'" def test_user_put_profile_date_validate_int(self, app, user_one, user_one_url, request_payload, request_key): # Not valid datatypes for dates @@ -1115,9 +1116,10 @@ def test_user_put_profile_date_validate_positive(self, app, user_one, user_one_u def test_user_put_profile_date_validate_ongoing_position(self, app, user_one, user_one_url, request_payload, request_key): # endDates for ongoing position request_payload['data']['attributes'][request_key][0]['ongoing'] = True + del request_payload['data']['attributes'][request_key][0]['endYear'] res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == 'Ongoing positions cannot have end dates.' + assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value True is not valid under any of the given schemas" def test_user_put_profile_date_validate_end_date(self, app, user_one, user_one_url, request_payload, request_key): # End date is greater then start date @@ -1143,9 +1145,7 @@ def test_user_put_profile_date_validate_start_date_no_end_date_not_ongoing(self, res = app.put_json_api(user_one_url, start_dates_no_end_dates_payload, auth=user_one.auth, expect_errors=True) user_one.reload() assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "{{u'startYear': 1991, u'{}': u'', u'startMonth': 9, u'ongoing':" \ - " False, u'department': u'', u'institution': u'Fake U'}}" \ - ' is not valid under any of the given schemas'.format('title' if user_attr == 'jobs' else 'degree') + assert res.json['errors'][0]['detail'] == "For 'ongoing' the field value True is not valid under any of the given schemas" def test_user_put_profile_date_validate_end_date_no_start_date(self, app, user_one, user_attr, user_one_url, end_dates_no_start_dates_payload, request_key): # End dates, but no start dates From 72d4a4501319b4af75cae1d7fbe79fc88c5bc8b9 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Mon, 30 Jul 2018 15:10:02 -0400 Subject: [PATCH 18/20] add startYear <- endYear dependency --- api/users/education-schema.json | 3 ++- api/users/employment-schema.json | 3 ++- api_tests/users/views/test_user_detail.py | 12 ++++++------ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/api/users/education-schema.json b/api/users/education-schema.json index 48273327e43..3dda2a3481f 100644 --- a/api/users/education-schema.json +++ b/api/users/education-schema.json @@ -55,7 +55,8 @@ "endMonth": ["endYear"], "startMonth": ["startYear"], "startYear": ["ongoing"], - "endYear": ["ongoing"] + "endYear": ["ongoing"], + "endYear": ["startYear"] } } } \ No newline at end of file diff --git a/api/users/employment-schema.json b/api/users/employment-schema.json index f7e294a2f54..f2e77fb5096 100644 --- a/api/users/employment-schema.json +++ b/api/users/employment-schema.json @@ -55,7 +55,8 @@ "endMonth": ["endYear"], "startMonth": ["startYear"], "startYear": ["ongoing"], - "endYear": ["ongoing"] + "endYear": ["ongoing"], + "endYear": ["startYear"] } } } \ No newline at end of file diff --git a/api_tests/users/views/test_user_detail.py b/api_tests/users/views/test_user_detail.py index 2f1bfa5be2a..c838e356f22 100644 --- a/api_tests/users/views/test_user_detail.py +++ b/api_tests/users/views/test_user_detail.py @@ -1090,12 +1090,12 @@ def test_user_put_profile_validation_empty_string(self, app, user_one, user_one_ assert res.status_code == 400 assert res.json['errors'][0]['detail'] == "For 'institution' the field value u'' is too short" - def test_user_put_profile_validation_ongoing_dependency(self, app, user_one, user_one_url, request_payload, request_key): + def test_user_put_profile_validation_start_year_dependency(self, app, user_one, user_one_url, request_payload, request_key): # Tests to make sure ongoing is bool del request_payload['data']['attributes'][request_key][0]['ongoing'] res = app.put_json_api(user_one_url, request_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'ongoing' is a dependency of u'endYear'" + assert res.json['errors'][0]['detail'] == "u'ongoing' is a dependency of u'startYear'" def test_user_put_profile_date_validate_int(self, app, user_one, user_one_url, request_payload, request_key): # Not valid datatypes for dates @@ -1138,7 +1138,7 @@ def test_user_put_profile_date_validate_start_month_dependency(self, app, user_o # No endMonth with endYear res = app.put_json_api(user_one_url, start_month_dependency_payload, auth=user_one.auth, expect_errors=True) assert res.status_code == 400 - assert res.json['errors'][0]['detail'] == "u'startYear' is a dependency of u'startMonth'" + assert res.json['errors'][0]['detail'] == "u'startYear' is a dependency of u'endYear'" def test_user_put_profile_date_validate_start_date_no_end_date_not_ongoing(self, app, user_one, user_attr, user_one_url, start_dates_no_end_dates_payload, request_key): # End date is greater then start date @@ -1149,10 +1149,10 @@ def test_user_put_profile_date_validate_start_date_no_end_date_not_ongoing(self, def test_user_put_profile_date_validate_end_date_no_start_date(self, app, user_one, user_attr, user_one_url, end_dates_no_start_dates_payload, request_key): # End dates, but no start dates - res = app.put_json_api(user_one_url, end_dates_no_start_dates_payload, auth=user_one.auth) + res = app.put_json_api(user_one_url, end_dates_no_start_dates_payload, auth=user_one.auth, expect_errors=True) user_one.reload() - assert res.status_code == 200 - assert getattr(user_one, user_attr) == end_dates_no_start_dates_payload['data']['attributes'][request_key] + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == "u'startYear' is a dependency of u'endYear'" @pytest.mark.django_db From 53ab14747133685d86a5902fdb876ede8f88e020 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 31 Jul 2018 15:24:51 -0400 Subject: [PATCH 19/20] check for more string types --- osf/models/validators.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osf/models/validators.py b/osf/models/validators.py index 25cbb2349f5..a37a7bcacd1 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -4,6 +4,7 @@ from django.core.validators import URLValidator, validate_email as django_validate_email from django.core.exceptions import ValidationError as DjangoValidationError from django.utils.deconstruct import deconstructible +from django.utils.six import string_types from website.notifications.constants import NOTIFICATION_TYPES from website import settings @@ -38,7 +39,7 @@ def validate_year(item): except ValueError: raise ValidationValueError('Please enter a valid year.') else: - if type(item) == str and len(item) != 4: + if isinstance(item, string_types) and len(item) != 4: raise ValidationValueError('Please enter a valid year.') From 633aa4615219108790b4e78b44e438b426410e27 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 31 Jul 2018 15:25:58 -0400 Subject: [PATCH 20/20] move schemas and make utility functions. --- api/users/schemas.py | 8 ---- api/users/schemas/__init__.py | 0 api/users/{ => schemas}/education-schema.json | 0 .../{ => schemas}/employment-schema.json | 0 api/users/schemas/utils.py | 40 +++++++++++++++++++ api/users/serializers.py | 35 ++-------------- 6 files changed, 43 insertions(+), 40 deletions(-) delete mode 100644 api/users/schemas.py create mode 100644 api/users/schemas/__init__.py rename api/users/{ => schemas}/education-schema.json (100%) rename api/users/{ => schemas}/employment-schema.json (100%) create mode 100644 api/users/schemas/utils.py diff --git a/api/users/schemas.py b/api/users/schemas.py deleted file mode 100644 index 3e9cb336f86..00000000000 --- a/api/users/schemas.py +++ /dev/null @@ -1,8 +0,0 @@ -import os -import json - -here = os.path.split(os.path.abspath(__file__))[0] - -def from_json(fname): - with open(os.path.join(here, fname)) as f: - return json.load(f) diff --git a/api/users/schemas/__init__.py b/api/users/schemas/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/api/users/education-schema.json b/api/users/schemas/education-schema.json similarity index 100% rename from api/users/education-schema.json rename to api/users/schemas/education-schema.json diff --git a/api/users/employment-schema.json b/api/users/schemas/employment-schema.json similarity index 100% rename from api/users/employment-schema.json rename to api/users/schemas/employment-schema.json diff --git a/api/users/schemas/utils.py b/api/users/schemas/utils.py new file mode 100644 index 00000000000..79fcc466df0 --- /dev/null +++ b/api/users/schemas/utils.py @@ -0,0 +1,40 @@ +import os +import json +import datetime +import jsonschema + +from api.base.exceptions import InvalidModelValueError + +here = os.path.split(os.path.abspath(__file__))[0] + +def from_json(fname): + with open(os.path.join(here, fname)) as f: + return json.load(f) + + +def validate_user_json(value, json_schema): + try: + jsonschema.validate(value, from_json(json_schema)) + except jsonschema.ValidationError as e: + if len(e.path) > 1: + raise InvalidModelValueError("For '{}' the field value {}".format(e.path[-1], e.message)) + raise InvalidModelValueError(e.message) + except jsonschema.SchemaError as e: + raise InvalidModelValueError(e.message) + + validate_dates(value) + + +def validate_dates(info): + for history in info: + + if history.get('startYear'): + startDate = datetime.date(history['startYear'], history.get('startMonth', 1), 1) + + if not history['ongoing']: + if history.get('endYear'): + endDate = datetime.date(history['endYear'], history.get('endMonth', 1), 1) + + if history.get('startYear') and history.get('endYear'): + if (endDate - startDate).days <= 0: + raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') diff --git a/api/users/serializers.py b/api/users/serializers.py index ce93278e881..4bf968cbab6 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -1,6 +1,3 @@ -import datetime - -import jsonschema from guardian.models import GroupObjectPermission from django.utils import timezone @@ -17,7 +14,7 @@ from api.files.serializers import QuickFilesSerializer from osf.exceptions import ValidationValueError, ValidationError from osf.models import OSFUser, QuickFilesNode -from api.users.schemas import from_json +from api.users.schemas.utils import validate_user_json class QuickFilesRelationshipField(RelationshipField): @@ -132,38 +129,12 @@ def profile_image_url(self, user): size = self.context['request'].query_params.get('profile_image_size') return user.profile_image_url(size=size) - def _validate_user_json(self, value, json_schema): - try: - jsonschema.validate(value, from_json(json_schema)) - except jsonschema.ValidationError as e: - if len(e.path) > 1: - raise InvalidModelValueError("For '{}' the field value {}".format(e.path[-1], e.message)) - raise InvalidModelValueError(e.message) - except jsonschema.SchemaError as e: - raise InvalidModelValueError(e.message) - - self._validate_dates(value) - - def _validate_dates(self, info): - for history in info: - - if history.get('startYear'): - startDate = datetime.date(history['startYear'], history.get('startMonth', 1), 1) - - if not history['ongoing']: - if history.get('endYear'): - endDate = datetime.date(history['endYear'], history.get('endMonth', 1), 1) - - if history.get('startYear') and history.get('endYear'): - if (endDate - startDate).days <= 0: - raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.') - def validate_employment(self, value): - self._validate_user_json(value, 'employment-schema.json') + validate_user_json(value, 'employment-schema.json') return value def validate_education(self, value): - self._validate_user_json(value, 'education-schema.json') + validate_user_json(value, 'education-schema.json') return value def update(self, instance, validated_data):