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

Add period mismatch error handling to API #1026

Merged
merged 3 commits into from
Jul 28, 2021
Merged

Add period mismatch error handling to API #1026

merged 3 commits into from
Jul 28, 2021

Conversation

HAEKADI
Copy link
Contributor

@HAEKADI HAEKADI commented Jul 23, 2021 •
edited
Loading

Connected to #916

Bug fix:

  • [Web API] Handle a period mismatch error:
    • The exception expected by a period mismatch error was not the good one.
    • A period mismatch error renders a 500 internal server error instead of a 400 bad request error since it's caused by the user.

@HAEKADI HAEKADI requested a review from sandcha July 23, 2021 11:45
bonjourmauko
bonjourmauko reviewed Jul 23, 2021
View reviewed changes
Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Looks good :) a couple of suggestions.

openfisca_web_api/app.py Show resolved Hide resolved
openfisca_web_api/app.py Outdated Show resolved Hide resolved
@bonjourmauko bonjourmauko self-assigned this Jul 23, 2021
@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Jul 23, 2021
@HAEKADI HAEKADI marked this pull request as draft July 23, 2021 14:19
@bonjourmauko bonjourmauko added kind:fix Bugs are defects and failure demand. and removed kind:feat A feature request, a feature deprecation labels Jul 23, 2021
bonjourmauko
bonjourmauko requested changes Jul 23, 2021
View reviewed changes
Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Now a proper review 😃

  • Code looks good to me 🙌
  • Would it be possible to add a test?

Normally, with all API endpoints, we should be able to test that they handle success/error properly —hence the bug here.

I think we're currently just testing for success, but not for error https://github.com/openfisca/openfisca-core/blob/master/tests/web_api/test_calculate.py#L287-L314

@HAEKADI HAEKADI self-assigned this Jul 23, 2021
@HAEKADI HAEKADI requested a review from bonjourmauko July 27, 2021 13:59
@HAEKADI HAEKADI marked this pull request as ready for review July 27, 2021 13:59
bonjourmauko
bonjourmauko approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Great! Thanks for this contribution @HAEKADI 😃

@bonjourmauko bonjourmauko merged commit f395c30 into master Jul 28, 2021
@bonjourmauko bonjourmauko deleted the error-handling-916 branch July 28, 2021 17:54
@bonjourmauko bonjourmauko mentioned this pull request Jul 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonjourmauko bonjourmauko bonjourmauko approved these changes

@sandcha sandcha Awaiting requested review from sandcha

Assignees

@bonjourmauko bonjourmauko

@HAEKADI HAEKADI

Labels
kind:fix Bugs are defects and failure demand.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HAEKADI @bonjourmauko