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

Fix return codes and messages #12

Merged
merged 5 commits into from
Jun 18, 2021
Merged

Conversation

ajoaoff
Copy link

@ajoaoff ajoaoff commented Jun 8, 2021

Fixes #10 and fixes #9.

Description of the change

This PR changes the way error codes are returned, using werkzeug classes.
Also, fix the error where empty items were allowed

Tests were modified to reflect changes in code.

@ajoaoff ajoaoff requested review from a team, rmotitsuki and jab1982 and removed request for a team June 8, 2021 21:18
Copy link

@ArturoQuintana ArturoQuintana left a comment

Choose a reason for hiding this comment

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

We need to apply some changes to cover the same issues but focused on Patching

@@ -84,19 +85,17 @@ def update_mw(self, mw_id):
"""Update a maintenance window."""
Copy link

@ArturoQuintana ArturoQuintana Jun 10, 2021

Choose a reason for hiding this comment

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

The modifications to this method do not cover all the problems specified. For instance, patching the following payloads arises the same problem:

payload1 = {
            "start": start.strftime(TIME_FMT),
            "end": new_time.strftime(TIME_FMT),
            "items": []
        }

and

payload1 = {
            "start": start.strftime(TIME_FMT),
            "end": new_time.strftime(TIME_FMT)
        }

The solution resides in reusing the same approach used in create_mw. I have tested that adding the following code between lines 88 and 89 on main.py solve the issue:

try:            
    maintenance = MW.from_dict(data, self.controller)
except ValueError as err:            
    raise BadRequest(f'{err}')
if maintenance is None:
    raise BadRequest('One or more items are invalid')

Copy link
Author

Choose a reason for hiding this comment

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

Doing it the way you suggested would create a new maintenance instance instead of updating.
The second payload is valid for updating, as you don't need to include all the arguments, just the ones you want to change.

Use werkzeug exception classes to return the correct error
codes.
Also raise a ValueError if no item is provided on creation
Fixes kytos#43
Fix some return codes on update, delete and end maintenance.
Fix kytos#44
@ArturoQuintana
Copy link

@ajoaoff after applying the patches, we have detected that the following payload is still throwing the same error (the return code is 200 instead of 400 as expected for an empty items field):

payload1 = {
            "start": start.strftime(TIME_FMT),
            "end": new_time.strftime(TIME_FMT),
            "items": []
        }

@ajoaoff ajoaoff merged commit 40791be into kytos-ng:master Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants