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

Default response format to JSON #2170

Merged
merged 3 commits into from
Nov 26, 2021
Merged

Conversation

DavisRayM
Copy link
Contributor

@DavisRayM DavisRayM commented Nov 24, 2021

Changes / Features implemented

  • Default the response format on the charts endpoint to JSON if not format is passed

Steps taken to verify this change does what is intended

  • Updated tests

Side effects of implementing this change

The charts endpoint will always return a JSON response if the format query param is empty

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Related to #2151

Main reason for the update is because the browser would still return a Not supported error with the changes introduced in #2151; Which wasn't the ideal scenario for an API.... If the format is not specified we should always default to JSON IMO

denniswambua
denniswambua previously approved these changes Nov 24, 2021
@DavisRayM DavisRayM enabled auto-merge (rebase) November 24, 2021 10:25
@DavisRayM DavisRayM force-pushed the charts-lacking-format-issue branch from 31bad98 to fc67b7c Compare November 24, 2021 11:32
@DavisRayM DavisRayM disabled auto-merge November 24, 2021 14:13
@DavisRayM DavisRayM enabled auto-merge (squash) November 24, 2021 14:14
@@ -160,6 +161,7 @@ def test_get_on_categorized_field(self):
self.assertEqual(response.data['data'][0]['gender'], 'Male')
self.assertEqual(response.data['data'][1]['gender'], 'Female')

@skip("https://github.com/onaio/onadata/pull/2170")
Copy link
Contributor

Choose a reason for hiding this comment

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

@DavisRayM Not sure why we are skipping this

Copy link
Contributor Author

@DavisRayM DavisRayM Nov 25, 2021

Choose a reason for hiding this comment

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

Skipping it because we default to JSON instead of returning a bad request with these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just remove or edit that test?

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 can remove it, it's a bit similar to one of the other tests so I wasn't so sure about editing it. Let me remove it and re-request review

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

- Skip `test_return_bad_request_on_non_json_request_with_field_name`
  test
@DavisRayM DavisRayM force-pushed the charts-lacking-format-issue branch from 6268be7 to b6990f8 Compare November 25, 2021 07:08
@DavisRayM DavisRayM merged commit 7715263 into master Nov 26, 2021
@DavisRayM DavisRayM deleted the charts-lacking-format-issue branch November 26, 2021 08:27
@DavisRayM DavisRayM mentioned this pull request Nov 26, 2021
1 task
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.

2 participants