Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PLAT-926] Add employment and institution information to user serializer #8546

Merged
merged 23 commits into from
Jul 31, 2018
Merged

[PLAT-926] Add employment and institution information to user serializer #8546

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
510e465
add allow jobs and schools attributes to be updated and validated
Johnetordoff Jul 13, 2018
b6cf748
add tests for v2 updating of jobs and schools attributes
Johnetordoff Jul 13, 2018
9c735bb
add json schema validation and fix tests for that
Johnetordoff Jul 17, 2018
8ae71f9
remove vestigial code
Johnetordoff Jul 17, 2018
130ff07
remove _update_social unnecessary method
Johnetordoff Jul 17, 2018
270702a
fix comments
Johnetordoff Jul 17, 2018
3c6e4c3
add tests for 403s
Johnetordoff Jul 17, 2018
82ecae8
add new line to json files
Johnetordoff Jul 23, 2018
a37c058
use Django builtin validation methods
Johnetordoff Jul 23, 2018
57dabbf
DRY up tests
Johnetordoff Jul 23, 2018
d7338f3
make start year optional
Johnetordoff Jul 23, 2018
699ec81
remove extraneous code
Johnetordoff Jul 23, 2018
198fae1
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf…
Johnetordoff Jul 24, 2018
aed443d
Merge branch 'master' of https://github.com/CenterForOpenScience/osf.…
Johnetordoff Jul 24, 2018
89178ff
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf…
Johnetordoff Jul 24, 2018
39e0c63
unbreak tests after Travis speed up
Johnetordoff Jul 24, 2018
8fd6d66
update schemas and add more tests
Johnetordoff Jul 25, 2018
344f50a
handle startYear with no startMonth
Johnetordoff Jul 26, 2018
811b7b7
add jsonschema validation for ongoing attribute.
Johnetordoff Jul 26, 2018
b7d9324
make the employment/education schema lock tight and remove some custo…
Johnetordoff Jul 27, 2018
72d4a45
add startYear <- endYear dependency
Johnetordoff Jul 30, 2018
53ab147
check for more string types
Johnetordoff Jul 31, 2018
633aa46
move schemas and make utility functions.
Johnetordoff Jul 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions api/users/education-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we put these in a directory?

api/users/schemas
├── education-schema.json
├── ....schema.json
└── utils.py  # contains the functions for using the schemas -- see previous comments

"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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

},
"endYear": {
"type": "integer",
"minimum": 1900
},
"ongoing": {
"oneOf": [{
"enum": [false],
"required": ["startYear", "endYear"]
},
{
"enum": [true],
"required": ["startYear"],
"not": {
"required": ["endYear"]
}
}
],
"type": "boolean"
},
"department": {
"type": "string"
},
"institution": {
"type": "string",
"minLength": 1
}
},
"required": [
"institution"
],
"additionalProperties": false,
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be dependencies for startMonth and startYear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are not necessary because startMonth and startYear are both always required no matter what, so they're not really dependencies of each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that doesn't match current functionality. I'm able to add an education entry to my profile without a start date.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check these dependencies with functionality on production? Start year and end year can be entered and saved without start month and end month, but months can't be saved without years. I also think there should be a "endYear": "startYear" dependency here because an end year can't be saved unless there's a start year.

"endMonth": ["endYear"],
"startMonth": ["startYear"],
"startYear": ["ongoing"],
"endYear": ["ongoing"],
"endYear": ["startYear"]
}
}
}
62 changes: 62 additions & 0 deletions api/users/employment-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
{
"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": {
"oneOf": [{
"enum": [false],
"required": ["startYear", "endYear"]
},
{
"enum": [true],
"required": ["startYear"],
"not": {
"required": ["endYear"]
}
}
],
"type": "boolean"
},
"department": {
"type": "string"
},
"institution": {
"type": "string",
"minLength": 1
}
},
"required": [
"institution"
],
"additionalProperties": false,
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be dependencies for startMonth and startYear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are not necessary because startMonth and startYear are both always required no matter what, so they're not really dependencies of each other.

"endMonth": ["endYear"],
"startMonth": ["startYear"],
"startYear": ["ongoing"],
"endYear": ["ongoing"],
"endYear": ["startYear"]
}
}
}
8 changes: 8 additions & 0 deletions api/users/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import os
import json

here = os.path.split(os.path.abspath(__file__))[0]

def from_json(fname):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of the helper function. One nitpick: it's a bit of a misnomer. From the name, I'd expect from_json to take a JSON string as input, but it actually takes a filepath. Perhaps load_jsonschema would be more appropriate?

Also see my other comment about co-locating all the jsonschema-related code.

with open(os.path.join(here, fname)) as f:
return json.load(f)
46 changes: 42 additions & 4 deletions api/users/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import datetime

import jsonschema
from guardian.models import GroupObjectPermission

from django.utils import timezone
Expand All @@ -7,13 +10,14 @@
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
from osf.exceptions import ValidationValueError, ValidationError
from osf.models import OSFUser, QuickFilesNode
from api.users.schemas import from_json


class QuickFilesRelationshipField(RelationshipField):
Expand Down Expand Up @@ -57,8 +61,8 @@ 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')
education = JSONAPIListField(required=False, source='schools')
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())

Expand Down Expand Up @@ -128,6 +132,40 @@ 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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure these should be instance methods--there's no usage of self. How about co-locating all the jsonschema validation functions in your new api/users/schemas.py module?

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you post an example of the existing error and why reformatting is necessary/beneficial?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing message does not have the name of the field erroring in it. For example if institution field is an empty string, the error is just to '' is too short. So as you can see the new message indicates the erroring field and what is happening to it.

raise InvalidModelValueError(e.message)
except jsonschema.SchemaError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catching of the different schema errors.

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')
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():
Expand Down
Loading