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.

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
Show file tree
Hide file tree
Changes from 3 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
52 changes: 52 additions & 0 deletions api/users/education-schema.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{
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": {
"type": "boolean"
},
"department": {
"type": "string"
},
"institution": {
"type": "string",
"minLength": 1
}
},
"required": [
"startYear",
"startMonth",
"ongoing",
"department",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should department and degree be in this list? I'm able to add an education entry to my profile with only "Institution" set.

"institution",
"degree"
],
"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"],
"endYear": ["endMonth"]
}
}
}
52 changes: 52 additions & 0 deletions api/users/employment-schema.json
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above: should department and title be in this list?

"institution",
"title"
],
"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"],
"endYear": ["endMonth"]
}
}
}
8 changes: 8 additions & 0 deletions api/users/schemas.py
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)
59 changes: 55 additions & 4 deletions api/users/serializers.py
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,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'}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove employment_required_keys and education_required_keys since we're using jsonschema.

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 +134,44 @@ 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

This was pulled from the first part of the update method? doesn't look like it's being used.

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):
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:

startDate = datetime.date(history['startYear'], history['startMonth'], 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed above: startMonth and endMonth aren't required so these lines will throw KeyErrors


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.')
elif history.get('endYear') or history.get('endMonth'):
raise InvalidModelValueError(detail='Ongoing positions cannot have end dates.')
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe adding the following to the json schema will validate this for you:

"dependencies": {
	"endMonth": ["endYear"],
	"startMonth": ["startYear"],
    "endYear": ["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"]
}

The "not" will ensure that "endYear" is not included if "ongoing" is true.

The "anyOf" will ensure that "startYear" is required if "ongoing" is true and that "endYear" is included if "ongoing" is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hesitant to add this to the schema because:
A) It's hard for an outside observer to immediately know what this is for and I can't comment inside a JSON file to make this clearer.
B) The error message this produces isn't really human readable, getting back to the current message is likely going to include a lot of catching and re-throwing.

But there's definitely a benefit to always relying using the schema, so 🤷‍♂️ .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I definitely agree that the error messages are way less than ideal. My opinion is that using the schema for validation is a lot nicer than writing our own, and we can document this in a more human-readable format in the dev docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


def update(self, instance, validated_data):
assert isinstance(instance, OSFUser), 'instance must be a User'
for attr, value in validated_data.items():
Expand All @@ -142,6 +186,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, 'education-schema.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to reduce the amount of logic that happens in this method (and better separate logic), you could implement field level validators for schools and jobs (each method would just call self._validate_user_json(...)).

See @erinspace's work in #8551 for an example of this.

instance.schools = value
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these assignment statements are necessary anymore. The fields will be set by the setattr(instance, attr, value) call below.

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