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

[ENG-965] Feature/education employment endpoints #9163

Open
wants to merge 12 commits into
base: feature/norm_education_employment
Choose a base branch
from

Conversation

UdayVarkhedkar
Copy link
Contributor

Purpose

Verify and update UserEducation and UserEmployment endpoint functionality, request validation, and testing.

Changes

  • Fix undefined import
  • Clean date validation
  • Separate profile_type and object_type in endpoint testing and update test functions to fix broken tests (tests were calling invalid functions/attributes)
  • Require ongoing parameter to be defined if start_date is provided (via jsonschema validation) and add tests accordingly

QA Notes

The UserEducation and UserEmployment API endpoints should be tested/verified by QA. This includes checking the CRUD (create, read, update, and delete) functionality of the API endpoints, verifying that authorization for the API endpoints is in line with expectations, ensuring that parameter validation is working as expected or that helpful errors are provided.

Documentation

No, documentation for these endpoints does not currently exist.

Side Effects

N/A

Ticket

Jira Ticket

@UdayVarkhedkar UdayVarkhedkar changed the title Feature/education employment endpoints [ENG-965] Feature/education employment endpoints Sep 23, 2019
# start date with undefined ongoing fails
undefined_ongoing = payload(institution='hullo', start_date='2018-01-01')
res = app.post_json(list_url, undefined_ongoing, auth=user.auth, expect_errors=True)
assert res.status_code == 400
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition to this test.

@corbinSanders
Copy link
Contributor

Looks good! As we discussed in my PR, I like your implementation of validate_dates better!

@corbinSanders
Copy link
Contributor

Looks good!

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Only one really minor change request. So minor.

if history.get('start_date') and history.get('end_date'):
if (end_date - start_date).days <= 0:
raise InvalidModelValueError(detail='End date must be greater than or equal to the start date.')
if not history.get('ongoing', False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're here, could you make this a positive if rather than the negative case, so it's a bit more readable? So if history.get('ongoing', False): and do the other branch, then put this branch in the else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified and changes have been tested locally

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

🎉 🐧 🎉

@brianjgeiger
Copy link
Collaborator

@UdayVarkhedkar Tests are not happy. Are those being handled by one of the related PRs?

@UdayVarkhedkar
Copy link
Contributor Author

Yes, the failing tests are the ones being removed by this PR by Corbin. Tests around the API functionality added/validated by this PR have been run and verified locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants