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

Jump to bottom

[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 1 commit
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
9 changes: 2 additions & 7 deletions api/users/education-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,12 @@
}
},
"required": [
"ongoing",
"department",
"institution",
"degree"
"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"],
"endYear": ["endMonth"],
"startMonth": ["startYear"],
"startYear": ["startMonth"]
"startMonth": ["startYear"]
}
}
}
9 changes: 2 additions & 7 deletions api/users/employment-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,12 @@
}
},
"required": [
"ongoing",
"department",
"institution",
"title"
"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"],
"endYear": ["endMonth"],
"startMonth": ["startYear"],
"startYear": ["startMonth"]
"startMonth": ["startYear"]
}
}
}
11 changes: 7 additions & 4 deletions api/users/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be

if history.get('startYear'):
	startDate = datetime.date(history['startYear'], history.get('startMonth'), 1)

to prevent KeyErrors.


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.')
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.

👍


Expand Down
60 changes: 59 additions & 1 deletion api_tests/users/views/test_user_detail.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

The frontend doesn't currently allow this:
screen shot 2018-07-30 at 2 37 44 pm

assert getattr(user_one, user_attr) == end_dates_no_start_dates_payload['data']['attributes'][request_key]


@pytest.mark.django_db
class TestUserSchools(UserProfileMixin):
Expand Down